fix(kotlin-sdk): degrade mnemonic storage off lock binding on false-locked devices - #4643
fix(kotlin-sdk): degrade mnemonic storage off lock binding on false-locked devices#4643HashEngineering wants to merge 1 commit into
Conversation
…ocked devices Some OEM builds (HONOR/MagicOS Android 16 in the field; same mechanism as Google Issue Tracker 506989112 on Fairphone) perform unlocks that never satisfy the Keystore's UNLOCKED_DEVICE_REQUIRED gate, so the lock-bound master-alias key stays denied for the whole unlock session while KeyguardManager reports the device unlocked. storeMnemonic's bounded false-locked retry (built for the transient Keystore2 blip) can never outwait that, so wallet creation was unfixably failing on those devices. Add a last rung to the ladder: when the retry schedule exhausts still false-locked, treat the device's UNLOCKED_DEVICE_REQUIRED implementation as defective and store under a new never-lock-bound alias (MASTER_ALIAS_UNBOUND — same hardware-backed non-auth AES-256-GCM, no setUnlockedDeviceRequired ever), recording the defect durably in the same atomic edit. From then on mnemonic writes go straight to the unbound alias, the createWallet preflight stops probing, reads route by the blob's recorded alias (mnemonicalias.<walletIdHex>, the privkeyalias discipline), and pre-existing lock-bound blobs are re-wrapped best-effort on their first successful read. Nothing is ever deleted or re-keyed, genuinely-locked denials keep failing fast, the auth-gated identity aliases are untouched, and healthy devices never provision the new alias — this is the #4060 no-lock-screen downgrade driven by operational evidence instead of a missing lock screen. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughWallet mnemonic storage now detects persistent false-locked Keystore failures, stores affected mnemonics with a never-lock-bound alias, records the producing alias, and rewraps legacy blobs when possible. Tests cover retries, fallback behavior, reads, failures, and buffer scrubbing. ChangesFalse-locked Keystore handling
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The change restores wallet creation on affected OEM devices while preserving locked-device failures and healthy-device behavior. The expanded Kotlin tests pass, and no merge-blocking risk is identified. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
HashEngineering
changed the title
|
⛔ Final review complete — 1 blocking finding(s) (commit c574e29) · triage: critical · Phase 2 only (queue backlog) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 2 only (queue backlog)
The fallback alias and durable defect marker are implemented coherently, and the retry/degradation paths have substantial coverage. However, the opportunistic re-wrap adds an unsynchronized write to the read path, allowing a deleted or concurrently replaced mnemonic to be restored. The change also modifies a public non-suspending method into a suspending method despite the PR declaring that there are no breaking changes; cancellation during re-wrap additionally leaves plaintext unsanitized.
Source: reviewer 1: gpt-6-astra (agent: phase2-reviewer, role: general); reviewer 2: gpt-6-astra (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6-astra (agent: astra-verifier, role: final-verifier)
Review provenance
- Triage:
criticalbygpt-6-astra(effort low) — This is a substantial security-sensitive storage and cryptographic key-management change that alters mnemonic encryption, alias selection, durable migration state, atomic persistence, and retry behavior, where regressions could affect wallet availability or seed protection. - Phase 1 reviewers: not run (skipped for throughput: 17 PRs queued, above the 10 limit)
- Fresh verifier:
gpt-6-astra— final-verifier; agentastra-verifier - Phase 2 reviewers:
gpt-6-astra— general (completed, effort xhigh); agentphase2-reviewer,gpt-6-astra— security-auditor (completed, effort xhigh); agentphase2-reviewer
🔴 1 blocking | 🟡 2 suggestion(s)
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt`:
- [BLOCKING] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:481-483: Re-wrap can resurrect a mnemonic after deletion
`retrieveMnemonicUtf8` reads the mnemonic and alias from one DataStore snapshot, decrypts the plaintext, and then performs a separate `store.edit` through `rewrapMnemonicUnbound`. If `deleteMnemonic(walletId)` completes after the snapshot/decrypt but before that edit, the re-wrap writes the old ciphertext and alias back into the store, resurrecting a mnemonic that was just deleted. The same race can overwrite a newer mnemonic written concurrently. The atomicity of the re-wrap edit does not protect the read-to-write interval because the edit is not conditional on the original blob and alias still being present. Serialize re-wraps with mnemonic writes/deletes or perform a compare-and-set edit that only replaces the entry when the original encoded blob and alias still match.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:481-484: Cancellation during re-wrap can leave decrypted mnemonic bytes unsanitized
On the defect path, `plain` is decrypted and passed to `rewrapMnemonicUnbound` before ownership is returned to the caller. That function deliberately rethrows `CancellationException`; if cancellation occurs during the suspending `store.edit`, `retrieveMnemonicUtf8` exits without returning `plain`, so the caller cannot scrub it. The same ownership problem applies to an unexpected throwable from the re-wrap. Clear the plaintext buffer before propagating any re-wrap failure that prevents returning it, while preserving the existing best-effort behavior for ordinary re-wrap errors.
- [SUGGESTION] packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/security/WalletStorage.kt:250: Changing this public method to suspend is a breaking API change
`WalletStorage` is a public class and `ensureMasterKeyNotLockBlocked` is declared as a public method without an `internal` modifier. The base API exposed a regular `fun`; changing it to `suspend fun` requires source callers to be inside a coroutine and changes the generated JVM-facing method shape. The PR's assertion that the change is internal and has no external callers is not sufficient to preserve compatibility for consumers that access the public `WalletStorage` API. Preserve the existing public method with a blocking-free wrapper/alternative, introduce a separate suspending API while retaining the old signature, or explicitly treat and document this as a breaking API change.
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | ||
| rewrapMnemonicUnbound(walletId, plain) | ||
| } |
There was a problem hiding this comment.
🔴 Blocking: Re-wrap can resurrect a mnemonic after deletion
retrieveMnemonicUtf8 reads the mnemonic and alias from one DataStore snapshot, decrypts the plaintext, and then performs a separate store.edit through rewrapMnemonicUnbound. If deleteMnemonic(walletId) completes after the snapshot/decrypt but before that edit, the re-wrap writes the old ciphertext and alias back into the store, resurrecting a mnemonic that was just deleted. The same race can overwrite a newer mnemonic written concurrently. The atomicity of the re-wrap edit does not protect the read-to-write interval because the edit is not conditional on the original blob and alias still being present. Serialize re-wraps with mnemonic writes/deletes or perform a compare-and-set edit that only replaces the entry when the original encoded blob and alias still match.
source: ['claude']
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | ||
| rewrapMnemonicUnbound(walletId, plain) | ||
| } | ||
| return plain |
There was a problem hiding this comment.
🟡 Suggestion: Cancellation during re-wrap can leave decrypted mnemonic bytes unsanitized
On the defect path, plain is decrypted and passed to rewrapMnemonicUnbound before ownership is returned to the caller. That function deliberately rethrows CancellationException; if cancellation occurs during the suspending store.edit, retrieveMnemonicUtf8 exits without returning plain, so the caller cannot scrub it. The same ownership problem applies to an unexpected throwable from the re-wrap. Clear the plaintext buffer before propagating any re-wrap failure that prevents returning it, while preserving the existing best-effort behavior for ordinary re-wrap errors.
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | |
| rewrapMnemonicUnbound(walletId, plain) | |
| } | |
| return plain | |
| if (alias == KeystoreManager.MASTER_ALIAS && prefs[MASTER_LOCK_DEFECT_KEY] == true) { | |
| try { | |
| rewrapMnemonicUnbound(walletId, plain) | |
| } catch (t: Throwable) { | |
| plain.fill(0) | |
| throw t | |
| } | |
| } |
source: ['claude']
| * no lock state can deny, so there is nothing to preflight. | ||
| */ | ||
| fun ensureMasterKeyNotLockBlocked(operation: String) { | ||
| suspend fun ensureMasterKeyNotLockBlocked(operation: String) { |
There was a problem hiding this comment.
🟡 Suggestion: Changing this public method to suspend is a breaking API change
WalletStorage is a public class and ensureMasterKeyNotLockBlocked is declared as a public method without an internal modifier. The base API exposed a regular fun; changing it to suspend fun requires source callers to be inside a coroutine and changes the generated JVM-facing method shape. The PR's assertion that the change is internal and has no external callers is not sufficient to preserve compatibility for consumers that access the public WalletStorage API. Preserve the existing public method with a blocking-free wrapper/alternative, introduce a separate suspending API while retaining the old signature, or explicitly treat and document this as a breaking API change.
source: ['claude']
2 participants
Issue being fixed or feature implemented
Wallet creation fails permanently on some OEM Android builds, and no amount of retrying fixes it.
Those builds perform unlocks that never satisfy the Keystore's
UNLOCKED_DEVICE_REQUIREDgate, so a key created withsetUnlockedDeviceRequired(true)stays denied for the entire unlock session — whileKeyguardManagercheerfully reports the device unlocked.WalletStorage.storeMnemonic's bounded false-locked retry was built for the transient Keystore2 blip and cannot outwait a device that is simply never going to satisfy the gate, socreateWalletfails and the wallet is unusable.Same mechanism as Google Issue Tracker 506989112 (reported there on Fairphone). Related: #4060, which added a no-lock-screen downgrade — this is the same downgrade, driven by operational evidence rather than by the absence of a lock screen.
Observed in the field on a HONOR PTP-N49 (MagicOS, Android 16, security patch 2026-07-01): 23 consecutive
KeystoreDeviceLockedExceptiondenials on'encrypt'/'createWallet'against the lock-bound master alias across a full day, and not one successful bind.What was done?
Added a last rung to the existing retry ladder in
WalletStorage: when the false-locked retry schedule exhausts and the device is still false-locked, treat that device'sUNLOCKED_DEVICE_REQUIREDimplementation as defective and stop relying on it.KeystoreManager.MASTER_ALIAS_UNBOUND(org.dashfoundation.wallet.master.unbound) — same hardware-backed, non-auth AES-256-GCM key as the master alias, butsetUnlockedDeviceRequiredis never applied to it.WalletStorage.healFalseLockedMnemonicStore/storeMnemonicUnbound— on exhaustion, store under the unbound alias and record the defect durably in the same atomic edit, so a crash cannot leave the two disagreeing.mnemonicalias.<walletIdHex>tag, following the existingprivkeyaliasdiscipline. No global "which alias are we on" flag to get out of step.WalletStorage.rewrapMnemonicUnbound— pre-existing lock-bound blobs are re-wrapped onto the unbound alias best-effort on their first successful read. A failed re-wrap leaves the blob readable where it is.PlatformWalletManager.ensureMasterKeyNotLockBlockedbecamesuspendand stops probing once the defect is on record;isMasterKeyLockBindingDefectObserved()exposes the state.What this deliberately does not do:
How Has This Been Tested?
packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/security/WalletStorageDeviceLockedRetryTest.kt— extended to 17 tests covering the pre-check matrix (genuinely locked, unlocked, keyguard-showing-but-not-secure, locked-but-not-lock-bound, locked-with-defect-on-record), the retry path, and the new degradation: exhaustion → unbound alias, straight-to-unbound once recorded, original denial propagated when the degradation encrypt also fails, re-wrap on first successful read, and blob still readable when re-wrap fails. Four tests assert the mnemonic buffer is scrubbed on every exit — success, degraded store, final denial, and cancellation during retry backoff.824 tests, 0 failures (JDK 17 required; the module does not build on 11).
Device-verified on the affected hardware. Same HONOR PTP-N49, same starting state (
restored 0 wallet(s), socreateWallet→encrypton the lock-bound master alias):KeystoreDeviceLockedException, 07:07→18:43bound toline in the logapp wallet bound to new SDK wallet~1 s after SDK startTwo honest limits on that evidence:
dash-sdk.db, because the pre-change runs never got far enough to create the store), so the decrypt path on an existing lock-bound alias — and therefore the re-wrap-on-first-read path — is covered by unit tests only.Emulators cannot reproduce the defect at all: AOSP classifies every denial as genuinely locked, so the false-locked branch is unreachable there.
Breaking Changes
None.
PlatformWalletManager.ensureMasterKeyNotLockBlockedbecamesuspend, but it is internal to the SDK and has no external callers.Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Documentation
Tests