Merge branch 'feat/app-lock-backoff' into 'main'
Back off on repeated app-lock attempts (ios#41, Android half) See merge request paulhitt/flamenet-messenger-android!37
This commit was merged in pull request #64.
This commit is contained in:
@@ -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 |
|
||||
|
||||
@@ -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())
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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))
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user