Zero key material after use (ios#39, Android half)
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 <noreply@anthropic.com>
This commit is contained in:
Paul Hitt
co-authored by
Claude Opus 5
Claude Opus 5
parent
f362db0ee7
commit
4a587a7d4d
@@ -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
|
||||
|
||||
@@ -133,12 +133,17 @@ object AccountKdf {
|
||||
context: String,
|
||||
length: Int = 32,
|
||||
params: Params = Params.PBKDF2_V1,
|
||||
): ByteArray = Hkdf.derive(
|
||||
ikm = master(passphrase, salt, params),
|
||||
): 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<ByteArray, ByteArray> {
|
||||
val ikm = master(passphrase, salt, params)
|
||||
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,
|
||||
)
|
||||
return expand(Context.ACCOUNT_AUTH) to expand(Context.ACCOUNT_IDENTITY)
|
||||
// ⚠️ 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)
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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 <T> 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()
|
||||
}
|
||||
@@ -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<IllegalStateException> {
|
||||
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 })
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user