Merge branch 'fix/parental-pin-and-device-transfer' into 'main'

Stretch the parental PIN, and keep app data off new phones

See merge request paulhitt/flamenet-messenger-android!152

app/src/main/AndroidManifest.xml

@@ -62,6 +62,7 @@
 
     <application
         android:allowBackup="false"
+        android:dataExtractionRules="@xml/data_extraction_rules"
         android:icon="@mipmap/ic_launcher"
         android:roundIcon="@mipmap/ic_launcher_round"
         android:label="@string/app_name"
@@ -74,6 +75,13 @@
           backed up; the encrypted records would be. Restoring those onto a
           different device would produce a vault nothing can open, which
           presents as a corrupt install rather than as a missing key.
+
+          On its own it no longer did that job. From Android 12, for an app
+          targeting 31 or higher, some manufacturers' phones still move app
+          data to a new phone with allowBackup off, and this app targets 35.
+          dataExtractionRules is what now keeps every domain out of both cloud
+          backup and device-to-device transfer. allowBackup stays for the
+          Android versions before 12, where it is still the whole answer.
         -->
 
         <!--

app/src/main/kotlin/io/flamenet/messenger/app/ParentalControls.kt

@@ -1,16 +1,37 @@
 package io.flamenet.messenger.app
 
 import android.content.Context
