Move database out of iCloud backup path (NEW-20)#293
Closed
TaprootFreak wants to merge 1 commit into
Closed
Conversation
On iOS, getApplicationDocumentsDirectory() maps to Documents/ which is included in iCloud backups by default. The encrypted wallet database was therefore uploaded to Apple servers. Moved to getApplicationSupportDirectory() (Library/Application Support/) which is excluded from backups. Existing databases are automatically migrated on first launch after update.
Collaborator
|
Why don't we just disable backups? |
Contributor
Author
|
|
Danswar
added a commit
that referenced
this pull request
Jul 4, 2026
Hardens the at-rest handling of seed and key material flagged by the source-level audit in #612, plus the related iOS-backup exposure NEW-20 from #298. Refs #612 Refs #298 (NEW-20) ## Findings | # | Sev | Decision | Fix | Test | |---|-----|----------|-----|------| | S1 | HIGH | **Scoped** screenshot protection, not app-wide `FLAG_SECURE` (rationale below); close the one remaining gap | `restore_wallet_view` (user types their existing 12 words) now blocks screenshots + the app-switcher snapshot on `initState` / re-enables on `dispose`, like the sibling seed screens | widget test asserts the `no_screenshot` channel gets `screenshotOff` on init, `screenshotOn` on dispose | | S2 | HIGH | Already fixed on `staging` | `deleteCurrentWallet` → `purgeWallet` (seed row **and** mnemonic key) landed in #621; regenerate path still uses account-only delete | covered by existing `wallet_repository_test` / `wallet_service_test` | | S3 | MED | Owned by #619 | — | — | | S4 | MED | Arm the auto-lock on `paused` **and** `hidden`; **not** on `inactive` | `lifecycle_initializer` arms the mnemonic-drop + PIN re-lock from both `hidden` and `paused`, made idempotent per background episode | 6 widget tests (paused arms, hidden+paused / paused+hidden lock once, inactive inert, resume re-arms) | | S5 | MED | **Deferred** — cannot verify a safe migration without on-device runs (details below) | runtime unchanged | — | | S6 | LOW | Fail typed & gracefully | `decryptSeed` throws a typed `SeedDecryptionException` for a missing separator, invalid base64, or GCM auth failure instead of a raw `RangeError` / `InvalidCipherTextException` | static tests for each failure branch + toString | | NEW-20 | — | Exclude the encrypted DB from iCloud **in place** (no move) | `NSURLIsExcludedFromBackupKey` set on the SQLCipher file + `-wal`/`-shm`/`-journal` sidecars via a method channel; wired into boot | `database_test` asserts the exact path set; adapter is the platform boundary | ## S1 — why scoped, not app-wide `FLAG_SECURE` I inspected the `no_screenshot` **1.1.0** source (`com.flutterplaza.no_screenshot.NoScreenshotPlugin`): `screenshotOff()` calls `window.addFlags(FLAG_SECURE)` and `screenshotOn()` calls `clearFlags(FLAG_SECURE)`. `FLAG_SECURE` is exactly the OS mechanism that blanks the recents/app-switcher thumbnail **and** blocks OS/3rd-party screen recording. So on the seed screens where `screenshotOff()` is active, those vectors are already closed while the screen is mounted — correcting the finding's premise that `no_screenshot` covers only manual screenshots. App-wide `FLAG_SECURE` in `MainActivity` was considered and **declined**: - **Technical incompatibility.** `no_screenshot` toggles the *same* window `FLAG_SECURE`. A flag set globally in `MainActivity.onCreate` would be silently *cleared* by the `screenshotOn()` in any seed screen's `dispose`, leaving the whole app unprotected afterwards. Making app-wide work would require ripping `no_screenshot` out entirely — which directly collides with the in-flight #619 (adds `no_screenshot` to verify-seed). - **Product stance.** The codebase deliberately keeps non-sensitive screens screenshot-able (e.g. receive-QR sharing); #619 reaffirms the per-screen pattern with the comment *"so non-sensitive screens stay screenshot-able."* The residual risk in the scoped model is *completeness* — a screen that shows/handles seed words but forgets to opt in (exactly the S3 miss). Auditing the seed screens found `restore_wallet` unprotected (the user types their real recovery phrase there); this PR closes it. `create_wallet` and `settings_seed` were already protected; verify-seed is handled by #619. If the team later wants belt-and-suspenders app-wide protection, the clean path is to drop `no_screenshot` and set `FLAG_SECURE` in `MainActivity`, accepting the loss of legitimate screenshots — a product call, out of scope here. iOS app-switcher blanking is not included (Android-focused finding; iOS would need a native cover-view — not cheap/clean). ## S4 — lock on `paused`/`hidden`, not `inactive` Only `hidden` armed the auto-lock + mnemonic drop, so a platform that skips/coalesces `hidden` left `_lastBackgroundTime == null` and never force-locked. Now both `hidden` and `paused` arm it, guarded by a per-episode flag (reset on `resumed`) so the `hidden`+`paused` pair — and their double `lockCurrentWallet()`, which would otherwise double-decrement the active-unlock-holder count — locks exactly once. `inactive` is intentionally **not** a trigger: on iOS it fires for transient, still-foreground interruptions — the biometric (Face ID/Touch ID) prompt, permission dialogs, the incoming-call banner, Control Center. The biometric unlock prompt in particular raises `inactive`, so locking there would drop the mnemonic and re-arm the PIN gate in the middle of the very unlock the user is completing. ## S5 — deferred, with recommendation `const FlutterSecureStorage()` is constructed without hardened options (Android `encryptedSharedPreferences`, iOS `KeychainAccessibility`). It holds the SQLCipher DB key, the mnemonic AES key, and the PIN hash/lockout. I am **not** changing this at runtime, because I cannot establish a verified-safe migration without on-device runs, and the downside is catastrophic: on `flutter_secure_storage` **9.2.4**, flipping Android to `encryptedSharedPreferences: true` switches the storage backend, and existing entries written by the legacy cipher backend can become **unreadable** — losing the mnemonic-decryption key would brick every existing wallet. A read-old → rewrite-new migration is possible in principle but must be validated on real Android/iOS devices with an upgrade-in-place install, which is outside this environment. **Recommendation (follow-up):** - Android: add `AndroidOptions(encryptedSharedPreferences: true)` behind a one-time, idempotent read-legacy → rewrite-hardened migration, gated on a persisted `migratedV2` marker; validate an upgrade-in-place install on-device before shipping. - iOS: set `IOSOptions(accessibility: KeychainAccessibility.first_unlock_this_device)` so keys are not eligible for iCloud-Keychain/backup restore onto another device. Reads of existing items remain valid (accessibility governs *when* readable, not the lookup); still verify on-device. ## S6 `decryptSeed` now guards the missing-separator case and wraps base64/GCM failures in a typed `SeedDecryptionException`, so a corrupted `walletInfos.seed` row surfaces a catchable error on the unlock path instead of an uncaught `RangeError`. AES-GCM's rejection of tampered ciphertext is unchanged — only *typed*. ## NEW-20 (#298) — DB out of iCloud backup The SQLCipher DB lives in `Documents/`, which iOS backs up to iCloud. The earlier move-based #293 was closed unmerged; note that `Library/Application Support/` is itself backed up by default, so a move alone would not have excluded it. This PR excludes the file **in place** via `NSURLIsExcludedFromBackupKey` (no move → no migration risk to a wallet-bearing file), covering the `-wal`/`-shm`/`-journal` sidecars too, idempotently on each launch. Android is unaffected (`android:allowBackup="false"`). ## Coordination - No overlap with #757: it rewrites `verifyPin` (adds `PinVerificationResult`); this PR touches `decryptSeed` and the `FlutterSecureStorage` *nothing*, and the new `SeedDecryptionException` sits at the end of the file, away from #757's edits. - S2 (#621) and S3 (#619) are noted above, not re-fixed. ## Tests Targeted suites green on Flutter 3.41.6 (storage, repository, lifecycle, restore-wallet). `flutter analyze` clean on all changed files. Kotlin/Swift changes are minimal and not emulator-testable here; the Dart side is covered. ## Post-review corrections (2026-07-04) - **S4 premise:** Flutter ≥3.13 synthesizes `hidden` before every `paused` (`ServicesBinding._generateStateTransitions`), so a platform delivering `paused` without `hidden` is unreachable and the described lock gap was theoretical. The change is still worthwhile for a different reason: the per-episode idempotence guard suppresses the duplicate `lockCurrentWallet()` the base code fired on every `hidden`+`paused` episode (double-decrementing the unlock-holder count). - **NEW-20 sidecars:** the DB runs in default rollback-journal mode (no WAL pragma; drift 2.32.1 doesn't enable it), so `-wal`/`-shm` never exist and a live `-journal` is rolled back and deleted before the boot-time exclusion runs. The sidecar handling is forward-proofing only; the effective exclusion today is the main DB file. Recommended before/at merge: one on-device check that the xattr actually sticks on the main file (the exclusion call fails silently by design: `if let` + `try?` + `unawaited`). --------- Co-authored-by: Daniel Padrino <danswarrior1@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Move the SQLCipher database from
Documents/toLibrary/Application Support/on iOS. Existing databases are automatically migrated on first launch after update.Why
On iOS,
getApplicationDocumentsDirectory()maps toDocuments/which is included in iCloud backups by default. The encrypted wallet database was therefore uploaded to Apple servers. Combined with the Keychain entry for the DB encryption key (which can also be included in encrypted iCloud backups depending on accessibility settings), this creates a risk if the user's iCloud account is compromised.Library/Application Support/is excluded from iCloud backups by default.Test plan