Files

DanS 7e4822c021 fix(security): warn that the seed-backup file is unencrypted plaintext (W4-5)

The seed-phrase "Save" already wrote the file 0600 and zeroed the in-memory buffer, but
the success message was a bare "Saved to <path>" — no hint that it's a permanent
UNENCRYPTED copy of the seed at a predictable location. The message now reads
"Saved an UNENCRYPTED seed file — move it to secure offline storage and delete this
copy: <path>". English source updated; the res/lang back-fill of this changed key is
deferred to the batch i18n pass.

Also documents W5-3 (lite create-time passphrase) as a product decision rather than a
speculative change: the field is already wiped on every path (minimal security risk),
but the labeled masked "passphrase" input at lite create/open/restore is never consumed
by the backend — so either remove the dead UI or wire it into the lite encrypt flow.

Finishes the actionable part of the wallet-hardening P0-A cluster (docs/wallet-hardening.md).
Build-clean; ctest 1/1.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

2026-08-02 14:25:15 -05:00

11 KiB

Raw Blame History

Wallet Loading & Management — Hardening Plan

Prioritized, grouped remediation for the wallet loading/management audit (33 verified findings + diagnosability QoL). Companion to the findings artifact. Line references are against dev.

Provenance: 7 parallel subsystem finders, each finding adversarially verified against the code; the 3 highest-impact confirmed findings re-checked by hand. 32 confirmed, 1 refuted (W1-5), 1 raised (W5-3 Low→Med).
Severity: 8 High · 12 Medium · 13 Low.

Status legend: ☐ not started · ◐ in progress · ☑ landed & verified

Roadmap (ordered by risk; shared fixes grouped)

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)	◐ 6/7
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	☐
F	W7-2, W7-3, W7-4, QoL	Diagnostics foundation + QoL bundle	☐

P0-A — Secret hardening

Shared fix: a SecureString RAII buffer (zeroes on destruction) retrofitted onto the un-scrubbed key/passphrase paths, plus console redaction and deleting the plaintext export.

W7-1 (High) console_tab.cpp:1419 — RPC console echoes/stores/clipboards raw secrets. Fix: an allowlist of secret-bearing first-tokens (walletpassphrase, walletpassphrasechange, encryptwallet, importprivkey, importwallet, z_importkey, z_importviewingkey, signrawtransaction, magicrecoverkey, lite equivalents); echo > walletpassphrase **** and keep the raw text out of command_history_. Extract a pure redactConsoleCommand(cmd) helper for unit testing. ← implementing first (self-contained + testable).
W2-1 (High) wallet_security_workflow.cpp:66 — delete the obsidiandecryptexport<ts> plaintext key dump after z_importwallet succeeds (overwrite-then-unlink).
W4-3 (High) app_network.cpp:4481 — sodium_memzero the concatenated all-keys string in exportAllKeys; write the backup 0600. (Also unify with ExportAllKeysDialog — QoL.)
W4-1 (High) app_network.cpp:3801 — zero the key copies in importPrivateKey/sweepPrivateKey (local + worker-lambda copies).
W2-3 (Med) app_security.cpp:1481 — zero the passphrase threaded through the decrypt lambda chain.
W4-5 (Med) app.cpp:3577 — the seed-backup .txt is a permanent predictable cleartext seed; at minimum warn + offer to delete, ideally discourage file save in favor of the on-screen phrase.
W5-3 (Med) lite_wallet_lifecycle_service.cpp:322 — remove the dead passphrase field from the lite create/open/restore requests (unused; a secret copied for nothing).

P0-B — Encryption integrity

W2-2 / W4-2 (High) wallet_security_controller.h:89 — the wizard's deferred encryption is in-memory only and silently lost if the daemon doesn't connect or the app quits/crashes first, so a wallet the user believes is encrypted stays plaintext. Fix: persist a lightweight encryption_requested_but_incomplete settings flag (NEVER the passphrase) when beginDeferredEncryption is called; surface a persistent warning banner while it's set; clear it only on confirmed encryptwallet success; on next connect, if set, re-prompt for the passphrase to complete it.
W2-4 (Med) app_security.cpp:480 — lockWallet only sets locked on RPC success; log the failure and notify (currently a silent no-op that can leave the wallet unlocked).

P1-A — Migrate-to-seed correctness (fund-adjacent; verify carefully)

W3-1 (High) app_network.cpp:4327 — adopt hardcodes datadir + "/wallet.dat"; use settings_->getActiveWalletFile() so migrating a non-default active wallet swaps the right file.
W3-2 (High) seed_wallet_creator.cpp:57 — remove_all(<config>/seed-migrate) unconditionally at Phase-1 start; refuse to wipe if a temp DRAGONX/wallet.dat already exists (a prior un-adopted swept wallet) and surface it, so swept funds in the temp wallet can't be destroyed by re-entry.
W3-4 (Med) app_network.cpp:1124 — block wallet switching while a migration is pending (getSeedMigrationPending()), not only while the dialog is open.
W3-3 (Med) app_network.cpp:4231 — persist the sweep opid so an app-close mid-Sweeping can resume/re-poll it instead of silently dropping the txid.

P1-B — Missing/wrong wallet-file safety

