diff --git a/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt b/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt index a09cb0b..ac00b39 100644 --- a/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt +++ b/app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt @@ -3,7 +3,9 @@ package dev.privacyllc.period.di import android.content.Context import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.PreferenceDataStoreFactory +import androidx.datastore.core.handlers.ReplaceFileCorruptionHandler import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.emptyPreferences import androidx.datastore.preferences.preferencesDataStoreFile import dagger.Module import dagger.Provides @@ -72,7 +74,18 @@ object DataModule { @Singleton @AppLockStore fun appLockDataStore(@ApplicationContext context: Context): DataStore = - PreferenceDataStoreFactory.create { + PreferenceDataStoreFactory.create( + // A corrupt lock file is replaced rather than thrown at every + // caller. Without this, one bad write leaves the app unable to read + // OR write the file: the lock can never be set again, and the + // "Forgot your PIN?" erase — the only way back in — fails too, + // because it writes to this same store. + // + // Empty means "no lock", which is deliberate and matches how + // AppLockRepository reads an unreadable file. The reasoning is + // there, and in docs/security/SECURITY.md. + corruptionHandler = ReplaceFileCorruptionHandler { emptyPreferences() }, + ) { context.preferencesDataStoreFile("app_lock") } diff --git a/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt b/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt index 58b3a4d..b1702a5 100644 --- a/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt +++ b/core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt @@ -4,10 +4,13 @@ import android.os.SystemClock import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.emptyPreferences import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.catch import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map +import java.io.IOException import java.security.MessageDigest import java.util.Base64 @@ -92,8 +95,28 @@ class AppLockRepository internal constructor( private val verifier: PinVerifier = PinVerifier(macs) - /** True once a PIN exists. Derived, never stored. */ - val hasPin: Flow = store.data.map { it[Keys.Verifier] != null } + /** + * True once a PIN exists. Derived, never stored. + * + * An unreadable file reads as **no lock**, and that is a decision rather + * than an oversight. + * + * This flow had no `.catch` at all, unlike the one in + * `UserPreferencesRepository`, and it is collected on the startup path in + * two places that cannot handle a throw — the gate's `stateIn`, which + * launches outside its own exception handler, and `MainActivity`'s + * `FLAG_SECURE` collector. A corrupt `app_lock` file crashed the app before + * any UI existed to say so. + * + * Opening is the safe direction, for the same reason `VerifierRecord.decode` + * already treats a corrupt record as "no PIN set": whoever can corrupt this + * file has the app's private storage, and so can read the database the lock + * does not encrypt anyway. The alternative is an app that cannot be opened + * *or* erased, because the way out is behind the lock that is broken. + */ + val hasPin: Flow = store.data + .catch { failure -> if (failure is IOException) emit(emptyPreferences()) else throw failure } + .map { it[Keys.Verifier] != null } suspend fun hasPinNow(): Boolean = hasPin.first() @@ -184,19 +207,47 @@ class AppLockRepository internal constructor( private fun readLockout(prefs: Preferences): LockoutState? { val blob = prefs[Keys.Lockout] val stored = prefs[Keys.LockoutMac] - if (blob == null && stored == null) return LockoutState() + if (blob == null && stored == null) return LockoutState().longerOf(unsavedLockout) if (blob == null || stored == null) return null val expected = runCatching { macOf(blob) }.getOrNull() ?: return null // A tampered or unverifiable counter reads as null, and null is maximum // backoff in LockoutPolicy — so editing the file is the worst available // move rather than the best one. if (!MessageDigest.isEqual(expected, stored.decodeB64())) return null - return LockoutState.decode(blob) + return LockoutState.decode(blob).longerOf(unsavedLockout) } + /** Whichever of the two has seen more failures — see [unsavedLockout]. */ + private fun LockoutState?.longerOf(other: LockoutState?): LockoutState? = when { + this == null || other == null -> this ?: other + other.failedAttempts > failedAttempts -> other + else -> this + } + + /** + * The failed attempt this process could not sign. + * + * Signing needs the Keystore, and the Keystore can be unavailable for a + * moment. When it is, the counter cannot be written — and the old code + * simply returned, so the attempt cost nothing and wrong PINs became free + * for as long as the condition lasted. That is the one direction this must + * not fail in. + * + * Held in memory instead, and read back by [readLockout] whenever it is the + * longer wait. Deliberately not persisted: it is the earned delay, not the + * maximum a tampered counter earns, and it is forgotten when the process + * dies — which is the same bound an attacker already has on a reboot. + */ + @Volatile + private var unsavedLockout: LockoutState? = null + private suspend fun writeLockout(state: LockoutState) { val blob = state.encode() - val mac = runCatching { macOf(blob) }.getOrNull() ?: return + val mac = runCatching { macOf(blob) }.getOrNull() ?: run { + unsavedLockout = state + return + } + unsavedLockout = null store.edit { it[Keys.Lockout] = blob it[Keys.LockoutMac] = mac.encodeB64() diff --git a/core/security/src/test/kotlin/dev/privacyllc/period/core/security/AppLockRepositoryTest.kt b/core/security/src/test/kotlin/dev/privacyllc/period/core/security/AppLockRepositoryTest.kt index a324eda..1cd9473 100644 --- a/core/security/src/test/kotlin/dev/privacyllc/period/core/security/AppLockRepositoryTest.kt +++ b/core/security/src/test/kotlin/dev/privacyllc/period/core/security/AppLockRepositoryTest.kt @@ -54,6 +54,57 @@ class AppLockRepositoryTest { @After fun tearDown() = scope.cancel() + // ----------------------------------------------------------------------- + // Failing safely + // ----------------------------------------------------------------------- + + @Test fun `an unreadable lock file reads as no lock rather than crashing`() = scope.runTest { + // Written as bytes no preferences parser will accept. The flow used to + // have no catch at all, and it is collected on the startup path in two + // places that cannot handle a throw — so this crashed the app before any + // UI existed to say why. + val broken = tmp.newFile("broken.preferences_pb").apply { writeBytes(byteArrayOf(1, 2, 3, 4, 5)) } + val brokenStore = PreferenceDataStoreFactory.create( + scope = CoroutineScope(scope.coroutineContext), + produceFile = { broken }, + ) + val brokenRepo = AppLockRepository(brokenStore, FakeMacProvider(), FakeClocks()) + + // Opening is the safe direction: the alternative is an app that can + // neither be opened nor erased, because the way out is behind the lock. + assertFalse(brokenRepo.hasPin.first()) + } + + @Test fun `a failed attempt still costs time when its counter cannot be signed`() = scope.runTest { + repo.setPin(cheapPin) + assertTrue(repo.check(cheapPin) is UnlockResult.Unlocked) + + // The counter stops signing while the verifier still works — a wrong PIN + // is still recognisably wrong, but the failure cannot be written down. + macs.backoffSigningFails = true + + // The first few are free by design — how many is LockoutPolicy's + // business, and tested there. + repeat(LockoutPolicy.FREE_ATTEMPTS + 1) { + assertTrue(repo.check("1111".toCharArray()) is UnlockResult.Wrong) + } + + // The fifth is not. Before this, the write returned early and every + // attempt cost nothing, so guessing was free for as long as the + // condition lasted — which is the one direction this must not fail in. + assertTrue( + "wrong PINs stayed free while the counter could not be signed", + repo.check("1111".toCharArray()) is UnlockResult.TooSoon, + ) + assertTrue(repo.lockoutRemainingMillis() > 0) + } + + @Test fun `a fresh install is not in a lockout it never earned`() = scope.runTest { + // The in-memory counter must not invent one, which is the failure mode + // of remembering it at all. + assertEquals(0L, repo.lockoutRemainingMillis()) + } + @Test fun `a fresh install has no PIN`() = scope.runTest { assertFalse(repo.hasPin.first()) assertEquals(UnlockResult.NoPin, repo.check(cheapPin)) diff --git a/core/security/src/test/kotlin/dev/privacyllc/period/core/security/FakeMacProvider.kt b/core/security/src/test/kotlin/dev/privacyllc/period/core/security/FakeMacProvider.kt index 818f36f..7c19515 100644 --- a/core/security/src/test/kotlin/dev/privacyllc/period/core/security/FakeMacProvider.kt +++ b/core/security/src/test/kotlin/dev/privacyllc/period/core/security/FakeMacProvider.kt @@ -23,6 +23,19 @@ internal class FakeMacProvider( var deleteCalls = 0 private set + /** + * Refuse to sign the lockout counter, while still verifying PINs. + * + * A narrow window on purpose, and the only one that matters: if the key is + * gone entirely the PIN cannot be checked either, and the app honestly + * reports that it does not know rather than charging for a guess it could + * not read. The failure worth defending against is the counter alone + * failing to sign — which used to return early and make wrong PINs free. + * + * Told apart by the domain-separation tag the two uses already carry. + */ + var backoffSigningFails = false + override fun hasKey(): Boolean = key != null override fun ensureKey() { @@ -31,6 +44,9 @@ internal class FakeMacProvider( } override fun mac(data: ByteArray): ByteArray { + if (backoffSigningFails && data.firstOrNull() == PinVerifier.TAG_BACKOFF) { + throw IllegalStateException("the signing key is unavailable") + } val k = key ?: throw IllegalStateException("the verifier key is absent") return Mac.getInstance("HmacSHA256") .apply { init(SecretKeySpec(k, "HmacSHA256")) } diff --git a/docs/security/SECURITY.md b/docs/security/SECURITY.md index 9e41ab6..5c662a7 100644 --- a/docs/security/SECURITY.md +++ b/docs/security/SECURITY.md @@ -83,6 +83,25 @@ Data Safety section, and never lets health data reach any of them. a partner, or a child, or a pocket, destroy a history permanently while knowing nothing. Both clock bypasses are closed — the wait is the longer of a wall-clock and a monotonic deadline, and a reboot re-applies it in full. +- **The lock fails towards opening, never towards a lock nobody can open.** An + unreadable or corrupt `app_lock` file reads as *no lock*, and a corrupt one is + replaced rather than thrown at every later write. The reasoning is the same + one `VerifierRecord.decode` already applies to a single corrupt record: whoever + can corrupt that file has the app's private storage, and therefore reads the + database this lock does not encrypt anyway — while the alternative is an app + that can be neither opened nor erased, because the way out is behind the lock + that is broken. Before this, the flow had no `catch` at all and was collected + on the startup path in two places that could not handle a throw, so a corrupt + file crashed the app before any UI existed to say why. +- **A wrong PIN costs time even when the counter cannot be written.** Signing the + backoff counter needs the Keystore, and the Keystore can be unavailable for a + moment; the write used to return early, which made guessing free for as long as + that lasted. The unwritten counter is now held in memory and read back whenever + it is the longer wait. Deliberately not persisted: it is the delay actually + earned, not the maximum a tampered counter earns, and it is forgotten on + process death — the same bound an attacker already gets from a reboot. If the + key is gone entirely the PIN cannot be checked either, and the app says it does + not know rather than charging for a guess it could not read. - **A forgotten PIN is not recoverable, and that is a decision** (tracker #34). The only route past the lock screen erases everything and grants access to nothing. It also clears the Keystore key, without which a user would have