fix(security): don't silently leave a wallet unencrypted or unlocked (W2-2, W2-4)

P0-B encryption-integrity cluster.

W2-2: the first-run wizard's "encrypt" stored the passphrase only in memory and let the
user into the app immediately, so a quit/crash or a failed daemon connect before the
deferred encryption applied left the wallet unencrypted with NO record encryption was
ever requested — the user believing it was encrypted. A persisted encryption_pending
settings flag is now set the moment encryption is requested (never the passphrase, only
the fact). refreshWalletEncryptionState() reconciles it on every connect: wallet observed
encrypted -> clear the flag; wallet NOT encrypted while the flag is set and no deferred
encryption is pending/in-flight -> a once-per-session "your wallet is NOT encrypted — open
Settings to finish" warning (the flag stays set, so it recurs each launch until resolved).
The passphrase is deliberately never persisted to auto-complete — surfacing it is the
secure choice.

W2-4: lockWallet()'s continuation only handled success — a failed walletlock RPC silently
left the wallet UNLOCKED (an unfulfilled auto-lock). It now logs and warns once (reset on
the next successful lock) so a failing auto-lock is visible instead of leaving the wallet
exposed.

Touches settings.{h,cpp}, app_wizard.cpp, app_security.cpp, app.h. Not unit-testable at
this layer (RPC/connect-driven state). Build-clean; ctest 1/1. See docs/wallet-hardening.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-08-02 14:48:02 -05:00
parent c7d163f44a
commit 8c12b27c0a
6 changed files with 50 additions and 1 deletions

View File

@@ -17,7 +17,7 @@ Status legend: ☐ not started · ◐ in progress · ☑ landed & verified
| Phase | Findings | Theme | Status |
|-------|----------|-------|--------|
| **P0-A** | W7-1, W2-1, W4-1, W4-3, W2-3, W4-5, W5-3 ✓ | Secret hardening (console redaction + delete-export + memzero + lite encrypt-at-create) | ☑ 7/7 |
| **P0-B** | W2-2/W4-2, W2-4 | Encryption integrity (never silently unencrypted) | |
| **P0-B** | W2-2/W4-2, W2-4 | Encryption integrity (never silently unencrypted) | |
| **P1-A** | W3-1, W3-2, W3-4, W3-3 | Migrate-to-seed correctness (fund-adjacent) | ☐ |
| **P1-B** | W1-1, W1-3, W1-2, W1-4 | Missing/wrong wallet-file safety | ☐ |
| **P2** | W6-2, W5-1, W5-2, W6-1, W6-3 | Stale state & lite save-failure surfacing | ☐ |
@@ -112,6 +112,11 @@ Land W7-2 first — it unblocks the rest.
## Progress log
- **P0-B / W2-2 (deferred encryption silently lost) + W2-4 (auto-lock silent-fail)** — ☑ landed:
- **W2-2:** the wizard's deferred encryption was stored only in memory, so a quit/crash or a failed daemon connect before it applied left the wallet unencrypted with **no record it was ever requested** — the user believing it was encrypted. Now a persisted `encryption_pending` settings flag is set the moment encryption is requested (**never the passphrase** — only the fact). `refreshWalletEncryptionState()` reconciles it on every connect: wallet observed **encrypted** → clear the flag; wallet **not** encrypted while the flag is set and no deferred encryption is pending/in-flight → a once-per-session **"your wallet is NOT encrypted — open Settings to finish"** warning (the flag stays set, so it recurs each launch until resolved). We deliberately don't persist the passphrase to auto-complete — surfacing it is the secure choice.
- **W2-4:** `lockWallet()`'s continuation only handled success — a failed `walletlock` silently left the wallet **unlocked** (an unfulfilled auto-lock). It now logs and warns once (reset on the next successful lock), so a failing auto-lock is visible instead of leaving the wallet exposed.
Touches `settings.{h,cpp}`, `app_wizard.cpp`, `app_security.cpp`, `app.h`. Not unit-testable at this layer (RPC/connect-driven state machine); build-clean, `ctest` 1/1.
- **P0-A / W5-3 (lite create-time passphrase)** — ☑ landed (chose option **(b) wire it up**). The lite create/open/restore passphrase was collected but never consumed by the backend — a "passphrase" field that did nothing. It now has a real meaning for all three operations, in `LiteWalletController`: **create/restore**`encryptWallet(passphrase)` (the backend encrypts + locks + saves the brand-new wallet); **open**`unlockWallet(passphrase)`, but only when `encryptionStatus()` reports the existing wallet is actually encrypted+locked (skips a spurious unlock otherwise). Encrypt/unlock take their own copy and wipe it; a post-create encrypt failure is `liteLog`'d (the wallet still exists — the create isn't failed). Six existing lite-controller tests carried an incidental `hunter2` create passphrase from the dead-field era; removed (they test non-encryption flows and want an unencrypted wallet), and added `testLiteWalletControllerCreateEncryptsWithPassphrase` to prove the new behavior. Build-clean; `ctest` 1/1. *(Follow-up UX polish: `settings_page` could show the passphrase field's meaning per operation — "encrypt" for create/restore vs "unlock" for open.)*
- **P0-A / W4-5 (seed-backup file)** — ☑ landed (proportionate): the seed "Save" already wrote 0600 + zeroed the in-memory buffer, but the success message was a bare "Saved to <path>". It now reads "**Saved an UNENCRYPTED seed file — move it to secure offline storage and delete this copy**: <path>", so the plaintext-on-disk risk is called out. `i18n.cpp` (English source; `res/lang` back-fill of this changed key is deferred to the batch i18n pass). A stronger fix (pre-save confirmation, or dropping the file-save in favor of on-screen + Copy) is a follow-up UX decision.
- **P0-A / W4-1 · W4-3 · W2-3 (memzero cluster)** — ☑ landed, using the file's established `sodium_memzero` pattern (matching the existing lambda-capture scrub at app_network.cpp:2885 and JSON scrub at :4025) rather than a new type, since this is fund-moving code: