From 4a587a7d4d06e9059ec02e850e5c32abcc4b9a1c Mon Sep 17 00:00:00 2001 From: Paul Hitt Date: Fri, 4 Sep 2026 13:43:39 -0400 Subject: [PATCH] Zero key material after use (ios#39, Android half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stretched master key in AccountKdf is an intermediate nobody needs after the HKDF expansion, and it is the most valuable thing in that file: every other account secret is derived from it. It now lives inside `SecureBytes.withSecret`, which wipes it however the block leaves — including out of a throw, because a derivation that failed half-way is exactly where material gets forgotten. 🔴 The Android guarantee is WEAKER than the iOS one, and PARITY.md says 🟡 rather than rounding it up to ✅. iOS has `memset_s`, specified not to be optimised away, and does not page application memory to disk. Neither holds on the JVM: a compacting collector can relocate a ByteArray and leave the bytes at the old address untouched, Android has had compressed swap on by default for years, and `Arrays.fill` is only "not eliminated in practice" rather than guaranteed. There is no way to pin a Java array, so that is a ceiling rather than a gap to close later. So the claim is written narrowly: a derived key is not left sitting in the heap after use. Not "the key is gone". Spending the value of this feature on a stronger claim than it supports would be the worst possible trade. ⚠️ `signInSecrets` does BOTH expansions inside the block. Wiping between them would derive the account seed from thirty-two zero bytes — a perfectly consistent, perfectly wrong identity, on every device, silently. That is a far worse failure than the leak this narrows, and `signInSecrets` had no pinned vector of its own, so a test compares the pair-path against the individual derivations. Proven: moving the wipe one line earlier turns it red. ⚠️ The limit is asserted rather than left to prose — a copy taken before a wipe survives it, and that is a passing test so nobody reads `wipe` as stronger than it is. 10 unit tests. App builds, every JVM suite green including the pinned KDF vectors. Co-Authored-By: Claude Opus 5 --- PARITY.md | 15 +- .../main/kotlin/io/flamenet/e2e/AccountKdf.kt | 38 +++-- .../kotlin/io/flamenet/e2e/SecureBytes.kt | 78 ++++++++++ .../kotlin/io/flamenet/e2e/SecureBytesTest.kt | 140 ++++++++++++++++++ 4 files changed, 256 insertions(+), 15 deletions(-) create mode 100644 engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt create mode 100644 engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt diff --git a/PARITY.md b/PARITY.md index 7ba7ccd..ff3b21d 100644 --- a/PARITY.md +++ b/PARITY.md @@ -160,7 +160,7 @@ being the one that leaks. |---|:--:|:--:|---| | Back off on repeated app-lock attempts | ✅ | ✅ | ios#41 — 3 free attempts, then 30s doubling to a 5-minute ceiling | | Handle a wrong clock | ✅ | ✅ | ios#45 — skew learned free from the `Date` header; expiry takes the LATER of device and relay | -| Zero key material after use | ✅ | ⚪ | ios#39 | +| Zero key material after use | ✅ | 🟡 | ios#39 — done, but the JVM guarantee is **weaker than iOS**; see below | | Pin the relay's identity | ✅ | ⚪ | ios#19 | ⚠️ **The back-off is not a defence of the data.** It slows a person holding an @@ -185,6 +185,19 @@ the UI thread, so the two fields are `@Volatile` and updated together under a lock. A torn read of "skew from now, timestamp from an hour ago" would age out an observation just made, or trust one that had not been. +🔴 **Zeroing is where Android cannot match iOS, and the row says 🟡 for that +reason rather than being rounded up to ✅.** iOS has `memset_s`, specified not to +be optimised away, and iOS does not page application memory to disk. Neither +holds here: a compacting collector can relocate a `ByteArray` and leave the +bytes at the old address untouched, Android has had compressed swap on by +default for years, and `Arrays.fill` is merely "not eliminated in practice" +rather than guaranteed. There is no way to pin a Java array, so this is a +ceiling on what any implementation here can offer — not a gap to close later. + +The claim Android supports is therefore narrower and is written that way in +`SecureBytes`: **a derived key is not left sitting in the heap after use.** Not +"the key is gone". + 🔴 One divergence from iOS, and it is a bug the Swift version cannot have. iOS computes `30 * pow(2, steps)` in a `Double`, which saturates to infinity and still caps correctly. Kotlin shifting a `Long` does not: `30L shl 60` wraps diff --git a/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt b/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt index dc5b87b..2b075a5 100644 --- a/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt +++ b/engine/src/main/kotlin/io/flamenet/e2e/AccountKdf.kt @@ -133,12 +133,17 @@ object AccountKdf { context: String, length: Int = 32, params: Params = Params.PBKDF2_V1, - ): ByteArray = Hkdf.derive( - ikm = master(passphrase, salt, params), - salt = ByteArray(0), - info = context.toByteArray(Charsets.UTF_8), - length = length, - ) + ): ByteArray = SecureBytes.withSecret(master(passphrase, salt, params)) { ikm -> + // ios#39 — the stretched master is an intermediate nobody needs after + // the expansion, and it is the most valuable thing in this file: every + // other secret is derived from it. Wiped however this leaves. + Hkdf.derive( + ikm = ikm, + salt = ByteArray(0), + info = context.toByteArray(Charsets.UTF_8), + length = length, + ) + } /** What gets sent to the account service. */ fun authSecret(passphrase: String, salt: ByteArray, params: Params = Params.PBKDF2_V1): ByteArray = @@ -179,14 +184,19 @@ object AccountKdf { salt: ByteArray, params: Params = Params.PBKDF2_V1, ): Pair { - val ikm = master(passphrase, salt, params) - fun expand(context: String) = Hkdf.derive( - ikm = ikm, - salt = ByteArray(0), - info = context.toByteArray(Charsets.UTF_8), - length = 32, - ) - return expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY) + return SecureBytes.withSecret(master(passphrase, salt, params)) { ikm -> + fun expand(context: String) = Hkdf.derive( + ikm = ikm, + salt = ByteArray(0), + info = context.toByteArray(Charsets.UTF_8), + length = 32, + ) + // ⚠️ BOTH expansions happen inside the block. Returning the pair and + // wiping afterwards would be the same thing; expanding after the + // wipe would derive two keys from thirty-two zero bytes, which is + // the failure mode this shape makes impossible to write. + expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY) + } } /** diff --git a/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt b/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt new file mode 100644 index 0000000..381e99d --- /dev/null +++ b/engine/src/main/kotlin/io/flamenet/e2e/SecureBytes.kt @@ -0,0 +1,78 @@ +package io.flamenet.e2e + +import java.util.Arrays + +/** + * Zeroing key material once it has been used (ios#39). + * + * 🔴 **This narrows a window. It does not remove the material, and claiming + * otherwise would spend the whole value of the feature on a lie.** What it buys + * is that a derived key does not sit in the process for the lifetime of the app + * after the one moment it was needed — which matters for a heap dump, a crash + * report, or a debugger attached later. It buys nothing against somebody + * reading memory *while* the secret is in use. + * + * 🔴 **The JVM guarantee is weaker than the iOS one, and the difference is not + * a detail.** iOS has `memset_s`, which is specified not to be optimised away, + * and iOS does not page application memory to disk. Neither holds here: + * + * - **A compacting garbage collector moves objects.** A `ByteArray` can be + * relocated between allocation and wiping, and the bytes at the old address + * are not cleared by the move. Zeroing reaches the array you are holding; it + * cannot reach a copy the runtime made on its own. There is no way to pin a + * Java array, so this is a ceiling on what any implementation here can offer. + * - **Android has compressed swap.** zram has been on by default for years, so + * a page holding key material can reach a backing store — unlike iOS, where + * the platform does not page application memory out at all. + * - **There is no `memset_s`.** `Arrays.fill` on an array that escapes the + * method is not eliminated in practice, because the JIT cannot prove the + * write is dead. "Not in practice" is a weaker statement than the C standard + * makes, and it is the honest one. + * + * ⚠️ **A Kotlin `String` cannot be zeroed**, same as Swift and for the same + * reasons: immutable, freely copied, and possibly interned. A passphrase that + * arrived as a `String` is beyond reach. The right response is to turn it into + * bytes early, wipe those, and not pretend about the original — which is why + * `AccountKdf.master` already calls `PBEKeySpec.clearPassword()` rather than + * trying to do anything about the `String` it was handed. + * + * So the claim this type supports, precisely: **a derived key is not left + * sitting in the heap after use.** Not "the key is gone". + */ +object SecureBytes { + + /** + * Zero this array in place. + * + * ⚠️ Reaches THIS array only. Anything that copied it beforehand keeps an + * untouched copy, and the runtime may have made copies nobody asked for. + */ + fun wipe(bytes: ByteArray?) { + if (bytes == null || bytes.isEmpty()) return + Arrays.fill(bytes, 0) + } + + /** + * Run [body] with key material and wipe it however [body] leaves. + * + * ⚠️ Wipes on the way out of a throw as well. A derivation that failed + * half-way is exactly the case where the material is most likely to be + * forgotten — which is why this is `try/finally` rather than a wipe at the + * end of a happy path. + */ + inline fun withSecret(secret: ByteArray, body: (ByteArray) -> T): T = + try { + body(secret) + } finally { + wipe(secret) + } + + /** + * A copy of [source], for callers that genuinely need one. + * + * 🔴 The copy is ordinary memory and this object stops being responsible + * for it. Every use of this is a decision to leave a second copy behind, + * and it is spelled out rather than implicit for that reason. + */ + fun copyOf(source: ByteArray): ByteArray = source.copyOf() +} diff --git a/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt b/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt new file mode 100644 index 0000000..4c89295 --- /dev/null +++ b/engine/src/test/kotlin/io/flamenet/e2e/SecureBytesTest.kt @@ -0,0 +1,140 @@ +package io.flamenet.e2e + +import kotlin.test.Test +import kotlin.test.assertContentEquals +import kotlin.test.assertEquals +import kotlin.test.assertFailsWith +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * Zeroing key material once it has been used (ios#39, Android half). + * + * 🔴 **The claim being tested is narrow, and the tests are shaped to keep it + * narrow.** What this buys is that a derived key is not left sitting in the + * heap after the one moment it was needed. It is not "the key is gone" — a + * compacting collector can have moved the array, Android has compressed swap, + * and there is no `memset_s` on the JVM. `SecureBytes` says all of that in + * prose; these assert only what is actually true. + * + * 🔴 **The most valuable test here is that the derived keys did not change.** + * Wiping an intermediate one line too early derives every account secret from + * thirty-two zero bytes — which produces a perfectly consistent, perfectly + * wrong identity, silently, on every device. That is a far worse failure than + * the leak this feature exists to narrow, so it is checked first. + */ +class SecureBytesTest { + + private val passphrase = "a-long-enough-passphrase" + private val salt = ByteArray(16) { it.toByte() } + + // ---- The failure that would matter most ----------------------------- + + /** + * 🔴 The pinned vectors already cover `authSecret` and `accountKeySeed`. + * This covers the shape wiping could break without moving a vector: that + * the two secrets from one stretch still match the two derived separately. + * If the wipe landed between the expansions, the second would be derived + * from zeroes and this diverges. + */ + @Test + fun `wiping the intermediate did not change what is derived`() { + val (auth, account) = AccountKdf.signInSecrets(passphrase, salt) + assertContentEquals(AccountKdf.authSecret(passphrase, salt), auth) + assertContentEquals(AccountKdf.accountKeySeed(passphrase, salt), account) + } + + /** + * ⚠️ And neither is zeroes, which is what "derived from a wiped buffer" + * would look like. A test that only compared the two paths would pass if + * BOTH were wrong in the same way. + */ + @Test + fun `no derived secret is a run of zeroes`() { + val (auth, account) = AccountKdf.signInSecrets(passphrase, salt) + assertFalse(auth.all { it == 0.toByte() }, "the auth secret is all zeroes") + assertFalse(account.all { it == 0.toByte() }, "the account seed is all zeroes") + assertFalse( + AccountKdf.identityKey(passphrase, salt).all { it == 0.toByte() }, + "the identity key is all zeroes", + ) + } + + /** Derivation stays deterministic across calls, wiping or not. */ + @Test + fun `derivation is still deterministic`() { + assertContentEquals( + AccountKdf.identityKey(passphrase, salt), + AccountKdf.identityKey(passphrase, salt), + ) + } + + /** Different contexts still separate, so the wipe did not collapse them. */ + @Test + fun `the contexts still produce different secrets`() { + val auth = AccountKdf.authSecret(passphrase, salt) + val identity = AccountKdf.identityKey(passphrase, salt) + val account = AccountKdf.accountKeySeed(passphrase, salt) + assertFalse(auth.contentEquals(identity)) + assertFalse(auth.contentEquals(account)) + assertFalse(identity.contentEquals(account)) + } + + // ---- The wiping itself ---------------------------------------------- + + @Test + fun `wipe zeroes the array it is given`() { + val secret = ByteArray(32) { 0x41 } + SecureBytes.wipe(secret) + assertTrue(secret.all { it == 0.toByte() }) + } + + @Test + fun `wiping nothing is not an error`() { + SecureBytes.wipe(null) + SecureBytes.wipe(ByteArray(0)) + } + + @Test + fun `withSecret wipes on the way out`() { + val secret = ByteArray(32) { 0x41 } + val seen = SecureBytes.withSecret(secret) { it.copyOf() } + assertTrue(seen.all { it == 0x41.toByte() }, "the body saw a wiped buffer") + assertTrue(secret.all { it == 0.toByte() }, "the buffer was not wiped") + } + + /** + * 🔴 **The case the `finally` exists for.** A derivation that failed + * half-way is exactly where the material gets forgotten, so the wipe has to + * survive a throw — not sit at the end of a happy path. + */ + @Test + fun `withSecret wipes when the body throws`() { + val secret = ByteArray(32) { 0x41 } + assertFailsWith { + SecureBytes.withSecret(secret) { error("derivation failed half-way") } + } + assertTrue(secret.all { it == 0.toByte() }, "a throw left the material behind") + } + + /** + * ⚠️ **The honest limit, asserted rather than left to the prose.** A copy + * taken before the wipe is untouched by it. This is not a bug to fix — a + * `ByteArray` cannot reach copies of itself — and stating it as a passing + * test stops anybody reading `wipe` as stronger than it is. + */ + @Test + fun `a copy taken beforehand survives the wipe`() { + val secret = ByteArray(32) { 0x41 } + val copy = SecureBytes.copyOf(secret) + SecureBytes.wipe(secret) + assertTrue(secret.all { it == 0.toByte() }) + assertTrue(copy.all { it == 0x41.toByte() }, + "the copy was wiped too, which would mean these share a buffer") + } + + @Test + fun `withSecret returns what the body returned`() { + assertEquals(7, SecureBytes.withSecret(ByteArray(4)) { 7 }) + } +}