diff --git a/docs/wallet-hardening.md b/docs/wallet-hardening.md index 1e24b4d..a827990 100644 --- a/docs/wallet-hardening.md +++ b/docs/wallet-hardening.md @@ -16,7 +16,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 (SecureString + console redaction + delete-export) | ◐ | +| **P0-A** | W7-1, W2-1, W4-1, W4-3, W2-3 ✓ · W4-5, W5-3 ☐ | Secret hardening (console redaction + delete-export + memzero) | ◐ 5/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 | ☐ | @@ -112,5 +112,10 @@ Land W7-2 first — it unblocks the rest. ## Progress log +- **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.) diff --git a/src/app_network.cpp b/src/app_network.cpp index 0c33be3..6370af4 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -3783,6 +3783,8 @@ void App::exportAllKeys(std::function callba (*pending)--; if (*pending == 0 && callback) { callback(*keys_result, *exported, *total); + // Scrub the concatenated all-keys buffer once the consumer (backup writer) has used it. + if (!keys_result->empty()) sodium_memzero(&(*keys_result)[0], keys_result->size()); } }); } @@ -3814,7 +3816,7 @@ void App::importPrivateKey(const std::string& rawKey, int startHeight, == services::WalletSecurityController::KeyKind::Shielded; // Run on the worker thread — import requests a full rescan (rescan=true), so the // synchronous curl call can take many seconds; never block the UI thread on it. - worker_->post([this, key, viewing, shielded, startHeight, callback]() -> rpc::RPCWorker::MainCb { + worker_->post([this, key, viewing, shielded, startHeight, callback]() mutable -> rpc::RPCWorker::MainCb { std::string err, addr; try { rpc::RPCClient::TraceScope trace("Settings / Import key"); @@ -3826,6 +3828,11 @@ void App::importPrivateKey(const std::string& rawKey, int startHeight, // A start height (shielded RPCs only) rescans from that block instead of genesis. if (startHeight > 0 && (viewing || shielded)) params.push_back(startHeight); nlohmann::json r = rpc_->call(method, params); + // Scrub the key out of the request params (the json holds its own copy of it). + if (params.is_array() && !params.empty() && params[0].is_string()) { + std::string& pk = params[0].get_ref(); + if (!pk.empty()) sodium_memzero(&pk[0], pk.size()); + } // z_import* return {type,address}; importprivkey returns the t-address string. if (r.is_object() && r.contains("address") && r["address"].is_string()) addr = r["address"].get(); @@ -3838,6 +3845,8 @@ void App::importPrivateKey(const std::string& rawKey, int startHeight, // below would never run, leaving a stuck "Importing…" spinner. err = "Import failed (unknown error)"; } + // Scrub the worker's copy of the key now that the request has been sent (all paths). + if (!key.empty()) sodium_memzero(&key[0], key.size()); return [this, err, addr, callback]() { if (!err.empty()) { if (callback) callback(false, err, ""); @@ -3848,6 +3857,7 @@ void App::importPrivateKey(const std::string& rawKey, int startHeight, if (callback) callback(true, "", addr); }; }); + if (!key.empty()) sodium_memzero(&key[0], key.size()); // scrub the calling-frame copy } // Sweep a spending key: import it (a full rescan populates its UTXOs/notes — the stock node has no @@ -3885,7 +3895,7 @@ void App::sweepPrivateKey(const std::string& rawKey, int startHeight, int destMo const bool shielded = services::WalletSecurityController::classifyPrivateKey(key) == services::WalletSecurityController::KeyKind::Shielded; const double fee = DRAGONX_DEFAULT_FEE; - worker_->post([this, key, startHeight, destMode, destExisting, shielded, fee]() -> rpc::RPCWorker::MainCb { + worker_->post([this, key, startHeight, destMode, destExisting, shielded, fee]() mutable -> rpc::RPCWorker::MainCb { std::string err, dest, sourceAddr, amountStr; double amount = 0.0; try { @@ -3909,6 +3919,11 @@ void App::sweepPrivateKey(const std::string& rawKey, int startHeight, int destMo else { method = "importprivkey"; params = {key, "", true}; } if (startHeight > 0 && shielded) params.push_back(startHeight); nlohmann::json r = rpc_->call(method, params); + // Scrub the key out of the request params (the json holds its own copy of it). + if (params.is_array() && !params.empty() && params[0].is_string()) { + std::string& pk = params[0].get_ref(); + if (!pk.empty()) sodium_memzero(&pk[0], pk.size()); + } // 2. Determine the swept address. importprivkey returns the t-address string; z_importkey // returns null, so diff the z-address list to find the one the key just added. @@ -3967,6 +3982,8 @@ void App::sweepPrivateKey(const std::string& rawKey, int startHeight, int destMo } catch (...) { err = "Sweep failed (unknown error)"; } + // Scrub the worker's copy of the spending key now that the request has been sent (all paths). + if (!key.empty()) sodium_memzero(&key[0], key.size()); return [this, err, sourceAddr, dest, amount, amountStr, fee]() { invalidateAddressValidationCache(); refreshAddresses(); @@ -4003,6 +4020,7 @@ void App::sweepPrivateKey(const std::string& rawKey, int startHeight, int destMo }); }; }); + if (!key.empty()) sodium_memzero(&key[0], key.size()); // scrub the calling-frame copy } void App::exportSeedPhrase(std::function callback) @@ -4478,13 +4496,12 @@ void App::backupWallet(const std::string& destination, std::function #include #include +#include +#include #include #include #include @@ -1480,12 +1482,14 @@ void App::renderDecryptWalletDialog() { // Run entire decrypt flow on worker thread if (worker_) { - worker_->post([this, passphrase]() -> rpc::RPCWorker::MainCb { + worker_->post([this, passphrase = std::move(passphrase)]() mutable -> rpc::RPCWorker::MainCb { WalletSecurityDecryptRpcAdapter decryptRpc(rpc_.get(), [this](rpc::RPCClient& client, const char* context) { return sendStopCommandSafely(client, context); }); auto unlock = services::WalletSecurityWorkflowExecutor::unlockWallet(passphrase, decryptRpc); + // Scrub the passphrase — unlock is its only use in this flow. + if (!passphrase.empty()) sodium_memzero(&passphrase[0], passphrase.size()); if (!unlock.ok) { return [this]() { wallet_security_workflow_.failEntry("Incorrect passphrase");