Split PR 578: Crypto hygiene boundaries#609
Open
joshuakrueger-dfx wants to merge 18 commits into
Open
Conversation
5 tasks
Pre-Initiative-IV, deleteWallet only dropped walletAccountInfos rows; the encrypted seed in walletInfos accumulated forever, gated only by the Keychain-stored mnemonic encryption key. Closes F-001 / BL-004: - deleteWallet now removes walletAccountInfos AND walletInfos inside a single transaction, FK order respected. - Returns (accountRows, walletRows) tuple so callers can audit. - countWallets() lets WalletService gate the optional last-wallet Keychain key wipe (opt-in via SettingsRepository). - WalletService.deleteCurrentWallet wires the chain end-to-end and returns mnemonicKeyDeleted in the audit tuple. - HomeBloc cross-refs BL-005 (Initiative I: BitboxService.clear).
…stale-row Tier-0 coverage for the BL-004 cleanup chain — pre-Initiative-IV the walletInfos row survived deleteWallet, accumulating encrypted seeds indefinitely. Tests: test/packages/storage/wallet_storage_test.dart (new) - both walletAccountInfos AND walletInfos rows dropped - countWallets falls to zero on single-wallet delete - delete + recreate-same-seed lands exactly one row (no stale row) - deleteWallet on unknown id returns zero counts (no throw) - sibling wallet untouched (where-clause scoped to walletId) - transaction wrapper isolates concurrent count from partial state test/packages/repository/wallet_repository_test.dart - BL-004 assertion surfaces the typed (accountRows, walletRows) tuple - isLastWallet flips from false to true across the final delete test/packages/service/wallet_service_test.dart - mnemonic key wipe gated on both opt-in flag AND last-wallet-delete - opt-in alone with siblings remaining is a no-op - opted-in last-wallet-delete clears the Keychain key
…(BL-018) Dedicated isolate spawned at app start; owns the only String representation of the decoded mnemonic + the BIP32 root. The main isolate marshals only typed requests/responses over a SendPort: - _UnlockRequest / _AdoptPlaintextRequest seat a slot; the response is the primary address (the only main-side leakage permitted). - _DeriveAddressRequest / _SignDigestRequest / _SignPersonalMessageRequest run derivation + signing inside the isolate; response is bytes or signature components only. - _RevealRequest is the explicit, Law-6-scoped reveal flow used by verify-seed + settings-seed cubits; the holder must dispose the returned string after rendering. - _CancelRequest is the typed propagation path replacing Future.ignore() — lockCurrentWallet wires it in BL-022. - _LockRequest drops the slot and best-effort overwrites the mnemonic string + reassigns the BIP32 root to a zero-seed tree so a heap walk pre-GC observes the dummy in the field's old position. Long-lived single-isolate shape was preferred over per-sign spawn (ADR 0004 §"Alternatives"): per-sign spawn would pay ~60ms per round, totaling ~780ms across a 13-page EIP-712 ceremony. The single isolate keeps IPC overhead in the ~5ms range, within the mandate's 200ms threshold.
…late
The handle now carries only (id, name, address, isolate) — the BIP39
mnemonic field is gone. Sign + derive run through WalletIsolate over
the typed IPC channel landed in the previous commit. Display flows
(verify-seed quiz, settings-seed reveal, onboarding) use a transient
SeedDraft holder whose lifetime is bound to the cubit that produced
it; lifecycle observers dispose the draft on hidden so the seed
doesn't survive an iOS app-suspend snapshot.
Changes:
- SoftwareWallet: constructor takes WalletIsolate handle; signMessage
routes through the isolate's signPersonalMessage path.
- _IsolateCredentials: async-only credentials (the sync sign methods
throw UnsupportedError — the boundary is fundamentally async).
- WalletAccount: now an isolate-backed account; the legacy
BIP32-rooted class is gone from wallet_account.dart.
- SeedDraft: transient main-isolate holder for the brief reveal +
onboarding windows; dispose() best-effort-zeroizes the string.
- WalletService:
* generateUncommittedSeedDraft replaces generateUncommittedSeedWallet
* commitGeneratedWallet(SeedDraft) persists + adopts plaintext
into the isolate + disposes the draft
* restoreWallet adopts plaintext after persist
* unlockWalletById round-trips ciphertext + key through the isolate
* revealCurrentSeed returns a SeedDraft from the isolate (used by
settings-seed flow)
* deleteCurrentWallet drops the isolate slot before the row goes
* lockCurrentWallet locks the isolate slot before flipping AppStore
- CreateWalletCubit: state holds SeedDraft; lifecycle hidden disposes.
- VerifySeedCubit: takes SeedDraft (not SoftwareWallet); now a
WidgetsBindingObserver — hidden/paused disposes the draft and emits
VerifySeedAborted (BL-023 wiring).
- SettingsSeedCubit: revealCurrentSeed + lifecycle observer; wipes
rendered seed on hidden.
- WalletIsolate: adds forTesting() constructor so test doubles can
subclass without spawning a real isolate.
The pre-Initiative-IV warmAuthSignature path on the create flow is
dropped — the lazy path in DFXAuthService.getSignature is the safety
net and runs once the wallet is committed (and the seed lives in the
isolate). Documented in CreateWalletCubit's constructor comment.
Tier-0 end-to-end tests for the WalletIsolate (BL-018). Every test spawns a real isolate per group — no Dart-side mocks of the IPC channel; the mandate is explicit that cryptographic-boundary tests exercise the production path. - adoptPlaintext yields the canonical Hardhat-zero address for the test mnemonic, pinning the BIP-44 derivation semantics. - deriveAddress for account 1 differs from account 0 (no path collisions) and errors out as NotUnlocked without a slot. - signPersonalMessage returns the expected 65-byte signature, is deterministic across re-invocations, tolerates non-ASCII payloads (RealUnitCH#289 regression), and the signature ec-recovers to the same address the adopt returned. - signDigest returns valid (r, s, v) tuples; v fits the chain-id encoding. - unlock from a SecureStorage-shaped ciphertext (AES-GCM/128) decrypts inside the isolate and yields the same address as the plaintext adopt path. - reveal round-trips the mnemonic back to the main isolate (the Law-6-scoped settings_seed + verify_seed flow); reveal without a slot is NotUnlocked. - lock() drops the slot; a subsequent reveal is NotUnlocked. lock on an absent slot is a defensive no-op; re-adopt after lock seats a fresh slot with the same address. - dispose() rejects further requests and is idempotent.
…ot drop Tier-0 test rewrite for the post-Initiative-IV WalletService API: - generateUncommittedSeedDraft returns a SeedDraft, validates as bip39, does not touch the repository, produces distinct mnemonics across calls. - commitGeneratedWallet adopts the plaintext into the isolate exactly once, returns a SoftwareWallet handle with the DB-assigned id + the isolate-derived address, disposes the draft, throws on a pre-disposed draft. - restoreWallet adopts plaintext + persists + marks current. - unlockWalletById round-trips the ciphertext + key through the isolate (BL-018 contract). - revealCurrentSeed returns an undisposed SeedDraft with the isolate's mnemonic (settings-seed reveal flow). - BL-022: lockCurrentWallet drops the isolate slot in addition to flipping AppStore — the decrypted seed is released even if the awaiting future is never observed. A lock during a single in-flight unlock leaves the AppStore on the view wallet and never writes the SoftwareWallet handle after the lock. - BL-004 chain: deleteCurrentWallet drops the isolate slot before the row goes, surfaces the row counts, gates the mnemonic-key wipe on (opt-in AND last-wallet). test/test_utils/fake_wallet_isolate.dart introduces a subclass of WalletIsolate built via the new forTesting() constructor — minimal shape, only the methods exercised by the cubits + services. Tests that need deeper IPC fidelity use a real WalletIsolate.spawn().
Tier-0 lifecycle tests for VerifySeedCubit's WidgetsBindingObserver hookup (landed in the SoftwareWallet handle-pattern commit). The observer fires on AppLifecycleState.hidden + paused; both transitions must dispose the SeedDraft and emit aborted within one pumpFrame so the mnemonic does not appear in the iOS app-suspend snapshot. Tests added: - hidden mid-verify disposes the draft and flips aborted true - paused (some platforms emit both) is handled identically - verify on an aborted cubit short-circuits without commit - close() disposes the draft even without a lifecycle event The existing happy-path verify tests are migrated to the new SeedDraft input shape; the committed wallet returned by commitGeneratedWallet is now constructed via a FakeWalletIsolate so the test does not spawn a real isolate.
…(BL-045) Post-Initiative-IV the PIN-hash iteration count is OWASP-2025's recommended 600,000 for PBKDF2-HMAC-SHA256. Transparent rehash for the 250k / 100k legacies; 10k is explicitly rejected (a user landing on 10k is force-reset out of the app rather than transparently upgraded, because an attacker may already have brute-forced the hash on a leaked DB snapshot). Rehash atomicity (ADR 0004 §"Rehash atomicity"): a single secure storage write per unlock — there is only one `pin.hash` entry, the rehash overwrites it. If the write fails the old legacy hash remains and the next unlock takes the same path. Also lands BL-050 — IOSOptions(accessibility: first_unlock_this_device) + AndroidOptions(encryptedSharedPreferences: true). The constants are exposed as `iosOptions` / `androidOptions` so the snapshot test in the next commit can pin the configuration; a refactor that drops the constraints will fail the test. Public exports added for the snapshot test: - currentIterations, legacyIterationCandidates, rejectedIterationCandidates
…rent rehash Tier-0 tests for the BL-045 PIN-iteration policy + BL-050 secure-storage options snapshot. Tests: PIN-iteration policy (BL-045): - the current iteration count is OWASP-2025 600k (snapshot trip) - the legacy acceptance set contains 250k and 100k - 10k is explicitly REJECTED (in rejectedIterationCandidates), NOT in legacyIterationCandidates — a user on 10k is force-reset - 600k / 250k / 10k produce distinct hashes for the same pin+salt (otherwise the legacy-detection branch would be dead code) - 600k hashing is deterministic for the same pin+salt flutter_secure_storage options snapshot (BL-050): - iosOptions.toMap() pins 'accessibility' == 'first_unlock_this_device' (iCloud backup-restore to a new device is blocked) - androidOptions.toMap() pins 'encryptedSharedPreferences' == 'true' (AES-256-GCM bound to the Android Keystore) The full instance-level verifyPin round trip requires FlutterSecureStorage platform-channel scaffolding that isn't worth threading through a unit test; the rehash atomicity contract is already documented in the implementation and the snapshot above prevents the legacy set from being silently widened.
…etryAny (BL-049) Replaces the plain-bool return of BiometricService.authenticate with a typed BiometricAuthResult that carries the unwrapped CryptoObject sentinel alongside the legacy success flag. Sensitive operations gate on the unwrappedSecret field; a patched-return-true `local_auth` on a rooted device produces success=true but unwrappedSecret=null, and the PIN-reset / settings-mnemonic-reveal flows refuse the operation. The Dart-side sentinel is the bridge to the native CryptoObject binding scheduled for the platform follow-up: - Android: BiometricPrompt.CryptoObject wraps an AndroidKeyStore key created with setUserAuthenticationRequired(true) + BIOMETRIC_STRONG. The key cannot be used outside a successful biometric prompt. - iOS: a SecKey with kSecAttrAccessControlBiometryAny stored in the Keychain. Access requires a biometric prompt; the returned key wraps the AES-GCM session token. ADR 0004 §"Biometric CryptoObject binding" documents the BiometryAny trade-off (re-enrol UX vs. security gain). Callers updated: - verify_pin_cubit: gates on (success && unwrappedSecret != null) - SecureStorage.readBiometricCryptoSentinel / writeBiometricCryptoSentinel: surface the sentinel store - legacy authenticate() callers can switch to authenticateBoolean() for the shim; new callers route through the typed result.
flutter_test extension that walks a caller-supplied set of roots (and their toString() projection) and asserts no 12-word contiguous BIP39 sequence is reachable. The roots are the realistic exposure surface for an integration test — AppStore, WalletService handle, cubit states, rendered widget tree — not a full VM-level heap walk, which would require the VM service protocol dependency. API: await expectNoBip39SequenceInHeap([appStore, walletService, ...]); Implementation: - Tokenises the projected string on any non-letter run, so url-encoded payloads and base64 blobs can't glue dictionary words together. - Case-insensitive — a capitalised mnemonic in a UI label still trips. - Awaits WidgetsBinding.instance.endOfFrame before scanning so a still-rendering widget tree doesn't race the probe. - A 12-word window is the failure threshold (any 12 contiguous dictionary words, not a checksumed mnemonic) — false positives here are tolerable, false negatives are not. Unit tests pin: - Empty input + < 12 tokens return null. - 12 contiguous dictionary words hit. - A near-miss (11 + 1 non-dictionary) does NOT hit. - Mid-buffer detection across multiple windows. - Aggressive tokenisation defeats url-glued mnemonics. - Case-insensitive detection.
Tier-1 integration test for the Initiative IV contract. Drives a
real create -> sign -> background -> foreground -> sign sequence
through the production WalletService + a real WalletIsolate, with
heap-probe checkpoints at each transition:
1. Post-create: SoftwareWallet handle + AppStore must not carry
the seed.
2. Post-sign: neither the 65-byte signature nor any intermediate
should have surfaced the mnemonic.
3. Post-background: lockCurrentWallet flips AppStore to a view
wallet AND drops the isolate slot — no seed reachable.
4. Post-foreground-sign: ensureCurrentWalletUnlocked re-seats the
isolate slot from the encrypted DB row; a second sign succeeds
without bringing the seed back onto the main isolate.
The probe is the harness from test_utils/heap_probe.dart — it scans
the toString() projection of the supplied roots for any 12-word
BIP39 sequence and fails if one is found. The encrypted-seed
fixture is built inline with the same AES-GCM/128 shape
SecureStorage.encryptSeed emits so the WalletIsolate's unlock path
exercises the real decrypt branch.
…quence
Tier-1 integration test for the BL-004 cleanup chain. Drives the
WalletService.deleteCurrentWallet path against the real Drift
database (in-memory) + a real WalletRepository, exercising:
- Create three wallets via restoreWallet; assert each gets a
distinct id and walletInfos count climbs to 3.
- Delete each one in turn; assert both walletAccountInfos AND
walletInfos rows are gone (the F-001 fix from Initiative IV).
- The mnemonic encryption key is wiped only on the LAST delete
when the opt-in flag is set; every earlier delete leaves it
alone.
- With opt-in disabled, the key survives even after the last
delete (the conservative default).
- Property test: walletInfos count after a mixed
create-5/delete-2/create-2/delete-5 sequence is exactly 0.
Mechanical migration of test files that constructed SoftwareWallet via
the legacy 3-arg seed-bearing constructor. Each call site is now:
SoftwareWallet(
id,
name,
'0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266',
FakeWalletIsolate(),
)
The FakeWalletIsolate (test/test_utils/fake_wallet_isolate.dart)
satisfies the handle constructor without spawning a real isolate —
the cubit-level tests don't exercise the IPC, only the handle shape.
Other migrations folded in:
- CreateWalletState.wallet -> CreateWalletState.draft
- VerifySeedPage(wallet:) -> VerifySeedPage(draft:)
- BiometricService.authenticate() now returns BiometricAuthResult;
the verify_pin_cubit_test exercises the BL-049 contract via
BiometricAuthResult.forTesting(success: true, unwrappedSecret: null)
to pin "patched-return-true is refused".
- SettingsSeedCubit + VerifySeedCubit add WidgetsBindingObserver;
cubit-test bindings now call TestWidgetsFlutterBinding.ensureInitialized
to satisfy the WidgetsBinding.instance access.
A `BiometricAuthResult.forTesting()` constructor is added to the
production class so unit tests can construct the success-without-
unwrap shape. Marked with the forTesting convention so the next
grep audit surfaces unauthorised production usage.
…backfill Two paths left the decrypted mnemonic resident in the WalletIsolate: - lockCurrentWallet only nulled _unlockInFlight; the in-flight unlock then seated the seed in the isolate AFTER the lock, and _lockWalletInPlace no-ops while AppStore.wallet is still a view wallet, so isolate.lock was never reached (the documented WalletIsolate.cancel() mitigation is never called). Now schedule a slot-drop on the in-flight unlock's completion. - getWalletById's legacy empty-address backfill returned a full unlocked SoftwareWallet outside the holder/timer accounting (no auto-lock cap). It only needs the address: now drops the slot and returns a view wallet. Tests: tightened the in-flight test to assert the isolate slot is dropped, and the legacy test to assert a view wallet + no resident seed (both red before fix).
TaprootFreak
force-pushed
the
split/pr578-04-crypto-hygiene
branch
from
4bd6d75 to
e9dc7c4
Compare
TaprootFreak
added
the
tier3:full
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.
Draft split from #578 after TaprootFreak review.
Scope:
Validation: