security: scrub on-screen secrets on lock; redact lite console secret args; no-shell wallet rebuild

- 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 <noreply@anthropic.com>

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<void()> job)

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

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.

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.

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) {

src/wallet/lite_owned_string.cpp

@@ -40,7 +40,8 @@ LiteBridgeOwnedString::LiteBridgeOwnedString(LiteBridgeOwnedString&& other) noex
 LiteBridgeOwnedString& LiteBridgeOwnedString::operator=(LiteBridgeOwnedString&& other) noexcept
 {
     if (this == &other) return *this;
-    copyValueBeforeFree();
+    // Free only — do NOT copy the raw value here: nothing reads copiedValue_ after this, and the copy
+    // (std::string assignment) can throw bad_alloc, which from this noexcept path would call std::terminate.
     releaseRawValue();
     wipeString(copiedValue_);
     moveFrom(std::move(other));
@@ -49,7 +50,8 @@ LiteBridgeOwnedString& LiteBridgeOwnedString::operator=(LiteBridgeOwnedString&&
 
 LiteBridgeOwnedString::~LiteBridgeOwnedString()
 {
-    copyValueBeforeFree();
+    // Free only (see operator= note): the copy is unnecessary when the object is dying and must never
+    // throw out of a destructor. The copy happens solely in intoResult(), the one path that reads it.
     releaseRawValue();
     wipeString(copiedValue_);
 }
@@ -66,13 +68,16 @@ LiteBridgeStringResult LiteBridgeOwnedString::intoResult()
         return {false, {}, "lite bridge freeString function is unavailable"};
     }
 
+    // copyValueBeforeFree() can throw bad_alloc; if it does, rawValue_ is still set and the destructor
+    // (now free-only) releases it — no leak, no re-copy, no terminate.
     copyValueBeforeFree();
     releaseRawValue();
 
-    const std::string resultValue = copiedValue_;
+    // Move the copy into the result instead of making a second const copy that would linger unwiped.
+    std::string resultValue = std::move(copiedValue_);
     wipeString(copiedValue_);
     if (looksLikeError(resultValue)) return {false, {}, resultValue};
-    return {true, resultValue, {}};
+    return {true, std::move(resultValue), {}};
 }
 
 void LiteBridgeOwnedString::copyValueBeforeFree()

src/wallet/lite_result_parsers.cpp

@@ -95,6 +95,9 @@ bool jsonToUnsigned(const json& value, std::uint64_t& output)
         if (value.is_string()) {
             const auto text = value.get<std::string>();
             if (text.empty()) return false;
+            // stoull follows strtoull: it accepts a leading '-' (wrapping to a huge value), '+' and
+            // whitespace. Require a leading digit so a hostile/buggy backend can't forge a balance.
+            if (!std::isdigit(static_cast<unsigned char>(text[0]))) return false;
             std::size_t parsedSize = 0;
             const auto parsed = std::stoull(text, &parsedSize, 10);
             if (parsedSize != text.size()) return false;
@@ -125,6 +128,8 @@ bool jsonToSigned(const json& value, std::int64_t& output)
         if (value.is_string()) {
             const auto text = value.get<std::string>();
             if (text.empty()) return false;
+            // Reject leading whitespace / '+' that strtoll would otherwise accept (a '-' is valid here).
+            if (text[0] != '-' && !std::isdigit(static_cast<unsigned char>(text[0]))) return false;
             std::size_t parsedSize = 0;
             const auto parsed = std::stoll(text, &parsedSize, 10);
             if (parsedSize != text.size()) return false;
@@ -337,6 +342,10 @@ bool parseTransactionRecord(const json& value,
         for (std::size_t index = 0; index < metadata.size(); ++index) {
             LiteTransactionOutput parsedOutput;
             if (!parseTransactionOutput(metadata[index], path + ".outgoing_metadata[" + std::to_string(index) + "]", parsedOutput, result)) return false;
+            // Reject negative values and guard the running sum: signed overflow is UB, and a hostile
+            // backend could otherwise overflow the total into a bogus (even negative) amount.
+            if (parsedOutput.value < 0) return false;
+            if (output.amount > std::numeric_limits<std::int64_t>::max() - parsedOutput.value) return false;
             output.amount += parsedOutput.value;
             output.outgoingMetadata.push_back(std::move(parsedOutput));
         }