+import io.flamenet.e2e.AccountKdf
 import java.security.MessageDigest
 import java.security.SecureRandom
 
 /**
  * Device-side supervision a parent configures on a child's phone.
+ *
+ * The PIN is stretched with [AccountKdf.master], PBKDF2-SHA256 at 650,000
+ * iterations, the same function the account passphrase uses and the same one
+ * the iPhone app moved to on 2026-09-25. Until then this was ONE round of
+ * SHA-256 with a salt beside it: four digits is ten thousand candidates and a
+ * single SHA-256 is nanoseconds, so whoever could read this preferences file
+ * had the PIN. The file does not leave the device (see
+ * `res/xml/data_extraction_rules.xml`), which is what keeps it out of reach;
+ * the stretch is what makes a copy that did get out expensive to use.
+ *
+ * Stretching is slow on purpose, and it runs only at the moments somebody
+ * has just typed a PIN and tapped, which is where a pause is expected.
  */
 object ParentalControls {
     private const val PREFS = "parental-controls"
     private const val PIN_HASH = "pinHash"
     private const val PIN_SALT = "pinSalt"
+
+    /**
+     * Which way [PIN_HASH] was made. Absent on every record written before the
+     * stretch, which is how those are recognised and upgraded.
+     */
+    private const val PIN_KDF = "pinKdf"
+    private const val KDF_PBKDF2 = "pbkdf2-sha256-650000"
+
     const val CONTACTS_GATED = "contactsGated"
     const val STORIES_HIDDEN = "storiesHidden"
     const val CALLS_DISABLED = "callsDisabled"
@@ -40,8 +61,9 @@ object ParentalControls {
         val salt = ByteArray(16).also(SecureRandom()::nextBytes)
         prefs(context)
             .edit()
-            .putString(PIN_SALT, salt.joinToString("") { "%02x".format(it) })
+            .putString(PIN_SALT, salt.toHex())
-            .putString(PIN_HASH, hash(pin, salt))
+            .putString(PIN_HASH, stretch(pin, salt).toHex())
+            .putString(PIN_KDF, KDF_PBKDF2)
             // The gates default ON, a parent turning this on wants supervision,
             // and each is individually releasable afterwards.
             .putBoolean(CONTACTS_GATED, true)
@@ -54,21 +76,56 @@ object ParentalControls {
         context: Context,
         pin: String,
     ): Boolean {
-        val stored = prefs(context).getString(PIN_HASH, null) ?: return false
+        val p = prefs(context)
-        val saltHex = prefs(context).getString(PIN_SALT, null) ?: return false
+        val stored = p.getString(PIN_HASH, null)?.fromHex() ?: return false
-        val salt = saltHex.chunked(2).map { it.toInt(16).toByte() }.toByteArray()
+        val salt = p.getString(PIN_SALT, null)?.fromHex() ?: return false
-        return hash(pin, salt) == stored
+        val stretched = p.getString(PIN_KDF, null) == KDF_PBKDF2
+
+        val candidate = if (stretched) stretch(pin, salt) else legacySha256(pin, salt)
+        // Constant time. `==` on the hex strings stopped at the first
+        // differing character.
+        if (!MessageDigest.isEqual(candidate, stored)) return false
+
+        // A PIN set before the stretch is rewritten the first time it is
+        // entered, which is the only moment it can be: the old hash cannot be
+        // upgraded without the PIN in hand. Only the three PIN fields change.
+        // Not `enable`, which turns every gate back on, and a parent who had
+        // released the calls gate would find it back.
+        if (!stretched) {
+            val fresh = ByteArray(16).also(SecureRandom()::nextBytes)
+            p
+                .edit()
+                .putString(PIN_SALT, fresh.toHex())
+                .putString(PIN_HASH, stretch(pin, fresh).toHex())
+                .putString(PIN_KDF, KDF_PBKDF2)
+                .apply()
+        }
+        return true
     }
 
     fun disable(context: Context) = prefs(context).edit().clear().apply()
 
-    private fun hash(
+    private fun stretch(
         pin: String,
         salt: ByteArray,
-    ): String =
+    ): ByteArray = AccountKdf.master(pin, salt)
+
+    /** The format every PIN had before 2026-09-25. Read only, to upgrade it. */
+    private fun legacySha256(
+        pin: String,
+        salt: ByteArray,
+    ): ByteArray =
         MessageDigest
             .getInstance("SHA-256")
             .apply { update(salt) }
             .digest(pin.toByteArray(Charsets.UTF_8))
-            .joinToString("") { "%02x".format(it) }
+
+    private fun ByteArray.toHex() = joinToString("") { "%02x".format(it) }
+
+    private fun String.fromHex(): ByteArray? =
+        if (length % 2 != 0) {
+            null
+        } else {
+            runCatching { chunked(2).map { it.toInt(16).toByte() }.toByteArray() }.getOrNull()
+        }
 }

app/src/main/res/xml/data_extraction_rules.xml

@@ -0,0 +1,49 @@
+<?xml version="1.0" encoding="utf-8"?>
+<!--
+  Nothing this app stores leaves the phone, by backup or by transfer.
+
+  `android:allowBackup="false"` in the manifest used to be the whole answer,
+  and from Android 12 it is not. Android's own documentation: for apps
+  targeting API 31 or higher, on devices from some manufacturers it "disables
+  cloud-based backup and restore ... but doesn't disable device-to-device
+  transfers for the app". This app targets 35. So a phone-to-phone move could
+  carry the vault's encrypted records to a new phone without the Keystore key
+  that opens them, which is the unopenable vault the manifest comment was
+  written to prevent, and it would carry the plaintext preferences with it.
+
+  Every domain is excluded from both modes, including the device-protected
+  ones. Leaving a section out does NOT turn it off: "If there are no rules for
+  a particular backup mode, such as if the <device-transfer> section is
+  missing, that mode is fully enabled for all content." That is the mistake
+  this file exists to not make, so both sections are spelled out in full.
+
+  No <cross-platform-transfer>. It exists from Android 16 QPR2 (API 36.1),
+  this app compiles against 35, and it transfers only to an iOS app named by
+  bundle and team id in <platform-specific-params>. Naming none is naming no
+  destination. If this app ever targets 36.1, revisit this rather than
+  assume.
+-->
+<data-extraction-rules>
+    <cloud-backup>
+        <exclude domain="root" path="." />
+        <exclude domain="file" path="." />
+        <exclude domain="database" path="." />
+        <exclude domain="sharedpref" path="." />
+        <exclude domain="external" path="." />
+        <exclude domain="device_root" path="." />
+        <exclude domain="device_file" path="." />
+        <exclude domain="device_database" path="." />
+        <exclude domain="device_sharedpref" path="." />
+    </cloud-backup>
+    <device-transfer>
+        <exclude domain="root" path="." />
+        <exclude domain="file" path="." />
+        <exclude domain="database" path="." />
+        <exclude domain="sharedpref" path="." />
+        <exclude domain="external" path="." />
+        <exclude domain="device_root" path="." />
+        <exclude domain="device_file" path="." />
+        <exclude domain="device_database" path="." />
+        <exclude domain="device_sharedpref" path="." />
+    </device-transfer>
+</data-extraction-rules>

app/src/test/kotlin/io/flamenet/messenger/app/NothingLeavesThePhoneTest.kt

@@ -0,0 +1,69 @@
+package io.flamenet.messenger.app
+
+import org.w3c.dom.Element
+import java.io.File
+import javax.xml.parsers.DocumentBuilderFactory
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertTrue
+
+/**
+ * App data stays on the phone: no cloud backup, and no phone-to-phone
+ * transfer.
+ *
+ * `allowBackup="false"` alone stopped being enough on Android 12 for an app
+ * targeting 31 or higher, and this one targets 35. The data extraction rules
+ * are what carry the promise now, and they fail OPEN: a missing section, or
+ * one with no rules in it, means that mode transfers everything. So this
+ * checks every domain in both sections rather than that the file exists.
+ */
+class NothingLeavesThePhoneTest {
+    private val domains =
+        setOf(
+            "root", "file", "database", "sharedpref", "external",
+            "device_root", "device_file", "device_database", "device_sharedpref",
+        )
+
+    private val rules = File("src/main/res/xml/data_extraction_rules.xml")
+    private val manifest = File("src/main/AndroidManifest.xml")
+
+    private fun section(name: String): Element? {
+        val doc =
+            DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(rules)
+        return doc.getElementsByTagName(name).item(0) as? Element
+    }
+
+    private fun excludedDomains(section: Element): Set<String> {
+        val nodes = section.getElementsByTagName("exclude")
+        return (0 until nodes.length)
+            .map { nodes.item(it) as Element }
+            .filter { it.getAttribute("path") == "." }
+            .map { it.getAttribute("domain") }
+            .toSet()
+    }
+
+    @Test
+    fun `the rules file is read at all`() {
+        assertTrue(rules.isFile, "no data_extraction_rules.xml, so nothing below means anything")
+        assertTrue(rules.readText().contains("<data-extraction-rules>"))
+    }
+
+    @Test
+    fun `every domain is kept out of cloud backup and device transfer`() {
+        for (mode in listOf("cloud-backup", "device-transfer")) {
+            val s = section(mode)
+            // A missing section is not "off", it is "everything".
+            assertTrue(s != null, "no <$mode> section, so that mode transfers everything")
+            assertEquals(domains, excludedDomains(s!!), "<$mode> leaves a domain out")
+            assertEquals(0, s.getElementsByTagName("include").length, "<$mode> includes something")
+        }
+    }
+
+    @Test
+    fun `the manifest points at the rules and still turns backup off`() {
+        val m = manifest.readText()
+        assertTrue(m.contains("android:dataExtractionRules=\"@xml/data_extraction_rules\""))
+        // Still the whole answer on the Android versions before 12.
+        assertTrue(m.contains("android:allowBackup=\"false\""))
+    }
+}

app/src/test/kotlin/io/flamenet/messenger/app/ParentalPinIsStretchedTest.kt

@@ -0,0 +1,139 @@
+package io.flamenet.messenger.app
+
+import android.content.Context
+import io.flamenet.e2e.AccountKdf
+import org.junit.runner.RunWith
+import org.robolectric.RobolectricTestRunner
+import org.robolectric.RuntimeEnvironment
+import org.robolectric.annotation.Config
+import java.io.File
+import java.security.MessageDigest
+import kotlin.test.BeforeTest
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertFalse
+import kotlin.test.assertNotEquals
+import kotlin.test.assertNull
+import kotlin.test.assertTrue
+
+/**
+ * The parental PIN is stretched, and a PIN set before that still works.
+ *
+ * Until 2026-09-25 this was one round of SHA-256 with the salt beside it, the
+ * same finding the iPhone app fixed that day. Four digits is ten thousand
+ * candidates, so a single SHA-256 left the PIN readable by anybody who could
+ * read the file. These tests read the raw preferences rather than asking
+ * `verify`, because a `verify` that still hashed once would pass every
+ * right-PIN/wrong-PIN check there is.
+ */
+@RunWith(RobolectricTestRunner::class)
+@Config(sdk = [34])
+class ParentalPinIsStretchedTest {
+    private val context: Context get() = RuntimeEnvironment.getApplication()
+    private val prefs get() = context.getSharedPreferences("parental-controls", Context.MODE_PRIVATE)
+
+    private fun hex(b: ByteArray) = b.joinToString("") { "%02x".format(it) }
+
+    private fun unhex(s: String) = s.chunked(2).map { it.toInt(16).toByte() }.toByteArray()
+
+    private fun sha256Once(
+        pin: String,
+        salt: ByteArray,
+    ) = MessageDigest.getInstance("SHA-256").apply { update(salt) }.digest(pin.toByteArray())
+
+    /** Write a record exactly as every build before the stretch did. */
+    private fun seedLegacy(
+        pin: String,
+        contacts: Boolean = true,
+        stories: Boolean = true,
+        calls: Boolean = true,
+    ): String {
+        val salt = ByteArray(16) { it.toByte() }
+        val hash = hex(sha256Once(pin, salt))
+        prefs
+            .edit()
+            .clear()
+            .putString("pinSalt", hex(salt))
+            .putString("pinHash", hash)
+            .putBoolean(ParentalControls.CONTACTS_GATED, contacts)
+            .putBoolean(ParentalControls.STORIES_HIDDEN, stories)
+            .putBoolean(ParentalControls.CALLS_DISABLED, calls)
+            .commit()
+        return hash
+    }
+
+    @BeforeTest
+    fun clean() {
+        prefs.edit().clear().commit()
+    }
+
+    @Test
+    fun `a new PIN is stretched, not hashed once`() {
+        ParentalControls.enable(context, "4821")
+        val salt = unhex(prefs.getString("pinSalt", null)!!)
+        val stored = prefs.getString("pinHash", null)!!
+
+        assertEquals("pbkdf2-sha256-650000", prefs.getString("pinKdf", null))
+        assertEquals(hex(AccountKdf.master("4821", salt)), stored, "not the account KDF")
+        assertNotEquals(hex(sha256Once("4821", salt)), stored, "still one round of SHA-256")
+    }
+
+    @Test
+    fun `the right PIN opens it and the wrong one does not`() {
+        ParentalControls.enable(context, "4821")
+        assertTrue(ParentalControls.verify(context, "4821"))
+        assertFalse(ParentalControls.verify(context, "4820"))
+        assertFalse(ParentalControls.verify(context, ""))
+    }
+
+    @Test
+    fun `a PIN set before the stretch still opens, and is upgraded`() {
+        val oldHash = seedLegacy("1234")
+        assertNull(prefs.getString("pinKdf", null), "the seed is not a legacy record")
+
+        assertTrue(ParentalControls.verify(context, "1234"), "an existing PIN stopped working")
+
+        assertEquals("pbkdf2-sha256-650000", prefs.getString("pinKdf", null), "not upgraded")
+        val salt = unhex(prefs.getString("pinSalt", null)!!)
+        val newHash = prefs.getString("pinHash", null)!!
+        assertNotEquals(oldHash, newHash)
+        assertEquals(hex(AccountKdf.master("1234", salt)), newHash)
+        // And it still opens afterwards, now through the stretched path.
+        assertTrue(ParentalControls.verify(context, "1234"))
+    }
+
+    @Test
+    fun `a wrong PIN on an old record changes nothing`() {
+        val oldHash = seedLegacy("1234")
+        assertFalse(ParentalControls.verify(context, "9999"))
+        assertNull(prefs.getString("pinKdf", null), "upgraded on a wrong PIN")
+        assertEquals(oldHash, prefs.getString("pinHash", null))
+    }
+
+    /**
+     * The trap the iPhone port named: upgrading through `enable` would turn
+     * every gate back on, and a parent who had released one would find it
+     * restored without having touched it.
+     */
+    @Test
+    fun `upgrading keeps the gates as the parent left them`() {
+        seedLegacy("1234", contacts = false, stories = true, calls = false)
+        assertTrue(ParentalControls.verify(context, "1234"))
+        assertFalse(prefs.getBoolean(ParentalControls.CONTACTS_GATED, true), "contacts gate came back on")
+        assertTrue(prefs.getBoolean(ParentalControls.STORIES_HIDDEN, false), "stories gate was dropped")
+        assertFalse(prefs.getBoolean(ParentalControls.CALLS_DISABLED, true), "calls gate came back on")
+    }
+
+    /**
+     * `==` on the hex strings stopped at the first differing character. Read
+     * from source because timing is not something a unit test can measure
+     * reliably, and this is the line that decides it.
+     */
+    @Test
+    fun `the comparison is constant time`() {
+        val source = File("src/main/kotlin/io/flamenet/messenger/app/ParentalControls.kt").readText()
+        assertTrue(source.contains("fun verify("), "ParentalControls.kt was not read")
+        assertTrue(source.contains("MessageDigest.isEqual(candidate, stored)"))
+        assertFalse(source.contains("== stored"), "a short-circuiting comparison is back")
+    }
+}