From a7a7c323b51005c6cdb1abd4f1ee9da250ff1be7 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Thu, 2 Jul 2026 09:45:54 -0500 Subject: [PATCH] refactor(security): derive backup exclusion set from DatabaseFiles Make BackupPolicy.EXCLUDED_DATABASE_PATHS the true single source of truth by deriving it from DatabaseFiles.NAME and DatabaseFiles.ACCOUNTS_NAME plus their SQLite sidecars via a new DatabaseFiles.fileNames() helper, instead of a hand-maintained list. This adds libremail-accounts.db (accounts + encrypted credentials, split into their own DB by #118/#111) to the never-back-up set, matching the field's stated intent, so a newly added database can never silently fall out of the exclusions again. Also fix DatabaseFiles.clear to wipe the cache DB via context.deleteDatabase(NAME), which additionally removes the -mj* master-journal temp files the hand-rolled suffix list missed. It still wipes ONLY the cache DB (NAME) and never the accounts DB (ACCOUNTS_NAME), preserving the sign-in-survives-cache-wipe separation from #111. Update the backup XML comments (data_extraction_rules.xml, backup_rules.xml) to note libremail-accounts.db is also kept off-device by the strict include- allowlist, and extend the tests to assert the accounts DB is covered by the exclusion SoT and that the derivation stays in lockstep with the XML resources. There is no active backup leak today: the XML is a strict include-allowlist, so the accounts DB was already excluded by omission. This closes the SoT drift #118 introduced and the -mj* gap, so the security posture no longer depends on the allowlist staying strict by luck. Closes #103 Co-Authored-By: Claude Fable 5 --- .../org/libremail/backup/BackupPolicy.kt | 18 ++++++----- .../org/libremail/data/local/DatabaseFiles.kt | 31 +++++++++++++------ app/src/main/res/xml/backup_rules.xml | 9 +++--- .../main/res/xml/data_extraction_rules.xml | 7 +++-- .../org/libremail/backup/BackupPolicyTest.kt | 28 ++++++++++++++++- .../backup/DataExtractionRulesTest.kt | 10 ++++++ 6 files changed, 79 insertions(+), 24 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt b/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt index 2ba34a6..1d9ba2c 100644 --- a/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt +++ b/app/src/main/kotlin/org/libremail/backup/BackupPolicy.kt @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later package org.libremail.backup +import org.libremail.data.local.DatabaseFiles import org.libremail.data.settings.AppSettings /** @@ -24,13 +25,16 @@ object BackupPolicy { "datastore/libremail_dbkey.preferences_pb", ) - /** `databases`-dir-relative names that must never leave the device (encrypted credentials + mail cache). */ - val EXCLUDED_DATABASE_PATHS: List = listOf( - "libremail.db", - "libremail.db-wal", - "libremail.db-shm", - "libremail.db-journal", - ) + /** + * `databases`-dir-relative names that must never leave the device: the encrypted mail cache + * ([DatabaseFiles.NAME]) AND the accounts + encrypted-credentials database + * ([DatabaseFiles.ACCOUNTS_NAME]), each with its SQLite sidecars. Derived from [DatabaseFiles] + * rather than hand-listed, so a newly added database can never silently fall out of the + * never-back-up set (issue #103). + */ + val EXCLUDED_DATABASE_PATHS: List = + DatabaseFiles.fileNames(DatabaseFiles.NAME) + + DatabaseFiles.fileNames(DatabaseFiles.ACCOUNTS_NAME) /** * Whether Android Backup may run for this app. Opt-in and OFF by default: nothing is backed up diff --git a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt index 4e52837..ab85e54 100644 --- a/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt +++ b/app/src/main/kotlin/org/libremail/data/local/DatabaseFiles.kt @@ -2,9 +2,8 @@ package org.libremail.data.local import android.content.Context -import java.io.File -/** Central name and wipe helper for the Room cache database file (and its SQLite sidecars). */ +/** Central names and wipe helper for the Room database files (and their SQLite sidecars). */ object DatabaseFiles { const val NAME = "libremail.db" @@ -17,15 +16,27 @@ object DatabaseFiles { const val ACCOUNTS_NAME = "libremail-accounts.db" /** - * Delete the database and any WAL/SHM/journal sidecars. Call only when no connection is open — - * used by the "clear + re-sync" path when the encryption key is invalidated and the encrypted - * database can no longer be decrypted. + * The statically-nameable SQLite sidecars that accompany a database file. A transient `-mj*` + * master journal can also exist, but its suffix is random and so can't be listed by name — + * [clear] leans on [Context.deleteDatabase] to sweep that one up. + */ + private val SIDECAR_SUFFIXES = listOf("-wal", "-shm", "-journal") + + /** + * [name] plus each of its statically-nameable sidecars. The single source of truth for which + * on-disk files make up a database file; `BackupPolicy` derives its never-back-up set from this + * so a new database (or a new sidecar suffix) can never silently fall out of the exclusions. + */ + fun fileNames(name: String): List = listOf(name) + SIDECAR_SUFFIXES.map { name + it } + + /** + * Delete the cache database ([NAME]) and every sidecar — including the `-mj*` master journal a + * hand-rolled suffix list would miss — via [Context.deleteDatabase]. NEVER touches + * [ACCOUNTS_NAME], so a cache-key invalidation keeps the user signed in (issue #111). Call only + * when no connection is open — used by the "clear + re-sync" path when the encryption key is + * invalidated and the encrypted database can no longer be decrypted. */ fun clear(context: Context) { - val db = context.getDatabasePath(NAME) - val dir = db.parentFile ?: return - listOf("", "-wal", "-shm", "-journal").forEach { suffix -> - File(dir, db.name + suffix).delete() - } + context.deleteDatabase(NAME) } } diff --git a/app/src/main/res/xml/backup_rules.xml b/app/src/main/res/xml/backup_rules.xml index 0059fd1..ce18508 100644 --- a/app/src/main/res/xml/backup_rules.xml +++ b/app/src/main/res/xml/backup_rules.xml @@ -5,10 +5,11 @@ which applies on API 31+. Backup is still gated by LibreMailBackupAgent (opt-in, OFF by default). makes this a strict allowlist: ONLY the libremail_settings DataStore is backed up. The - Keystore-sealed cache passphrase (datastore/libremail_dbkey.preferences_pb) and the encrypted - credentials + mail-cache database (libremail.db and its -wal/-shm/-journal side files) are kept - off-device by being omitted from the allowlist; the mail cache re-downloads on the next sync and - accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths + Keystore-sealed cache passphrase (datastore/libremail_dbkey.preferences_pb), the mail-cache + database (libremail.db) and the accounts + encrypted-credentials database (libremail-accounts.db, + which issue #111 split out of libremail.db) — each with their -wal/-shm/-journal side files — are + kept off-device by being omitted from the allowlist; the mail cache re-downloads on the next sync + and accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths outside an , so exclusion is expressed by omission rather than explicit entries.) --> diff --git a/app/src/main/res/xml/data_extraction_rules.xml b/app/src/main/res/xml/data_extraction_rules.xml index 0f07896..d3ca8b0 100644 --- a/app/src/main/res/xml/data_extraction_rules.xml +++ b/app/src/main/res/xml/data_extraction_rules.xml @@ -11,8 +11,11 @@ - datastore/libremail_dbkey.preferences_pb: the Keystore-sealed SQLCipher passphrase for the encrypted cache. The wrapping Keystore key is non-exportable and device-bound, so the ciphertext is useless anywhere else. - - libremail.db (+ -wal/-shm/-journal): encrypted IMAP/OAuth credentials and the cached mail. - The cache re-downloads on the next sync; accounts are re-added on a new device. + - libremail.db (+ -wal/-shm/-journal): the cached mail; re-downloads on the next sync. + - libremail-accounts.db (+ -wal/-shm/-journal): accounts, encrypted IMAP/OAuth credentials, + per-account settings and signatures (issue #111 moved these out of libremail.db). The + credentials are sealed with a device-bound key, so they would only ever restore as + undecryptable ciphertext; accounts are re-added on a new device. (Lint's FullBackupContent rule forbids paths outside an , so the exclusions are expressed by simply not listing those paths rather than as explicit entries.) --> diff --git a/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt b/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt index c9d0924..c60e33d 100644 --- a/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt +++ b/app/src/test/kotlin/org/libremail/backup/BackupPolicyTest.kt @@ -2,6 +2,7 @@ package org.libremail.backup import org.junit.Test +import org.libremail.data.local.DatabaseFiles import org.libremail.data.settings.AppSettings import kotlin.test.assertEquals import kotlin.test.assertFalse @@ -37,10 +38,35 @@ class BackupPolicyTest { } @Test - fun `the credentials and mail-cache database is never eligible for backup`() { + fun `the mail-cache database is never eligible for backup`() { assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db")) // WAL/SHM/journal side-files can hold recently written rows too. assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-wal")) assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-shm")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail.db-journal")) + } + + @Test + fun `the accounts and credentials database is never eligible for backup`() { + // Since #111 the accounts + encrypted IMAP/OAuth credentials live in their OWN database file + // (libremail-accounts.db), so it must be in the never-back-up set just like the cache. + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-wal")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-shm")) + assertTrue(BackupPolicy.EXCLUDED_DATABASE_PATHS.contains("libremail-accounts.db-journal")) + } + + @Test + fun `excluded database paths are derived from DatabaseFiles so none can silently fall out`() { + val derived = DatabaseFiles.fileNames(DatabaseFiles.NAME) + + DatabaseFiles.fileNames(DatabaseFiles.ACCOUNTS_NAME) + // Derived, not hand-maintained: the exclusion set is exactly the DatabaseFiles-known files — + // no more (nothing stale) and no less (every DB + sidecar covered). + assertEquals(derived, BackupPolicy.EXCLUDED_DATABASE_PATHS) + listOf(DatabaseFiles.NAME, DatabaseFiles.ACCOUNTS_NAME).forEach { name -> + DatabaseFiles.fileNames(name).forEach { path -> + assertTrue(path in BackupPolicy.EXCLUDED_DATABASE_PATHS, "$path must never be backed up") + } + } } } diff --git a/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt b/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt index 2de642c..e70a1c5 100644 --- a/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt +++ b/app/src/test/kotlin/org/libremail/backup/DataExtractionRulesTest.kt @@ -2,6 +2,7 @@ package org.libremail.backup import org.junit.Test +import org.libremail.data.local.DatabaseFiles import org.w3c.dom.Element import java.io.File import javax.xml.parsers.DocumentBuilderFactory @@ -73,4 +74,13 @@ class DataExtractionRulesTest { fun `full backup content (API 29-30) mirrors the same exclusions`() { assertSafe(parseSection(resource("backup_rules.xml"), "full-backup-content")) } + + @Test + fun `both the cache and accounts databases are guarded against the backup allowlist`() { + // The exclusion SoT is derived from DatabaseFiles, so both databases flow into secretPaths + // and are asserted-absent from every include section by assertSafe above. Pin that here so + // the accounts + credentials DB added in #111 can't quietly drop out of the guarded set. + assertTrue(secretPaths.contains("database:${DatabaseFiles.NAME}")) + assertTrue(secretPaths.contains("database:${DatabaseFiles.ACCOUNTS_NAME}")) + } } -- 2.47.3