From 6b0f2eda22b4876ee408e9d617baaf4c991388e7 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 09:28:25 -0500 Subject: [PATCH] security: scrub on-screen secrets on lock; redact lite console secret args; no-shell wallet rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Locking the wallet (manual / auto / lite) now calls wipeDisplayedSecrets(), which scrubs + closes any revealed seed phrase, migration seed, or exported key — previously a locked wallet could leave the phrase/key on screen and resident. Runs synchronously, before the async lock round-trip. - The console secret-arg redaction list only had full-node RPC names, so lite verbs (import / timport / encrypt / decrypt / unlock) echoed keys and passphrases into the scrollback and up-arrow history; added them. - The offline wallet-rebuild helper was invoked via a shell string (popen "sh -c") built from paths including the wallet filename, so a crafted name could inject a command; it now refuses any path with a shell metacharacter (rc stays -1 and the existing verify-before-swap fails safe). Co-Authored-By: Claude Opus 4.8 --- src/app.cpp | 19 +++++++++++++++++++ src/app.h | 1 + src/app_network.cpp | 17 +++++++++++++++-- src/app_security.cpp | 1 + src/ui/windows/console_tab_helpers.cpp | 4 ++++ 5 files changed, 40 insertions(+), 2 deletions(-) diff --git a/src/app.cpp b/src/app.cpp index 8f6be17..ab34a81 100644 --- a/src/app.cpp +++ b/src/app.cpp @@ -145,6 +145,25 @@ void App::wipeSecrets() sodium_memzero(import_clip_preview_, sizeof(import_clip_preview_)); // clipboard preview (SECRET) } +// Scrub + close any secret that is currently ON SCREEN (revealed seed phrase, migration seed, exported +// key) when the wallet locks — locking shouldn't leave a phrase/key visible or resident. Called on every +// lock transition (manual, auto, lite). Separate from wipeSecrets(), which is the exit-path scrub. +void App::wipeDisplayedSecrets() +{ + if (!seed_backup_phrase_.empty()) { + sodium_memzero(&seed_backup_phrase_[0], seed_backup_phrase_.size()); + seed_backup_phrase_.clear(); + } + if (!seed_migration_seed_.empty()) { + sodium_memzero(&seed_migration_seed_[0], seed_migration_seed_.size()); + seed_migration_seed_.clear(); + } + seed_backup_fetch_started_ = false; + show_seed_backup_ = false; + sodium_memzero(export_result_, sizeof(export_result_)); + ui::KeyExportDialog::hide(); // zeros its static s_key via secureWipeLiteSecret +} + // Enqueue a blocking xmrig start/stop op onto the dedicated serialized control thread so the render thread // never blocks on stop()'s SIGTERM->SIGKILL->join, while start/stop still execute in FIFO order. (M-03/…) void App::postMiningControl(std::function job) diff --git a/src/app.h b/src/app.h index 8f31dc2..06797a3 100644 --- a/src/app.h +++ b/src/app.h @@ -114,6 +114,7 @@ class App { public: App(); void wipeSecrets(); // scrub all resident secret buffers; called from ~App() AND the forced-exit path (L-05) + void wipeDisplayedSecrets(); // scrub + close any on-screen secret (seed/key displays) on wallet lock ~App(); // Non-copyable diff --git a/src/app_network.cpp b/src/app_network.cpp index fdaffcd..8376def 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -5399,6 +5399,7 @@ bool App::lockLiteWallet() if (!lite_wallet_) return false; const bool ok = lite_wallet_->lockWallet(); if (ok) { + wipeDisplayedSecrets(); // scrub any revealed seed/key display on lock (matches full-node) // Full-node App::lockWallet() sets state_.locked synchronously; the lite backend `lock` // doesn't, and state_.locked only refreshes on the next ~2s poll. Mirror the full-node // behavior here and tear down the chat session NOW so no decrypted store / unlocked DB key @@ -6917,9 +6918,21 @@ void App::rebuildWalletDatabase() // 3. Run the helper (src -> tmpOut). Quote both paths; capture its JSON line. Windowless // (runHiddenCapture) so a wallet rebuild never flashes a cmd.exe console; it runs the // helper via CreateProcess directly on Windows, so no cmd.exe outer-quote wrap is needed. - const std::string cmd = "\"" + helper + "\" \"" + src + "\" \"" + tmpOut + "\""; + // runHiddenCapture runs this via popen("sh -c …") on POSIX, where $( ), backticks and an + // embedded \" still expand inside the double-quotes — so a crafted wallet filename (src is + // datadir + the activeWalletName setting) could inject a shell command. Refuse any path + // carrying a shell metacharacter; rc stays -1 and the verify-before-swap below fails safe. + auto shellSafe = [](const std::string& p) { + return p.find_first_of("\"'`$;&|<>^\n\r") == std::string::npos; + }; int rc = -1; - const std::string jout = util::Platform::runHiddenCapture(cmd, /*mergeStderr=*/false, &rc); + std::string jout; + if (shellSafe(helper) && shellSafe(src) && shellSafe(tmpOut)) { + const std::string cmd = "\"" + helper + "\" \"" + src + "\" \"" + tmpOut + "\""; + jout = util::Platform::runHiddenCapture(cmd, /*mergeStderr=*/false, &rc); + } else { + DEBUG_LOGF("[App] wallet-rebuild refused: shell metacharacter in a path\n"); + } DEBUG_LOGF("[App] wallet-rebuild helper rc=%d out=%s\n", rc, jout.c_str()); // 4. Verify-before-swap: the output must be a readable BDB with the fund-critical keys. diff --git a/src/app_security.cpp b/src/app_security.cpp index ee3c45e..b880e69 100644 --- a/src/app_security.cpp +++ b/src/app_security.cpp @@ -598,6 +598,7 @@ void App::unlockWallet(const std::string& passphrase, int timeout) { void App::lockWallet() { if (!rpc_ || !rpc_->isConnected() || !worker_) return; if (lock_unlock_in_progress_) return; // Prevent duplicate async calls + wipeDisplayedSecrets(); // hide any revealed seed/key immediately, before the async lock round-trip lock_unlock_in_progress_ = true; // Use fast-lane worker to avoid blocking behind refreshData. diff --git a/src/ui/windows/console_tab_helpers.cpp b/src/ui/windows/console_tab_helpers.cpp index 0b53b91..b376946 100644 --- a/src/ui/windows/console_tab_helpers.cpp +++ b/src/ui/windows/console_tab_helpers.cpp @@ -15,6 +15,10 @@ const char* const kSecretConsoleCommands[] = { "importprivkey", "importwallet", "importmulti", "z_importkey", "z_importviewingkey", "z_importwallet", "signrawtransaction", "magicrecoverkey", "sethdseed", "importmnemonic", + // Lite-wallet verbs (console_command_reference kLiteKeyCommands): these take a spending/viewing key + // or a password as their argument. Full-node has no bare import/encrypt/decrypt/unlock verb, so a + // flat match is safe. + "import", "timport", "encrypt", "decrypt", "unlock", }; std::string firstConsoleTokenLower(const std::string& cmd, size_t& tokenEnd) {