DragonX/ObsidianDragon
1
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

fix(security): scrub in-memory key/passphrase copies in the wallet secret paths (W4-1, W4-3, W2-3)

Browse Source 
The wallet-hardening memzero cluster. Uses the file's established sodium_memzero
pattern (matching the existing lambda-capture scrub at app_network.cpp:2885 and the JSON
scrub at :4025) rather than a new type, since importPrivateKey/sweepPrivateKey are
fund-moving code.

- W4-1 importPrivateKey / sweepPrivateKey: the spending/viewing key was copied ≥3×
  (calling frame -> worker-lambda capture -> JSON params) and never scrubbed. Now zeroed
  on all paths: the calling-frame copy after the worker post, the lambda's captured copy
  (lambda made mutable, zeroed once the request is sent), and the request params copy.

- W4-3 exportAllKeys / backupWallet: the concatenated all-keys buffer is now zeroed after
  the consumer uses it, and the backup is written via
  Platform::writeFileAtomically(..., restrictPermissions=true) — atomic and owner-only
  (0600) — instead of a umask-default std::ofstream that left it world-readable.

- W2-3 decrypt-wallet passphrase: std::move-captured into the worker lambda (no plaintext
  copy left in the calling frame) and sodium_memzero'd right after unlockWallet, its only
  use.

Not unit-testable (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. See
docs/wallet-hardening.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
DanS
2026-08-02 14:12:44 -05:00
parent 9204fa148a
commit f9b622cb25
3 changed files with 35 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
docs/wallet-hardening.md
View File

@@ -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.)

31
src/app_network.cpp
View File

@@ -3783,6 +3783,8 @@ void App::exportAllKeys(std::function<void(const std::string&, int, int)> 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<std::string&>();
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<std::string>();
@@ -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<std::string&>();
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<void(bool, bool, const std::string&, const std::string&)> callback)
@@ -4478,13 +4496,12 @@ void App::backupWallet(const std::string& destination, std::function<void(bool,
return;
}
std::ofstream file(destination);
if (!file.is_open()) {
if (callback) callback(false, "Could not open file: " + destination);
// Write the key backup atomically and owner-only (0600) — it must never be even briefly
// world-readable, and the previous std::ofstream left it at the umask default.
if (!util::Platform::writeFileAtomically(destination, keys, /*restrictPermissions=*/true)) {
if (callback) callback(false, "Could not write file: " + destination);
return;
}
file << keys;
file.close();
std::string msg = "Wallet backup saved to " + destination + " — "
+ std::to_string(exported) + " of " + std::to_string(total) + " keys.";

6
src/app_security.cpp
View File

@@ -35,6 +35,8 @@
#include <filesystem>
#include <fstream>
#include <vector>
#include <utility>
#include <sodium.h>
#include <functional>
#include <memory>
#include <utility>
@@ -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");