diff --git a/docs/wallet-hardening.md b/docs/wallet-hardening.md new file mode 100644 index 0000000..e48bf6a --- /dev/null +++ b/docs/wallet-hardening.md @@ -0,0 +1,115 @@ +# 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 (SecureString + console redaction + delete-export) | ◐ | +| **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` 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(/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(/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 / 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.) diff --git a/src/ui/windows/console_tab.cpp b/src/ui/windows/console_tab.cpp index 630fffa..d893de7 100644 --- a/src/ui/windows/console_tab.cpp +++ b/src/ui/windows/console_tab.cpp @@ -1416,8 +1416,11 @@ bool ConsoleTab::submitConsoleCommand(ConsoleCommandExecutor& exec, const std::s { if (cmd.empty()) return false; - addLine("> " + cmd, ConsoleChannel::Command); - AppendConsoleHistory(command_history_, cmd, 100); + // Redact secret-bearing commands (walletpassphrase, z_importkey, …) before they reach the visible + // log and the recall history. The real `cmd` below is still executed unredacted. + const std::string display = RedactConsoleCommand(cmd); + addLine("> " + display, ConsoleChannel::Command); + AppendConsoleHistory(command_history_, display, 100); history_index_ = -1; // First token, lowercased, for built-in interception. diff --git a/src/ui/windows/console_tab_helpers.cpp b/src/ui/windows/console_tab_helpers.cpp index 50228f5..a79b346 100644 --- a/src/ui/windows/console_tab_helpers.cpp +++ b/src/ui/windows/console_tab_helpers.cpp @@ -1,10 +1,34 @@ #include "console_tab_helpers.h" #include +#include namespace dragonx { namespace ui { +namespace { +// First tokens (lowercase) of console/RPC commands that carry a secret argument on the command line. +// Output-secret commands (dumpprivkey / z_exportkey / z_exportmnemonic) are deliberately absent — +// their secret is in the RESULT, which is a separate redaction concern. +const char* const kSecretConsoleCommands[] = { + "walletpassphrase", "walletpassphrasechange", "encryptwallet", + "importprivkey", "importwallet", "importmulti", + "z_importkey", "z_importviewingkey", "z_importwallet", + "signrawtransaction", "magicrecoverkey", "sethdseed", "importmnemonic", +}; + +std::string firstConsoleTokenLower(const std::string& cmd, size_t& tokenEnd) { + size_t b = cmd.find_first_not_of(" \t"); + if (b == std::string::npos) { tokenEnd = cmd.size(); return {}; } + size_t e = cmd.find_first_of(" \t", b); + tokenEnd = (e == std::string::npos) ? cmd.size() : e; + std::string t = cmd.substr(b, tokenEnd - b); + std::transform(t.begin(), t.end(), t.begin(), + [](unsigned char c) { return static_cast(std::tolower(c)); }); + return t; +} +} // namespace + float ComputeConsoleInputHeight(float frameHeightWithSpacing, float itemSpacingY, float spacingSm, @@ -27,5 +51,27 @@ float ClampConsoleWrapWidth(float contentWidth, float paddingX) return std::max(50.0f, contentWidth - paddingX * 2.0f); } +bool ConsoleCommandCarriesSecret(const std::string& cmd) +{ + size_t end = 0; + const std::string name = firstConsoleTokenLower(cmd, end); + if (name.empty()) return false; + for (const char* s : kSecretConsoleCommands) if (name == s) return true; + return false; +} + +std::string RedactConsoleCommand(const std::string& cmd) +{ + size_t end = 0; + const std::string name = firstConsoleTokenLower(cmd, end); + if (name.empty()) return cmd; + bool secret = false; + for (const char* s : kSecretConsoleCommands) if (name == s) { secret = true; break; } + if (!secret) return cmd; + // Only redact if there are actually arguments after the command name. + if (cmd.find_first_not_of(" \t", end) == std::string::npos) return cmd; + return cmd.substr(0, end) + " ****"; +} + } // namespace ui } // namespace dragonx diff --git a/src/ui/windows/console_tab_helpers.h b/src/ui/windows/console_tab_helpers.h index 27f2d13..2cb691c 100644 --- a/src/ui/windows/console_tab_helpers.h +++ b/src/ui/windows/console_tab_helpers.h @@ -1,5 +1,7 @@ #pragma once +#include + namespace dragonx { namespace ui { @@ -14,5 +16,14 @@ float ComputeConsoleOutputHeight(float availableHeight, float minHeightRatio); float ClampConsoleWrapWidth(float contentWidth, float paddingX); +// True if `cmd`'s first token names a console/RPC command that carries a SECRET on its command line +// (passphrase, private/spending/viewing key, mnemonic). Output-secret commands (dumpprivkey, +// z_exportkey, z_exportmnemonic) are NOT covered — their secret is in the result, a separate concern. +bool ConsoleCommandCarriesSecret(const std::string& cmd); + +// A display/history-safe copy of `cmd`: the command name with its arguments replaced by "****" when +// it carries a secret, else `cmd` unchanged. The real command is still executed unredacted. +std::string RedactConsoleCommand(const std::string& cmd); + } // namespace ui } // namespace dragonx diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index 24cbab6..ea93c36 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -2523,6 +2523,29 @@ void testAllowsPlaintextRemote() EXPECT_TRUE(!Connection::usesPlaintextRemote(remoteTls)); // TLS → not plaintext, never refused } +void testConsoleSecretRedaction() +{ + using dragonx::ui::RedactConsoleCommand; + using dragonx::ui::ConsoleCommandCarriesSecret; + + // Secret-bearing commands are recognized (case- and whitespace-insensitive on the name). + EXPECT_TRUE(ConsoleCommandCarriesSecret("walletpassphrase myPass 60")); + EXPECT_TRUE(ConsoleCommandCarriesSecret("z_importkey SK-secret")); + EXPECT_TRUE(ConsoleCommandCarriesSecret(" ENCRYPTWALLET topsecret")); + EXPECT_TRUE(!ConsoleCommandCarriesSecret("getinfo")); + EXPECT_TRUE(!ConsoleCommandCarriesSecret("getwalletinfo")); // not a false-positive substring match + + // Redaction replaces the arguments with **** but preserves the (original-case) command name. + EXPECT_EQ(RedactConsoleCommand("walletpassphrase myPass 60"), std::string("walletpassphrase ****")); + EXPECT_EQ(RedactConsoleCommand("z_importkey SK-secret-key"), std::string("z_importkey ****")); + EXPECT_EQ(RedactConsoleCommand("ENCRYPTWALLET topsecret"), std::string("ENCRYPTWALLET ****")); + // A bare secret command with no argument is left unchanged (nothing to hide). + EXPECT_EQ(RedactConsoleCommand("walletpassphrase"), std::string("walletpassphrase")); + // Non-secret commands pass through untouched. + EXPECT_EQ(RedactConsoleCommand("sendtoaddress addr 1.0"), std::string("sendtoaddress addr 1.0")); + EXPECT_EQ(RedactConsoleCommand("getwalletinfo"), std::string("getwalletinfo")); +} + void testConnectHasStalled() { using dragonx::util::connectHasStalled; @@ -6861,6 +6884,7 @@ int main() testConnectHasStalled(); testIsLocalHost(); testAllowsPlaintextRemote(); + testConsoleSecretRedaction(); testDaemonLifecycleExecution(); testDaemonLifecycleAdapters(); testConsoleTextLayout();