W1-1 (High) app_network.cpp:1109 — fs::exists()-check the target wallet file in switchToWallet() and before the first daemon launch at startup; if missing, block with an explicit "Wallet file not found — moved or deleted?" dialog (browse / create-new) instead of letting the daemon fabricate an empty wallet.
W1-3 (Med) app_network.cpp:1095 — defer the syncedHere=true stamp to the first successful address/balance readback (idHash non-empty), not bare onConnected().
W1-2 (Med) app_network.cpp:198 — split DB_CORRUPT-specific strings from the generic "Error loading wallet" fallback; give DB_TOO_NEW its own message/action (not a salvage offer).
W1-4 (Low) wallets_dialog.h:393 — re-fs::exists() the in-datadir row before switching (match the out-of-datadir path).

P2 — State & lite persistence

W6-2 (Med) network_refresh_service.cpp:1183 — record a per-field last-success timestamp / a "refresh failed" flag so the UI can show a staleness badge instead of last-good-as-current.
W5-1 / W5-2 (Med) lite_wallet_controller.cpp:78,603 — liteLog() the failed save and bubble a one-shot UI warning (both call sites currently discard the bool).
W6-1 (Med) wallet_state.h:313 — reset mining/pool_mining in clear() (or comment why not).
W6-3 (Low) address_book.cpp:46 — per-entry try/catch: skip + count malformed entries instead of discarding the whole list.

F — Diagnostics foundation + QoL

Land W7-2 first — it unblocks the rest.

W7-2 (Med) logger.cpp:31 — call Logger::instance().init(<config>/dragonx-debug.log) early in main() on all platforms; add an "Open log folder" action.
W7-3 (Med) main.cpp:144 — add a sigaction-based crash handler writing dragonx-crash.log on POSIX (mirror the Windows SEH path).
W7-4 (Low) logger.cpp:39 — size-cap/rotate the log on init().
QoL — "Copy diagnostics for support" bundle; persistent alert history; daemon/RPC error banner; refresh-staleness badge; multi-wallet diagnostic panel; refresh-diagnostics panel; structured switch/migration audit logging; restore-from-seed entry point (W4-4, effort L).

Progress log

P0-A / W5-3 (lite create-time passphrase) — ⚑ needs a product decision, not a speculative code change. Investigation revised the finding: the passphrase field is already secureWipeLiteSecret'd on every path (controller:1175/1183/1192, settings_page:295), so the security exposure is minimal. The real issue is a misleading UI: settings_page.cpp:1733-1739 renders a labeled, masked "passphrase" InputText at lite create/open/restore, but the backend initialize* calls (lifecycle_service:326/334/342) never consume it (lite encryption is a separate post-open encrypt flow). A user may believe their lite wallet is passphrase-protected at creation when it isn't. Two options, both product calls: (a) remove the dead field + its UI (settings_page 121/272/276/281/295/1733-1740, the request passphrase fields, the redaction/wipe refs), or (b) wire it into the lite encrypt flow so it actually protects the new wallet. Not changed unilaterally.
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 ". It now reads "Saved an UNENCRYPTED seed file — move it to secure offline storage and delete this copy: ", 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:
- W4-1 importPrivateKey/sweepPrivateKey: the spending/viewing key is now scrubbed on all paths — the calling-frame copy (after the worker post), the worker-lambda's captured copy (lambda made mutable, zeroed after the request is sent), and the JSON request params copy.
- W4-3 exportAllKeys/backupWallet: the concatenated all-keys buffer is zeroed after the consumer uses it, and the backup file is now written via Platform::writeFileAtomically(..., restrictPermissions=true) (atomic + 0600) instead of a umask-default ofstream.
- W2-3 decrypt-wallet passphrase: std::move-captured into the worker lambda (so no plaintext copy is left in the calling frame) and sodium_memzero'd right after unlockWallet (its only use). Not unit-testable (the scrubbing has no observable RPC effect — the key value sent to the daemon is unchanged; only post-use memory zeroing is added). Build-clean; ctest 1/1 (no regression). Remaining in P0-A: W5-3 (remove the dead lite passphrase field), W4-5 (predictable plaintext seed-backup file).
P0-A / W2-1 — ☑ landed: the decrypt-wallet flow now scrubs (best-effort in-place zero-overwrite) and removes the plaintext key export (obsidiandecryptexport…) as soon as the z_importwallet attempt resolves — success or failure — so a full cleartext dump of every private key is no longer left on disk forever. Recovery remains the encrypted backup (wallet.dat.encrypted.bak). app_security.cpp (after the import call). Not unit-testable (fs I/O in a deep lambda); build-clean, ctest 1/1 (no regression).
P0-A / W7-1 — ☑ landed: RedactConsoleCommand/ConsoleCommandCarriesSecret in console_tab_helpers redact secret-bearing commands (an allowlist of 13 first-tokens: walletpassphrase, encryptwallet, z_importkey, …) to > walletpassphrase **** before they hit the console echo AND the recall history; the real command still executes unredacted. Wired into submitConsoleCommand (console_tab.cpp). New testConsoleSecretRedaction (11 assertions). Clean build; ctest 1/1. (Output-secret commands like z_exportkey — result redaction — remain a follow-up.)

11 KiB Raw Blame History