null
/
Privacy-Period-Tracker
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix: make the lock fail towards opening, not towards a locked-out app 

Three ways the lock could fail badly, all in its storage.

hasPin had no catch, unlike the equivalent flow 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 why.

It now reads as no lock. That is a decision, not a shrug: 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. It is the rule VerifierRecord.decode already
applies to a single corrupt record, extended to the file.

The store also gains a corruption handler. Without one, a single bad write
leaves DataStore unable to read OR write it: the lock can never be set
again, and the "Forgot your PIN?" erase fails too, since it writes here.

And a wrong PIN now costs time even when the counter cannot be signed.
Signing needs the Keystore, which can be briefly unavailable; the write
returned early, so guessing was free for as long as that lasted -- the one
direction this must not fail in. The unwritten counter is held in memory
and read back whenever it is the longer wait. Not persisted: it is the
delay actually earned rather than the maximum a tampered counter earns,
and it is forgotten on process death, which is the bound a reboot already
gives an attacker.

Writing that test found the honest boundary. If the key is gone entirely
the PIN cannot be checked either, so the app 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 while the verifier still
works. The fake now tells those apart by the domain-separation tag the two
uses already carry.

Proved with prove-guard, one red each: dropping the in-memory counter, and
removing the catch.

closes #64

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
null 2026-08-20 21:53:51 -05:00
parent a12d8e5c49
commit 7f67573e8a
5 changed files with 156 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
app/src/main/kotlin/dev/privacyllc/period/di/DataModule.kt
View File

@ -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<Preferences> =
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")
}

61
core/security/src/main/kotlin/dev/privacyllc/period/core/security/AppLockRepository.kt
View File

@ -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<Boolean> = 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<Boolean> = 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()

51
core/security/src/test/kotlin/dev/privacyllc/period/core/security/AppLockRepositoryTest.kt
View File

@ -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))

16
core/security/src/test/kotlin/dev/privacyllc/period/core/security/FakeMacProvider.kt
View File

@ -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")) }

19
docs/security/SECURITY.md
View File

@ -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