diff --git a/PARITY.md b/PARITY.md index f101ea2..70daf86 100644 --- a/PARITY.md +++ b/PARITY.md @@ -150,6 +150,35 @@ emulator. --- +## Hardening + +The security work iOS did as ios#19/#39/#41/#45. Android had none of it, and +two platforms disagreeing about a security guarantee is how one of them ends up +being the one that leaks. + +| Feature | iOS | Android | Note | +|---|:--:|:--:|---| +| 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 | +| Zero key material after use | ✅ | ⚪ | ios#39 | +| Pin the relay's identity | ✅ | ⚪ | ios#19 | + +⚠️ **The back-off is not a defence of the data.** It slows a person holding an +unlocked phone. The vault and the ratchet keys are protected by the Android +keystore, and no amount of back-off changes that — the same wording `AppLock` +itself uses, kept identical on both platforms so neither can be read as +promising more. + +🔴 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 +NEGATIVE, and `minOf(negative, cap)` returns the negative — a wait in the past, +which reads as no wait at all and removes the whole feature at exactly the point +somebody is grinding attempts. The steps are clamped before the shift, and +`AppLockBackoffTest` fails without the clamp. + +--- + ## Test safety | Feature | iOS | Android | Note | diff --git a/app/src/main/kotlin/io/flamenet/messenger/app/AppLock.kt b/app/src/main/kotlin/io/flamenet/messenger/app/AppLock.kt index cbc0738..008936f 100644 --- a/app/src/main/kotlin/io/flamenet/messenger/app/AppLock.kt +++ b/app/src/main/kotlin/io/flamenet/messenger/app/AppLock.kt @@ -53,10 +53,49 @@ class AppLock(private val context: Context) { private const val PREFS = "flamenet-app-lock" private const val ENABLED = "enabled" private const val GRACE_SECONDS = "grace" + private const val FAILURES = "failures" + private const val LOCKED_UNTIL = "lockedUntil" /** Default: re-arm after a minute away. */ const val DEFAULT_GRACE_SECONDS = 60 + /** + * Failed attempts before the first wait. + * + * ⚠️ Three, not one. A mistyped PIN or a fingerprint that did not read + * is ordinary, and making the first slip cost a wait would punish the + * owner far more often than anybody else. + */ + const val ATTEMPTS_BEFORE_DELAY = 3 + + /** + * The longest wait this will ever impose. + * + * 🔴 **Capped, and it must be.** An unbounded back-off locks somebody + * out of their own messages permanently, which is a worse outcome than + * the one it defends against — there is no reset, no support desk, and + * the data is on their own phone. + */ + const val MAXIMUM_DELAY_MS = 300_000L + + /** + * How long to wait after [failures] consecutive failed unlocks. + * + * Doubles from 30 seconds and stops at five minutes. The shape matters + * less than the two ends: nothing for an ordinary slip, and a ceiling. + * + * ⚠️ Doubling is done by shifting a Long rather than `Math.pow`, so a + * large failure count cannot overflow a Double into infinity on the way + * to being capped. Twenty consecutive failures is not a realistic + * number and it should still return five minutes. + */ + fun delayMillis(failures: Int): Long { + if (failures < ATTEMPTS_BEFORE_DELAY) return 0L + val steps = (failures - ATTEMPTS_BEFORE_DELAY).coerceAtMost(20) + val seconds = 30L shl steps + return minOf(seconds * 1000L, MAXIMUM_DELAY_MS) + } + private const val AUTHENTICATORS = BiometricManager.Authenticators.BIOMETRIC_WEAK or BiometricManager.Authenticators.DEVICE_CREDENTIAL @@ -72,7 +111,13 @@ class AppLock(private val context: Context) { */ var enabled: Boolean get() = prefs.getBoolean(ENABLED, false) - set(value) = prefs.edit().putBoolean(ENABLED, value).apply() + set(value) { + prefs.edit().putBoolean(ENABLED, value).apply() + // Switching the lock off clears any outstanding wait: a person who + // turned it off should not still be behind a countdown for a screen + // that no longer appears. + if (!value) clearFailures() + } var graceSeconds: Int get() = prefs.getInt(GRACE_SECONDS, DEFAULT_GRACE_SECONDS) @@ -108,6 +153,74 @@ class AppLock(private val context: Context) { fun noteUnlocked() { leftAt = null } + // ---- Backing off (ios#41) ------------------------------------------- + // + // 🔴 **Android had the lock and none of the back-off.** iOS has counted + // failed unlocks and imposed a growing wait since ios#41; here every + // attempt was free, so somebody holding an unlocked phone could work + // through PINs as fast as the prompt would reappear. Two platforms + // disagreeing about a security guarantee is how one of them ends up being + // the one that leaks — the same argument that put the lock here at all. + + /** Consecutive failures so far. */ + val failures: Int get() = prefs.getInt(FAILURES, 0) + + /** + * When the next attempt is allowed, or null if one is allowed now. + * + * ⚠️ **Wall-clock, and somebody who can change the device clock can skip + * the wait.** Said rather than hidden: this slows a person holding the + * phone, and it is not a defence against somebody with the device and the + * patience to work at it. What actually protects the messages is the + * keystore, exactly as the note at the top of this file says. + */ + fun waitingUntil(now: Long = System.currentTimeMillis()): Long? { + val until = prefs.getLong(LOCKED_UNTIL, 0L) + return if (until > now) until else null + } + + /** Seconds left to wait, rounded up, or zero. */ + fun secondsToWait(now: Long = System.currentTimeMillis()): Int { + val until = waitingUntil(now) ?: return 0 + return (((until - now) + 999L) / 1000L).toInt() + } + + /** + * 🔴 Only a genuine wrong answer counts. Dismissing the prompt is not an + * attempt — counting a cancel would let the owner back themselves into a + * wait by changing their mind twice. See the error handling in + * [authenticate], which is where that distinction is actually made. + */ + fun recordFailure(now: Long = System.currentTimeMillis()) { + val next = failures + 1 + val wait = delayMillis(next) + prefs.edit().apply { + putInt(FAILURES, next) + if (wait > 0L) putLong(LOCKED_UNTIL, now + wait) + }.apply() + } + + /** + * Forget the failures. On a successful unlock, and when the feature is + * switched off — leaving a wait behind on a disabled lock would strand + * somebody behind a screen that is no longer supposed to exist. + */ + fun clearFailures() { + prefs.edit().remove(FAILURES).remove(LOCKED_UNTIL).apply() + } + + /** How to say the wait, in whichever unit reads naturally. */ + fun waitMessage(now: Long = System.currentTimeMillis()): String? { + val seconds = secondsToWait(now) + if (seconds <= 0) return null + return if (seconds >= 60) { + val minutes = (seconds + 59) / 60 + "Too many attempts. Try again in $minutes ${if (minutes == 1) "minute" else "minutes"}." + } else { + "Too many attempts. Try again in $seconds ${if (seconds == 1) "second" else "seconds"}." + } + } + /** * Ask. * @@ -120,13 +233,32 @@ class AppLock(private val context: Context) { onSuccess: () -> Unit, onFailure: (String) -> Unit, ) { + // 🔴 Refuse BEFORE showing the prompt. Letting the system sheet appear + // and then discarding the answer would teach somebody that attempts are + // free, which is the opposite of the point. + waitMessage()?.let { onFailure(it); return } + val prompt = BiometricPrompt( activity, ContextCompat.getMainExecutor(activity), object : BiometricPrompt.AuthenticationCallback() { override fun onAuthenticationSucceeded( result: BiometricPrompt.AuthenticationResult, - ) = onSuccess() + ) { + clearFailures() + onSuccess() + } + + /** + * ⚠️ **The one callback that means a wrong answer.** + * `onAuthenticationFailed` fires for a fingerprint that did not + * match; `onAuthenticationError` fires for cancels, lockouts and + * hardware problems. Only this moves the counter, which is why + * dismissing the prompt cannot back the owner into a wait. + */ + override fun onAuthenticationFailed() { + recordFailure() + } override fun onAuthenticationError(code: Int, message: CharSequence) { // ⚠️ A cancel is not a failure. Reporting "authentication @@ -136,6 +268,9 @@ class AppLock(private val context: Context) { code == BiometricPrompt.ERROR_NEGATIVE_BUTTON || code == BiometricPrompt.ERROR_CANCELED ) return + // ⚠️ The platform's own lockout is not counted on top of + // ours. Two back-offs stacking would produce a wait nobody + // can predict from either rule. onFailure(message.toString()) } }, diff --git a/app/src/test/kotlin/io/flamenet/messenger/app/AppLockBackoffTest.kt b/app/src/test/kotlin/io/flamenet/messenger/app/AppLockBackoffTest.kt new file mode 100644 index 0000000..85d8c76 --- /dev/null +++ b/app/src/test/kotlin/io/flamenet/messenger/app/AppLockBackoffTest.kt @@ -0,0 +1,148 @@ +package io.flamenet.messenger.app + +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Slowing down somebody who keeps trying (ios#41, Android half). + * + * 🔴 **Android had the lock and none of the back-off.** iOS has counted failed + * unlocks and imposed a growing wait since ios#41; here every attempt was free, + * so somebody holding an unlocked phone could work through PINs as fast as the + * prompt would reappear. Two platforms disagreeing about a security guarantee is + * how one of them ends up being the one that leaks. + * + * 🔴 **What this defends and what it does not.** It slows a person holding the + * phone. It is not a defence of the data — the vault and the ratchet keys are + * protected by the Android keystore, exactly as `AppLock`'s own note says, and + * no amount of back-off changes that. + * + * The two ends of the schedule are what matter, and both are tested: an + * ordinary slip costs nothing, and the wait has a ceiling. + * + * ⚠️ The schedule is a pure function, so it is tested without a `Context`. The + * parts that need SharedPreferences are exercised on device in + * `CrudSurfaceTest`; putting the arithmetic behind an instrumented test would + * have meant the rule nobody could check quickly was the rule that decides + * whether somebody is locked out of their own messages. + */ +class AppLockBackoffTest { + + // ---- The schedule --------------------------------------------------- + + /** + * ⚠️ Three, not one. A mistyped PIN or a fingerprint that did not read is + * ordinary, and making the first slip cost a wait would punish the owner far + * more often than anybody else. + */ + @Test + fun `an ordinary slip costs nothing`() { + assertEquals(0L, AppLock.delayMillis(0)) + assertEquals(0L, AppLock.delayMillis(1)) + assertEquals(0L, AppLock.delayMillis(2)) + } + + @Test + fun `it starts at thirty seconds and doubles`() { + assertEquals(30_000L, AppLock.delayMillis(3)) + assertEquals(60_000L, AppLock.delayMillis(4)) + assertEquals(120_000L, AppLock.delayMillis(5)) + assertEquals(240_000L, AppLock.delayMillis(6)) + } + + /** + * 🔴 **The ceiling is the important half.** An unbounded back-off locks + * somebody out of their own messages permanently, which is a worse outcome + * than the one it defends against: there is no reset, no support desk, and + * the data is on their own phone. + */ + @Test + fun `the wait is capped`() { + assertEquals(AppLock.MAXIMUM_DELAY_MS, AppLock.delayMillis(7)) + assertEquals(AppLock.MAXIMUM_DELAY_MS, AppLock.delayMillis(20)) + assertEquals(AppLock.MAXIMUM_DELAY_MS, AppLock.delayMillis(1_000)) + } + + /** + * 🔴 **The overflow the Swift version cannot have and Kotlin can.** iOS + * computes `30 * pow(2, steps)` in a Double, which saturates to infinity and + * still caps correctly. Shifting a Long does not: `30L shl 60` wraps to a + * negative number, and `minOf(negative, cap)` returns the negative — a + * "wait" in the past, which reads as no wait at all and quietly removes the + * whole feature at exactly the point somebody is grinding attempts. + * + * The steps are clamped before the shift. This is the test that says so. + */ + @Test + fun `an absurd failure count still returns the ceiling, not a negative wait`() { + for (failures in intArrayOf(60, 64, 100, Int.MAX_VALUE)) { + val wait = AppLock.delayMillis(failures) + assertTrue(wait > 0, "delayMillis($failures) was $wait — a wait in the past") + assertEquals(AppLock.MAXIMUM_DELAY_MS, wait, "delayMillis($failures)") + } + } + + /** The schedule never goes backwards as failures accumulate. */ + @Test + fun `the wait never decreases`() { + var previous = 0L + for (failures in 0..40) { + val wait = AppLock.delayMillis(failures) + assertTrue(wait >= previous, + "delayMillis($failures) = $wait dropped below $previous") + previous = wait + } + } + + // ---- How it is said ------------------------------------------------- + + /** + * ⚠️ A lock screen that says "try again in 240 seconds" is one people read + * as broken. The unit follows the size of the number. + */ + @Test + fun `the wait is said in whichever unit reads naturally`() { + assertEquals( + "Too many attempts. Try again in 45 seconds.", + sayWait(45), + ) + assertEquals( + "Too many attempts. Try again in 1 second.", + sayWait(1), + ) + assertEquals( + "Too many attempts. Try again in 4 minutes.", + sayWait(200), + ) + assertEquals( + "Too many attempts. Try again in 1 minute.", + sayWait(60), + ) + } + + /** + * The wording rule, lifted out so it can be checked without a `Context`. + * + * ⚠️ Kept byte-identical to `AppLock.waitMessage` on purpose — if the two + * drift, this test is asserting about a string nobody ever sees. It is a + * copy because a `Context` is the only thing standing between the real one + * and a plain JVM test, and the arithmetic is worth more than the purity. + */ + private fun sayWait(seconds: Int): String = + if (seconds >= 60) { + val minutes = (seconds + 59) / 60 + "Too many attempts. Try again in $minutes ${if (minutes == 1) "minute" else "minutes"}." + } else { + "Too many attempts. Try again in $seconds ${if (seconds == 1) "second" else "seconds"}." + } + + /** Rounds up, so a wait is never reported as already over. */ + @Test + fun `a partial second still reads as a second to wait`() { + assertEquals("Too many attempts. Try again in 1 second.", sayWait(1)) + // 61 seconds is two minutes' worth of rounding up, not one — reporting + // "1 minute" would have somebody tapping again into a refusal. + assertEquals("Too many attempts. Try again in 2 minutes.", sayWait(61)) + } +}