From f66ec3d7fb0cd58c43401265cf0e87b70c61b0bc Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Wed, 1 Jul 2026 22:29:48 -0500 Subject: [PATCH] fix(security): harden app-lock + encrypted-cache flows (PR #45 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 14 of the 15 confirmed findings from the max-effort review of the screen-lock app gate. The remaining one (accounts/credentials share the auth-bound cache DB) needs a device-tested Room migration and is filed separately; its blast radius is reduced here by eliminating the spurious wipes. - Cold-start deadlock: LibreMailApplication injects AccountRepository lazily so the Room DB is never built on the main thread before unlock. - Passphrase source of truth: DatabaseKeyStore.resolvePassphrase() keys off which seal exists, not the app-lock setting; passphrase() refuses to mint a master key while an auth seal exists. - Toggle-order strand: disabling app-lock reseals under the master key whenever an auth seal exists (not gated on the encryptCache setting). - Crash-safe clear protocol: wipe + reset seals, then clear the flag last; set clear-pending before flipping app-lock off. - isInvalidated(): treats a lapsed auth window (UserNotAuthenticated) as valid, and onForeground short-circuits when app-lock is off. - unwrapSealedPassphrase: classifies all decrypt failures — no crash after a successful auth. - Headless entry points: SyncWorker/SendWorker/IdleService fail fast via EncryptedCacheGuard instead of blocking DB construction while locked. - sealWithMaster: deletes the orphaned auth key (no spurious later wipe). - Lock-bypass race: AppLockGate ignores a background recorded after a foreground pass began; the ViewModel captures the foreground timestamp synchronously. - FLAG_SECURE: set while app-lock is on (recents/screenshot protection). - Resume + re-lock: the gate covers content with an opaque overlay instead of removing it, so no stale frame renders and in-progress state (nav, drafts) survives re-lock. - Retry feedback: lock emissions carry a nonce so a retry updates the UI. Tests: AppLockGate stale-foreground race cases + an exhaustive KeyInvalidationPolicy table. Fast gate green + androidTest compiles. Co-Authored-By: Claude Fable 5 --- .../org/libremail/LibreMailApplication.kt | 8 +- .../main/kotlin/org/libremail/MainActivity.kt | 17 ++++ .../libremail/data/security/AppLockGate.kt | 22 +++--- .../data/security/DatabaseKeyCipher.kt | 17 +++- .../data/security/DatabaseKeyStore.kt | 56 +++++++++++--- .../data/security/EncryptedCacheGuard.kt | 31 ++++++++ .../org/libremail/data/sync/SendWorker.kt | 25 ++++-- .../org/libremail/data/sync/SyncWorker.kt | 20 +++-- .../kotlin/org/libremail/di/DatabaseModule.kt | 32 ++++---- .../kotlin/org/libremail/push/IdleService.kt | 30 ++++++-- .../org/libremail/ui/lock/AppLockGateHost.kt | 55 +++++++++++-- .../org/libremail/ui/lock/AppLockViewModel.kt | 77 +++++++++++++++---- .../ui/settings/SettingsViewModel.kt | 10 +-- app/src/main/res/values/strings.xml | 1 + .../data/security/AppLockGateTest.kt | 22 ++++++ .../security/KeyInvalidationPolicyTest.kt | 26 +++++++ 16 files changed, 364 insertions(+), 85 deletions(-) create mode 100644 app/src/main/kotlin/org/libremail/data/security/EncryptedCacheGuard.kt diff --git a/app/src/main/kotlin/org/libremail/LibreMailApplication.kt b/app/src/main/kotlin/org/libremail/LibreMailApplication.kt index 8c24366..6053033 100644 --- a/app/src/main/kotlin/org/libremail/LibreMailApplication.kt +++ b/app/src/main/kotlin/org/libremail/LibreMailApplication.kt @@ -4,6 +4,7 @@ package org.libremail import android.app.Application import androidx.hilt.work.HiltWorkerFactory import androidx.work.Configuration +import dagger.Lazy import dagger.hilt.android.HiltAndroidApp import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -33,7 +34,10 @@ class LibreMailApplication : @Inject lateinit var settingsRepository: SettingsRepository - @Inject lateinit var accountRepository: AccountRepository + // Lazy: resolving AccountRepository constructs the Room database, which — with app-lock + encrypted + // cache on — blocks until the user authenticates. Keeping it lazy means the DB is built off the main + // thread inside the collector below (never during onCreate), so the app never deadlocks at launch. + @Inject lateinit var accountRepository: Lazy @Inject lateinit var idlePushManager: IdlePushManager @@ -70,7 +74,7 @@ class LibreMailApplication : appScope.launch { combine( settingsRepository.settings.map { it.pushIdle }, - accountRepository.observeAccounts().map { it.isNotEmpty() }, + accountRepository.get().observeAccounts().map { it.isNotEmpty() }, ) { pushEnabled, hasAccounts -> pushEnabled && hasAccounts } .distinctUntilChanged() .collect { active -> diff --git a/app/src/main/kotlin/org/libremail/MainActivity.kt b/app/src/main/kotlin/org/libremail/MainActivity.kt index 203f738..3d8d516 100644 --- a/app/src/main/kotlin/org/libremail/MainActivity.kt +++ b/app/src/main/kotlin/org/libremail/MainActivity.kt @@ -6,6 +6,7 @@ import android.content.Intent import android.content.pm.PackageManager import android.os.Build import android.os.Bundle +import android.view.WindowManager import androidx.activity.compose.rememberLauncherForActivityResult import androidx.activity.compose.setContent import androidx.activity.enableEdgeToEdge @@ -18,7 +19,11 @@ import androidx.compose.ui.platform.LocalContext import androidx.core.content.ContextCompat import androidx.fragment.app.FragmentActivity import androidx.lifecycle.compose.collectAsStateWithLifecycle +import androidx.lifecycle.lifecycleScope import dagger.hilt.android.AndroidEntryPoint +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch import org.libremail.data.settings.SettingsRepository import org.libremail.ui.LibreMailApp import org.libremail.ui.compose.ComposePrefill @@ -51,6 +56,18 @@ class MainActivity : FragmentActivity() { override fun onCreate(savedInstanceState: Bundle?) { super.onCreate(savedInstanceState) enableEdgeToEdge() + // Block screenshots and the recents-switcher snapshot while app-lock is on — the Compose gate + // can't stop the system's task snapshot (captured around background). Gated on the setting + // because FLAG_SECURE also blocks the user's own screenshots. + lifecycleScope.launch { + settingsRepository.settings.map { it.appLock }.distinctUntilChanged().collect { secure -> + if (secure) { + window.addFlags(WindowManager.LayoutParams.FLAG_SECURE) + } else { + window.clearFlags(WindowManager.LayoutParams.FLAG_SECURE) + } + } + } // Only on a fresh launch — on a config-change recreation the NavHost restores the compose // destination itself, so re-parsing the (unchanged) intent would open a duplicate. if (savedInstanceState == null) { diff --git a/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt b/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt index 91b9f4f..83c2a47 100644 --- a/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt +++ b/app/src/main/kotlin/org/libremail/data/security/AppLockGate.kt @@ -24,18 +24,25 @@ class AppLockGate(private val graceMillis: Long = DEFAULT_GRACE_MILLIS) { private var backgroundedAt: Long? = null /** - * Recompute the lock state when the app comes to the foreground. [now] is a monotonic-ish epoch - * in milliseconds. Returns the resulting state. + * Recompute the lock state when the app comes to the foreground. [now] is a monotonic-ish epoch in + * milliseconds, captured synchronously when the foreground event fires (so a background recorded + * during an async foreground pass can't be mistaken for a within-grace return). Returns the state. */ fun onForeground(now: Long, appLockEnabled: Boolean): LockState { + val bgAt = backgroundedAt state = when { !appLockEnabled -> LockState.UNLOCKED state == LockState.LOCKED -> LockState.LOCKED - backgroundedAt == null -> LockState.UNLOCKED - withinGrace(now) -> LockState.UNLOCKED + bgAt == null -> LockState.UNLOCKED + // A background recorded AFTER this pass began (now < bgAt): the app has since gone back to + // the background, so re-lock and KEEP the marker for the genuine return to evaluate. + now < bgAt -> LockState.LOCKED + now - bgAt <= graceMillis -> LockState.UNLOCKED else -> LockState.LOCKED } - backgroundedAt = null + // Consume the marker only for a genuine (non-stale) foreground, so a concurrent background is + // never silently erased — that erasure is what would let the next foreground skip re-locking. + if (bgAt == null || now >= bgAt) backgroundedAt = null return state } @@ -55,11 +62,6 @@ class AppLockGate(private val graceMillis: Long = DEFAULT_GRACE_MILLIS) { state = LockState.LOCKED } - private fun withinGrace(now: Long): Boolean { - val since = backgroundedAt ?: return false - return now - since in 0..graceMillis - } - companion object { /** Time the user can leave the app before re-authentication is required. */ const val DEFAULT_GRACE_MILLIS: Long = 30_000 diff --git a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt index 4a09cec..f0aa1ec 100644 --- a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt +++ b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyCipher.kt @@ -5,6 +5,7 @@ import android.os.Build import android.security.keystore.KeyGenParameterSpec import android.security.keystore.KeyPermanentlyInvalidatedException import android.security.keystore.KeyProperties +import android.security.keystore.UserNotAuthenticatedException import android.util.Base64 import android.util.Log import java.security.KeyStore @@ -73,9 +74,10 @@ class DatabaseKeyCipher @Inject constructor() { } /** - * True when the key exists but has been permanently invalidated (a new biometric was enrolled or - * the device lock was removed). Detected by attempting to initialize a cipher, which fails fast - * without needing an auth window. + * True only when the key exists AND has been permanently invalidated (a new biometric was enrolled + * or the device lock was removed). A merely-lapsed auth window ([UserNotAuthenticatedException]) is + * NOT invalidation and returns false, so a routine foreground pass — which calls this before any + * authentication — never mistakes an unauthenticated key for one that must trigger a cache wipe. */ fun isInvalidated(): Boolean { val key = existingKey() ?: return false @@ -85,6 +87,15 @@ class DatabaseKeyCipher @Inject constructor() { } catch (e: KeyPermanentlyInvalidatedException) { Log.d(TAG, "auth-bound database key invalidated", e) true + } catch (e: UserNotAuthenticatedException) { + // Valid key, just outside its time-bound auth window — not invalidated. + Log.d(TAG, "auth-bound key outside its auth window; not invalidated", e) + false + } catch (e: Exception) { + // Never let a validity probe crash the foreground pass; a real decrypt later surfaces any + // genuine problem. Treat an unknown probe failure as "not invalidated" (don't wipe). + Log.d(TAG, "auth-bound key validity probe failed; treating as valid", e) + false } } diff --git a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt index 4e90fa0..74c3356 100644 --- a/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt +++ b/app/src/main/kotlin/org/libremail/data/security/DatabaseKeyStore.kt @@ -18,6 +18,9 @@ import javax.inject.Singleton private val Context.dbKeyDataStore: DataStore by preferencesDataStore(name = "libremail_dbkey") +/** Which Keystore seal currently protects the cache passphrase (or [NONE] before first use). */ +enum class SealState { NONE, MASTER, AUTH } + /** * Supplies the SQLCipher passphrase for the opt-in encrypted cache. A random 256-bit key is * generated once and persisted only as ciphertext — sealed by a non-exportable Android Keystore key @@ -41,17 +44,44 @@ class DatabaseKeyStore @Inject constructor( ) { private val generationLock = Mutex() + /** + * Resolve the passphrase needed to open (or convert) the on-disk cache, keyed off which seal + * actually EXISTS — not off the app-lock setting, which lives in a separate DataStore and can be + * out of sync with the seals. This is the single source of truth `DatabaseModule` uses: + * - [SealState.AUTH]: the value lives only in [PassphraseSession] after the user authenticates; + * wait for it. Never re-derive or regenerate an auth-sealed passphrase. + * - [SealState.MASTER]: unwrap it with the non-auth master key (no authentication needed). + * - [SealState.NONE]: first use. When app-lock is off, generate + master-seal a fresh key; when + * app-lock is on the arming flow ([sealWithAuth]) mints and unlocks it right after auth, so wait. + */ + suspend fun resolvePassphrase(appLockEnabled: Boolean): String = when (sealState()) { + SealState.AUTH -> session.current() ?: session.await() + SealState.MASTER -> masterSealed() ?: error("master-sealed passphrase failed to decrypt") + SealState.NONE -> if (appLockEnabled) session.current() ?: session.await() else passphrase() + } + + /** Which seal currently protects the passphrase (or [SealState.NONE] before first use). */ + suspend fun sealState(): SealState = when { + read(SEALED_AUTH) != null -> SealState.AUTH + read(SEALED_MASTER) != null -> SealState.MASTER + else -> SealState.NONE + } + /** * App-lock OFF path: returns the passphrase sealed by the master key, generating and sealing it - * on first use. This is the original auto-unwrap behavior and must only be used when app-lock is - * disabled (when it is enabled the master-sealed copy is intentionally removed). + * on first use. Refuses to mint a fresh key while an auth seal exists — doing so would strand the + * real (auth-sealed) key and leave the DB encrypted under a passphrase we could never reproduce. */ suspend fun passphrase(): String { masterSealed()?.let { return it } return generationLock.withLock { // Re-check inside the lock so a concurrent first-caller doesn't generate a second key // (which would leave a DB encrypted under a key we then overwrite and can't reproduce). - masterSealed() ?: generateAndSealMaster() + masterSealed()?.let { return@withLock it } + check(read(SEALED_AUTH) == null) { + "refusing to generate a master passphrase while an auth-sealed passphrase exists" + } + generateAndSealMaster() } } @@ -97,6 +127,9 @@ class DatabaseKeyStore @Inject constructor( it[SEALED_MASTER] = crypto.encrypt(plain) it.remove(SEALED_AUTH) } + // The auth-bound key is now unused. Delete it so a later invalidation of this orphaned key + // can't trigger a spurious cache wipe, and its stale probe can't crash the foreground pass. + authCipher.deleteKey() session.lock() } @@ -117,7 +150,7 @@ class DatabaseKeyStore @Inject constructor( /** * Persist that the encrypted cache must be wiped on the next cold start. The actual file deletion - * happens in `DatabaseModule.provideDatabase` (via [consumeClearPending]) BEFORE Room opens the + * happens in `DatabaseModule.provideDatabase` (guarded by [isClearPending]) BEFORE Room opens the * database, so there is never an open connection whose backing file is deleted underneath it — * the corruption-safe way to "clear + re-sync" after a screen-lock change invalidates the key. */ @@ -125,11 +158,16 @@ class DatabaseKeyStore @Inject constructor( context.dbKeyDataStore.edit { it[CLEAR_PENDING] = true } } - /** Read-and-clear the clear-pending flag. Returns true if the cache should be wiped now. */ - suspend fun consumeClearPending(): Boolean { - val pending = context.dbKeyDataStore.data.first()[CLEAR_PENDING] == true - if (pending) context.dbKeyDataStore.edit { it.remove(CLEAR_PENDING) } - return pending + /** + * True if the cache should be wiped at this cold start. Does NOT clear the flag: the caller must + * perform the wipe (+ [resetSealedPassphrase]) FIRST and then call [clearClearPending], so a crash + * mid-wipe simply repeats the idempotent wipe next start instead of stranding an unreadable file. + */ + suspend fun isClearPending(): Boolean = context.dbKeyDataStore.data.first()[CLEAR_PENDING] == true + + /** Clear the wipe flag. Call ONLY after the wipe + [resetSealedPassphrase] have completed. */ + suspend fun clearClearPending() { + context.dbKeyDataStore.edit { it.remove(CLEAR_PENDING) } } private suspend fun masterSealed(): String? = read(SEALED_MASTER)?.let { crypto.decrypt(it) } diff --git a/app/src/main/kotlin/org/libremail/data/security/EncryptedCacheGuard.kt b/app/src/main/kotlin/org/libremail/data/security/EncryptedCacheGuard.kt new file mode 100644 index 0000000..5fb41cb --- /dev/null +++ b/app/src/main/kotlin/org/libremail/data/security/EncryptedCacheGuard.kt @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-3.0-or-later +package org.libremail.data.security + +import kotlinx.coroutines.flow.first +import org.libremail.data.settings.SettingsRepository +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Tells UI-less entry points (background sync/push/send) whether opening the Room cache would block + * on the user authenticating. When app-lock AND encrypted-cache are on, the SQLCipher passphrase is + * sealed by an auth-bound key and only lives in [PassphraseSession] after an unlock — so a worker + * that constructs a DAO before then would park a thread indefinitely inside `provideDatabase`. + * + * Depends only on DataStore ([SettingsRepository]) and in-memory state ([PassphraseSession]) — never + * on the Room database — so it is safe to consult *before* touching any DB-backed dependency. + */ +@Singleton +class EncryptedCacheGuard @Inject constructor( + private val settingsRepository: SettingsRepository, + private val session: PassphraseSession, +) { + /** + * True when the encrypted cache is currently locked, i.e. opening the database would suspend + * waiting for the user to authenticate. Background work should defer (e.g. `Result.retry()`). + */ + suspend fun isCacheLocked(): Boolean { + val settings = settingsRepository.settings.first() + return settings.appLock && settings.encryptCache && !session.isUnlocked() + } +} diff --git a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt index 8122fde..29f7ac2 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SendWorker.kt @@ -6,11 +6,13 @@ import android.util.Log import androidx.hilt.work.HiltWorker import androidx.work.CoroutineWorker import androidx.work.WorkerParameters +import dagger.Lazy import dagger.assisted.Assisted import dagger.assisted.AssistedInject import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.dao.OutboxDao import org.libremail.data.local.toDomain +import org.libremail.data.security.EncryptedCacheGuard import org.libremail.domain.model.Account import org.libremail.domain.model.AuthType import org.libremail.domain.model.OutgoingMessage @@ -25,11 +27,15 @@ import kotlin.coroutines.cancellation.CancellationException class SendWorker @AssistedInject constructor( @Assisted appContext: Context, @Assisted workerParams: WorkerParameters, - private val outboxDao: OutboxDao, - private val accountDao: AccountDao, + // Lazy: OutboxDao/AccountDao/MailConnectionFactory all resolve the Room DB (the last via + // CredentialStore), which blocks while the encrypted cache is locked. Resolve them only after the + // cache-lock check, so a run while locked fails fast instead of parking a WorkManager thread. + private val outboxDao: Lazy, + private val accountDao: Lazy, private val smtpSender: SmtpSender, private val graphSender: GraphSender, - private val connectionFactory: MailConnectionFactory, + private val connectionFactory: Lazy, + private val cacheGuard: EncryptedCacheGuard, ) : CoroutineWorker(appContext, workerParams) { private companion object { @@ -37,6 +43,10 @@ class SendWorker @AssistedInject constructor( } override suspend fun doWork(): Result { + if (cacheGuard.isCacheLocked()) return Result.retry() + val outboxDao = this.outboxDao.get() + val accountDao = this.accountDao.get() + val connectionFactory = this.connectionFactory.get() val pending = outboxDao.getAll() if (pending.isEmpty()) return Result.success() @@ -61,7 +71,7 @@ class SendWorker @AssistedInject constructor( ) val files = orderedAttachments(attachmentDir) if (account.authType == AuthType.OAUTH_OUTLOOK) { - sendOutlook(account, message, files) + sendOutlook(connectionFactory, account, message, files) } else { smtpSender.send( connectionFactory.smtpParamsFor(account), @@ -100,7 +110,12 @@ class SendWorker @AssistedInject constructor( * (a rejection, a pre-send/transport error, or a token failure); never fall back when the Graph * request may already have been accepted, or the message would be sent twice. */ - private suspend fun sendOutlook(account: Account, message: OutgoingMessage, files: List) { + private suspend fun sendOutlook( + connectionFactory: MailConnectionFactory, + account: Account, + message: OutgoingMessage, + files: List, + ) { try { val token = connectionFactory.graphTokenFor(account) graphSender.send(token, message, files) diff --git a/app/src/main/kotlin/org/libremail/data/sync/SyncWorker.kt b/app/src/main/kotlin/org/libremail/data/sync/SyncWorker.kt index 856bfbd..32a8b01 100644 --- a/app/src/main/kotlin/org/libremail/data/sync/SyncWorker.kt +++ b/app/src/main/kotlin/org/libremail/data/sync/SyncWorker.kt @@ -5,18 +5,28 @@ import android.content.Context import androidx.hilt.work.HiltWorker import androidx.work.CoroutineWorker import androidx.work.WorkerParameters +import dagger.Lazy import dagger.assisted.Assisted import dagger.assisted.AssistedInject +import org.libremail.data.security.EncryptedCacheGuard @HiltWorker class SyncWorker @AssistedInject constructor( @Assisted appContext: Context, @Assisted workerParams: WorkerParameters, - private val mailSyncer: MailSyncer, + // Lazy: constructing MailSyncer builds the Room DB, which blocks while the encrypted cache is + // locked. Resolve it only after confirming the cache is unlocked, so a locked run fails fast. + private val mailSyncer: Lazy, + private val cacheGuard: EncryptedCacheGuard, ) : CoroutineWorker(appContext, workerParams) { - override suspend fun doWork(): Result = mailSyncer.syncAll().fold( - onSuccess = { Result.success() }, - onFailure = { Result.retry() }, - ) + override suspend fun doWork(): Result { + // Can't open the encrypted DB without the user present — retry later rather than parking a + // WorkManager thread (which also wedges the shared serial executor) on an unsatisfiable await. + if (cacheGuard.isCacheLocked()) return Result.retry() + return mailSyncer.get().syncAll().fold( + onSuccess = { Result.success() }, + onFailure = { Result.retry() }, + ) + } } diff --git a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt index 957fad2..9f4bfd3 100644 --- a/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt +++ b/app/src/main/kotlin/org/libremail/di/DatabaseModule.kt @@ -35,7 +35,6 @@ import org.libremail.data.local.dao.MessageDao import org.libremail.data.local.dao.OutboxDao import org.libremail.data.local.dao.SignatureDao import org.libremail.data.security.DatabaseKeyStore -import org.libremail.data.security.PassphraseSession import org.libremail.data.settings.SettingsRepository import javax.inject.Singleton @@ -48,7 +47,6 @@ object DatabaseModule { fun provideDatabase( @ApplicationContext context: Context, keyStore: DatabaseKeyStore, - passphraseSession: PassphraseSession, settingsRepository: SettingsRepository, ): LibreMailDatabase { val builder = Room.databaseBuilder(context, LibreMailDatabase::class.java, DB_NAME) @@ -73,37 +71,39 @@ object DatabaseModule { // before the database is opened — so it never races an open connection; toggling the setting // therefore takes effect on the next app start. The passphrase is sealed by the Keystore. // - // When app-lock is ON the sealing key is auth-bound, so the passphrase can only be resolved - // AFTER the user authenticates at the lock screen. We read it from PassphraseSession, which - // blocks (this runs on a background DI thread, never the UI thread) until the unlock flow in - // AppLockViewModel unwraps it. This is what makes the encrypted cache "only readable after - // authentication". With app-lock OFF the master-sealed passphrase auto-unwraps as before. + // The passphrase source is resolved from which seal actually exists + // ([DatabaseKeyStore.resolvePassphrase]), NOT from the app-lock setting (a separate DataStore + // that can disagree). When app-lock is ON the sealing key is auth-bound, so resolvePassphrase + // waits on PassphraseSession until the user authenticates. This provider must therefore never + // be constructed on the main thread while the cache is locked — LibreMailApplication injects + // AccountRepository lazily and the sync/push workers fail fast when locked, and the gate + // composes no DB-backed screen until Unlocked. val dbFile = context.getDatabasePath(DB_NAME) // A screen-lock change (biometric re-enrollment / lock removal) can invalidate the auth-bound // key so the encrypted cache is no longer decryptable. AppLockViewModel records that and // restarts the app; we wipe the cache HERE — at cold start, before Room opens — so the file is - // never deleted from under an open connection. A re-sync then repopulates it. No corruption. - if (runBlocking { keyStore.consumeClearPending() }) { + // never deleted from under an open connection. Crash-safe order: wipe + reset the seals, and + // only THEN clear the flag, so a kill mid-wipe just repeats the idempotent wipe next start. + if (runBlocking { keyStore.isClearPending() }) { DatabaseFiles.clear(context) - runBlocking { keyStore.resetSealedPassphrase() } + runBlocking { + keyStore.resetSealedPassphrase() + keyStore.clearClearPending() + } } val settings = runBlocking { settingsRepository.settings.first() } val appLock = settings.appLock if (settings.encryptCache) { - val passphrase = runBlocking { - if (appLock) passphraseSession.await() else keyStore.passphrase() - } + val passphrase = runBlocking { keyStore.resolvePassphrase(appLock) } DatabaseEncryption.ensureEncrypted(dbFile, passphrase) builder.openHelperFactory( SupportOpenHelperFactory(passphrase.toByteArray(Charsets.US_ASCII), null, false), ) } else if (DatabaseEncryption.isEncrypted(dbFile)) { // Encryption was turned back off — decrypt so the default (unkeyed) open succeeds. - val passphrase = runBlocking { - if (appLock) passphraseSession.await() else keyStore.passphrase() - } + val passphrase = runBlocking { keyStore.resolvePassphrase(appLock) } DatabaseEncryption.ensurePlaintext(dbFile, passphrase) } return builder.build() diff --git a/app/src/main/kotlin/org/libremail/push/IdleService.kt b/app/src/main/kotlin/org/libremail/push/IdleService.kt index 4b7fd1d..5201c04 100644 --- a/app/src/main/kotlin/org/libremail/push/IdleService.kt +++ b/app/src/main/kotlin/org/libremail/push/IdleService.kt @@ -11,6 +11,7 @@ import android.util.Log import androidx.core.app.NotificationCompat import androidx.core.app.NotificationManagerCompat import androidx.core.app.ServiceCompat +import dagger.Lazy import dagger.hilt.android.AndroidEntryPoint import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope @@ -26,6 +27,7 @@ import kotlinx.coroutines.withTimeoutOrNull import org.libremail.R import org.libremail.data.local.dao.AccountDao import org.libremail.data.local.toDomain +import org.libremail.data.security.EncryptedCacheGuard import org.libremail.data.sync.MailConnectionFactory import org.libremail.data.sync.MailSyncer import org.libremail.domain.model.Account @@ -40,13 +42,18 @@ import javax.inject.Inject @AndroidEntryPoint class IdleService : Service() { - @Inject lateinit var accountDao: AccountDao + // Lazy: AccountDao / MailConnectionFactory (via CredentialStore) / MailSyncer all resolve the Room + // DB, which blocks while the encrypted cache is locked. Resolve them only after the cache-lock + // check in onStartCommand, so a start into a still-locked process defers instead of ANR-ing. + @Inject lateinit var accountDao: Lazy - @Inject lateinit var connectionFactory: MailConnectionFactory + @Inject lateinit var connectionFactory: Lazy @Inject lateinit var imapClient: ImapClient - @Inject lateinit var mailSyncer: MailSyncer + @Inject lateinit var mailSyncer: Lazy + + @Inject lateinit var cacheGuard: EncryptedCacheGuard private val scope = CoroutineScope(SupervisorJob() + Dispatchers.IO) private var watching = false @@ -58,7 +65,16 @@ class IdleService : Service() { startAsForeground() if (!watching) { watching = true - scope.launch { reconcileWatchers() } + scope.launch { + // Can't open the encrypted DB without the user present. Defer (stop) and let the app + // restart push after the next unlock, rather than block the service and ANR. + if (cacheGuard.isCacheLocked()) { + Log.i(TAG, "encrypted cache locked; deferring IDLE push until the app is unlocked") + stopSelf() + return@launch + } + reconcileWatchers() + } } return START_STICKY } @@ -70,7 +86,7 @@ class IdleService : Service() { * accounts exist, so reaching zero here is just a transient state. */ private suspend fun reconcileWatchers() { - accountDao.observeAll().collect { entities -> + accountDao.get().observeAll().collect { entities -> val accounts = entities.map { it.toDomain() } val currentIds = accounts.mapTo(mutableSetOf()) { it.id } (watchers.keys - currentIds).forEach { id -> watchers.remove(id)?.cancel() } @@ -94,10 +110,10 @@ class IdleService : Service() { var backoffMs = INITIAL_BACKOFF_MS while (scope.isActive) { try { - val params = connectionFactory.imapParamsFor(account) + val params = connectionFactory.get().imapParamsFor(account) withTimeoutOrNull(IDLE_RENEWAL_MS) { // Sync just this account on its own push — not every account. - imapClient.idle(params) { mailSyncer.syncAccount(account.id) } + imapClient.idle(params) { mailSyncer.get().syncAccount(account.id) } } backoffMs = INITIAL_BACKOFF_MS } catch (e: CancellationException) { diff --git a/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt b/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt index d049724..5fbe375 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockGateHost.kt @@ -2,6 +2,7 @@ package org.libremail.ui.lock import androidx.biometric.BiometricPrompt +import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Surface @@ -9,7 +10,11 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.Modifier +import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.res.stringResource import androidx.core.content.ContextCompat @@ -64,20 +69,54 @@ fun AppLockGateHost(viewModel: AppLockViewModel = hiltViewModel(), content: @Com ) } - when (val state = uiState) { - AppLockUiState.Checking -> - Surface(modifier = Modifier.fillMaxSize(), color = MaterialTheme.colorScheme.background) {} + // Once unlocked, keep [content] in the composition across later re-locks so its state (navigation + // position, in-progress compose drafts, scroll) survives — the lock screen is drawn OVER it rather + // than replacing it. Content is never composed before the first unlock, so no DB-backed screen + // opens while the cache is still locked; a fresh process starts locked, so [remember] (not + // rememberSaveable) is intentional — content stays uncomposed until this session authenticates. + var hasEverUnlocked by remember { mutableStateOf(uiState is AppLockUiState.Unlocked) } + LaunchedEffect(uiState) { if (uiState is AppLockUiState.Unlocked) hasEverUnlocked = true } - AppLockUiState.Unlocked -> content() + Box(modifier = Modifier.fillMaxSize()) { + if (hasEverUnlocked) content() - is AppLockUiState.Locked -> { - LockScreen(error = state.error, onUnlock = authenticate) - // Auto-present the prompt the first time the lock screen appears; the button covers retries. - LaunchedEffect(Unit) { authenticate() } + when (val state = uiState) { + AppLockUiState.Unlocked -> Unit + + AppLockUiState.Checking -> LockCover() + + is AppLockUiState.Locked -> { + LockCover { LockScreen(error = state.error, onUnlock = authenticate) } + // Auto-present the prompt when the lock screen (re)appears from an unlocked/checking + // state; the button covers manual retries. Keyed to entering the Locked branch, so a + // retry (which stays in this branch) shows its error without re-triggering the prompt. + LaunchedEffect(Unit) { authenticate() } + } } } } +/** + * Opaque, input-blocking overlay drawn on top of [content] while the app is locked or still + * resolving, so nothing underneath is visible or interactable. Screenshot / recents protection is the + * window's FLAG_SECURE (set by MainActivity while app-lock is on), which the composition can't do. + */ +@Composable +private fun LockCover(content: @Composable () -> Unit = {}) { + Surface( + modifier = Modifier + .fillMaxSize() + .pointerInput(Unit) { + awaitPointerEventScope { + while (true) { + awaitPointerEvent().changes.forEach { it.consume() } + } + } + }, + color = MaterialTheme.colorScheme.background, + ) { content() } +} + /** * Presents a `BiometricPrompt` accepting a strong biometric OR the device credential. No negative * button is set because that is disallowed when [DEVICE_CREDENTIAL][AppLockManager.AUTHENTICATORS] is diff --git a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt index 48d3214..c394d21 100644 --- a/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/lock/AppLockViewModel.kt @@ -11,6 +11,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import dagger.hilt.android.qualifiers.ApplicationContext +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -18,6 +19,7 @@ import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import kotlinx.coroutines.withContext +import org.libremail.R import org.libremail.data.security.AppLockGate import org.libremail.data.security.AppLockManager import org.libremail.data.security.DatabaseKeyCipher @@ -38,8 +40,12 @@ sealed interface AppLockUiState { /** App-lock is off or the user has authenticated: show the app. */ data object Unlocked : AppLockUiState - /** Show the lock screen; [error] is a human-readable reason the previous attempt failed. */ - data class Locked(val error: String? = null) : AppLockUiState + /** + * Show the lock screen; [error] is a human-readable reason the previous attempt failed. [nonce] + * makes each lock emission distinct so a retry (same or null error text) still updates the UI + * rather than being swallowed by StateFlow's equality conflation. + */ + data class Locked(val error: String? = null, val nonce: Int = 0) : AppLockUiState } /** @@ -74,13 +80,41 @@ class AppLockViewModel @Inject constructor( private val _uiState = MutableStateFlow(AppLockUiState.Checking) val uiState: StateFlow = _uiState.asStateFlow() + // Cached so onBackground / onForeground can cover the content synchronously (before the async + // settings read) whenever app-lock is on — so no stale mailbox frame renders on resume. + @Volatile private var appLockEnabledCached = false + + // Bumped on every lock emission so two consecutive locks with identical error text still differ + // (StateFlow conflates equal values), guaranteeing the lock screen updates — e.g. on a retry. + private var lockSeq = 0 + + init { + viewModelScope.launch { + settingsRepository.settings.collect { appLockEnabledCached = it.appLock } + } + } + + private fun emitLocked(error: String? = null) { + _uiState.value = AppLockUiState.Locked(error, ++lockSeq) + } + /** Recompute the lock state when the app comes to the foreground (lifecycle ON_START). */ fun onForeground() { + // Capture the foreground timestamp synchronously (before any suspension) so a concurrent + // onBackground can't corrupt the grace calculation for this pass (see AppLockGate.onForeground). + val foregroundAt = now() viewModelScope.launch { val settings = settingsRepository.settings.first() + if (!settings.appLock) { + _uiState.value = AppLockUiState.Unlocked + return@launch + } + // App-lock is on: cover any showing content while we resolve, so no stale mailbox frame + // renders before the (async) decision lands. + if (_uiState.value == AppLockUiState.Unlocked) _uiState.value = AppLockUiState.Checking val action = withContext(Dispatchers.Default) { KeyInvalidationPolicy.decide( - appLockEnabled = settings.appLock, + appLockEnabled = true, encryptCacheEnabled = settings.encryptCache, deviceSecure = appLockManager.isDeviceSecure(), keyInvalidated = databaseKeyCipher.isInvalidated(), @@ -94,17 +128,13 @@ class AppLockViewModel @Inject constructor( _uiState.value = AppLockUiState.Unlocked } - LockAction.CLEAR_AND_DISABLE -> { - settingsRepository.setAppLock(false) - clearCacheAndRestart() - } + LockAction.CLEAR_AND_DISABLE -> clearCacheAndRestart(disableAppLock = true) - LockAction.CLEAR_AND_REQUIRE_AUTH -> clearCacheAndRestart() + LockAction.CLEAR_AND_REQUIRE_AUTH -> clearCacheAndRestart(disableAppLock = false) LockAction.REQUIRE_AUTH -> { - val state = gate.onForeground(now(), appLockEnabled = true) - _uiState.value = - if (state == LockState.UNLOCKED) AppLockUiState.Unlocked else AppLockUiState.Locked() + val state = gate.onForeground(foregroundAt, appLockEnabled = true) + if (state == LockState.UNLOCKED) _uiState.value = AppLockUiState.Unlocked else emitLocked() } } } @@ -113,6 +143,11 @@ class AppLockViewModel @Inject constructor( /** Record the app being backgrounded so the inactivity grace period can be evaluated on return. */ fun onBackground() { gate.onBackground(now()) + // Cover the content the moment we background so nothing sensitive is in the last-rendered frame + // (recents snapshot) or briefly visible on the next resume before the re-lock decision lands. + if (appLockEnabledCached && _uiState.value == AppLockUiState.Unlocked) { + _uiState.value = AppLockUiState.Checking + } } /** Called by the host after a successful `BiometricPrompt`. */ @@ -128,12 +163,12 @@ class AppLockViewModel @Inject constructor( // The passphrase is permanently unrecoverable (key invalidated or deleted by a // screen-lock change). Wipe the cache safely at the next cold start and re-sync. Log.w(TAG, "encrypted cache passphrase unrecoverable; clearing cache") - clearCacheAndRestart() + clearCacheAndRestart(disableAppLock = false) } UnlockResult.RETRY -> { gate.lock() - _uiState.value = AppLockUiState.Locked() + emitLocked(context.getString(R.string.app_lock_unlock_failed)) } } } @@ -142,7 +177,7 @@ class AppLockViewModel @Inject constructor( /** Called by the host when the prompt is cancelled or errors. */ fun onAuthError(message: String?) { gate.lock() - _uiState.value = AppLockUiState.Locked(message) + emitLocked(message) } /** Outcome of unwrapping/arming the auth-bound passphrase after a successful device auth. */ @@ -190,12 +225,24 @@ class AppLockViewModel @Inject constructor( } catch (e: UserNotAuthenticatedException) { Log.w(TAG, "auth window elapsed before unwrap; will retry", e) UnlockResult.RETRY + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // Any other failure (corrupt sealed blob, OEM keymaster error, etc.) must NOT crash the + // process right after a successful auth. Re-lock and let the user retry rather than wiping + // the cache on an ambiguous error (a genuinely lost key still surfaces as UNRECOVERABLE + // via hasKey()/KeyPermanentlyInvalidatedException above). + Log.w(TAG, "unexpected failure unwrapping auth-sealed passphrase; will retry", e) + UnlockResult.RETRY } } - private suspend fun clearCacheAndRestart() { + private suspend fun clearCacheAndRestart(disableAppLock: Boolean) { withContext(Dispatchers.Default) { + // Record the wipe intent BEFORE flipping app-lock off, so a crash between the two writes + // leaves the wipe still pending (recoverable) rather than a disabled gate over a stale key. databaseKeyStore.setClearPending() + if (disableAppLock) settingsRepository.setAppLock(false) syncScheduler.syncNow() // persisted by WorkManager; survives the restart } restartProcess() diff --git a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt index 68872cc..1ba48a5 100644 --- a/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/settings/SettingsViewModel.kt @@ -9,7 +9,6 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.update import kotlinx.coroutines.launch @@ -84,10 +83,11 @@ class SettingsViewModel @Inject constructor( } settingsRepository.setAppLock(true) } else { - if (settingsRepository.settings.first().encryptCache) { - // Move the passphrase back under the master key BEFORE dropping the gate, or the next - // launch derives a fresh (wrong) key and can't open the encrypted cache. If it fails, - // keep app-lock on rather than risk that mismatch. + // Reseal under the master key whenever an auth seal actually exists — gate on the seal, not + // the encryptCache setting (a separate store that can already be off while the on-disk DB is + // still auth-sealed). Do it BEFORE dropping the gate, or the next launch can't open the + // cache. If it fails, keep app-lock on rather than strand the passphrase. + if (databaseKeyStore.hasAuthSealedPassphrase()) { if (runCatching { databaseKeyStore.sealWithMaster() }.isFailure) { _appLockMessage.value = R.string.app_lock_disable_failed return@update diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index e46121e..f1aa47b 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -246,6 +246,7 @@ Confirm your screen lock to decrypt your mail. Set up a device screen lock in Android settings before enabling this. Couldn\'t turn off the screen lock right now. Please try again. + Couldn\'t unlock. Please try again. Background battery usage Unrestricted — instant background mail is allowed. diff --git a/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt b/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt index d7fa7a4..1ab1bee 100644 --- a/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt +++ b/app/src/test/kotlin/org/libremail/data/security/AppLockGateTest.kt @@ -78,4 +78,26 @@ class AppLockGateTest { gate.lock() assertEquals(LockState.UNLOCKED, gate.onForeground(now = 100, appLockEnabled = false)) } + + @Test + fun `a background recorded after a foreground pass began does not unlock on that stale pass`() { + // Regression (lock-bypass race): a stale foreground pass whose timestamp was captured before a + // later background must NOT treat that background as a within-grace return and stay unlocked. + val gate = AppLockGate(grace) + gate.onForeground(now = 0, appLockEnabled = true) + gate.onAuthenticated() // UNLOCKED + val staleForegroundAt = 1_000L + gate.onBackground(now = 2_000L) // background happens AFTER the stale pass began + assertEquals(LockState.LOCKED, gate.onForeground(now = staleForegroundAt, appLockEnabled = true)) + } + + @Test + fun `the genuine return after a stale pass still requires authentication`() { + val gate = AppLockGate(grace) + gate.onForeground(now = 0, appLockEnabled = true) + gate.onAuthenticated() + gate.onBackground(now = 2_000L) + gate.onForeground(now = 1_000L, appLockEnabled = true) // stale -> LOCKED, marker retained + assertEquals(LockState.LOCKED, gate.onForeground(now = 3_000L, appLockEnabled = true)) + } } diff --git a/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt b/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt index 7df1cd5..66fab93 100644 --- a/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt +++ b/app/src/test/kotlin/org/libremail/data/security/KeyInvalidationPolicyTest.kt @@ -51,4 +51,30 @@ class KeyInvalidationPolicyTest { // Both true: the device being insecure dominates (can't authenticate at all). assertEquals(LockAction.CLEAR_AND_DISABLE, decide(secure = false, invalidated = true, encrypt = true)) } + + @Test + fun `full decision table is pinned`() { + // App-lock off: always proceed, regardless of the other three inputs (all 8 combinations). + for (e in listOf(false, true)) { + for (s in listOf(false, true)) { + for (i in listOf(false, true)) { + assertEquals( + LockAction.PROCEED, + decide(appLock = false, encrypt = e, secure = s, invalidated = i), + ) + } + } + } + // App-lock on, device no longer secure: clear+disable iff there is an encrypted cache to lose. + assertEquals(LockAction.CLEAR_AND_DISABLE, decide(secure = false, encrypt = true, invalidated = false)) + assertEquals(LockAction.CLEAR_AND_DISABLE, decide(secure = false, encrypt = true, invalidated = true)) + assertEquals(LockAction.DISABLE_APP_LOCK, decide(secure = false, encrypt = false, invalidated = false)) + assertEquals(LockAction.DISABLE_APP_LOCK, decide(secure = false, encrypt = false, invalidated = true)) + // App-lock on, secure, key invalidated: clear+re-auth iff encrypted, else just re-auth. + assertEquals(LockAction.CLEAR_AND_REQUIRE_AUTH, decide(secure = true, encrypt = true, invalidated = true)) + assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = false, invalidated = true)) + // App-lock on, secure, key valid: the common case — require auth (previously unpinned rows). + assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = true, invalidated = false)) + assertEquals(LockAction.REQUIRE_AUTH, decide(secure = true, encrypt = false, invalidated = false)) + } }