From ee9ea1523376a7f6cc21483afe6e1d5d564c933b Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 16 Jul 2026 17:41:24 -0500 Subject: [PATCH] fix(rpc,ui): close the last secret residues found by the final-gate review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two verified medium leaks in the B7 scrub chain: - parseRpcResult parses the body into a local `response` tree and returns a COPY of response["result"] (operator[] yields an lvalue ref, so the by-value return copy-constructs). The local tree — holding its own heap copy of the secret — was then freed without zeroing, so callSecret/callSecretString still left one un-scrubbed copy. Add scrubJsonSecrets() (recursive string zero) and a scrubSource flag; the secret paths opt in, wiping the tree before it frees. The secret export chain is now fully covered: raw body → parse tree → result copy → caller-owned string. - key_export_dialog cleared s_key with plain std::string::clear() on the Close button, the scrim/Esc dismiss path, and the QR cache (s_qr_cached) — leaving the displayed private/spending key in freed heap on the ordinary close paths. Route all three through wallet::secureWipeLiteSecret (zero-then-clear), matching show()/hide(). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/rpc/rpc_client.cpp | 21 +++++++++++++++++---- src/rpc/rpc_client.h | 6 ++++-- src/ui/windows/key_export_dialog.cpp | 10 +++++----- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/src/rpc/rpc_client.cpp b/src/rpc/rpc_client.cpp index 0303c11..29f0d5f 100644 --- a/src/rpc/rpc_client.cpp +++ b/src/rpc/rpc_client.cpp @@ -23,6 +23,17 @@ namespace rpc { namespace { +// Recursively zero every string value in a JSON tree in place — used to wipe a discarded parse tree +// that held a secret (B7). Operates on the underlying std::string buffers via get_ref. +void scrubJsonSecrets(nlohmann::json& j) { + if (j.is_string()) { + auto& s = j.get_ref(); + if (!s.empty()) sodium_memzero(&s[0], s.size()); + } else if (j.is_object() || j.is_array()) { + for (auto& el : j) scrubJsonSecrets(el); + } +} + std::mutex g_trace_mutex; RPCClient::TraceCallback g_trace_callback; std::atomic_bool g_trace_enabled{false}; @@ -317,7 +328,7 @@ std::string RPCClient::performCall(const std::string& method, const json& params return response_data; } -json RPCClient::parseRpcResult(long httpCode, const std::string& body) +json RPCClient::parseRpcResult(long httpCode, const std::string& body, bool scrubSource) { // Bitcoin/Hush RPC returns HTTP 500 for application-level errors // (insufficient funds, bad params, etc.) with a valid JSON body. @@ -355,7 +366,9 @@ json RPCClient::parseRpcResult(long httpCode, const std::string& body) throw RpcError(errCode, "RPC error: " + err_msg); } - return response["result"]; + json result = response["result"]; // a COPY of the subobject (operator[] yields an lvalue ref) + if (scrubSource) scrubJsonSecrets(response); // zero the discarded tree's secret before it frees (B7) + return result; } json RPCClient::call(const std::string& method, const json& params) @@ -382,7 +395,7 @@ json RPCClient::callSecret(const std::string& method, const json& params) long http_code = 0; std::string response_data = performCall(method, params, http_code); try { - json result = parseRpcResult(http_code, response_data); + json result = parseRpcResult(http_code, response_data, /*scrubSource=*/true); if (!response_data.empty()) sodium_memzero(&response_data[0], response_data.size()); return result; } catch (...) { @@ -401,7 +414,7 @@ std::string RPCClient::callSecretString(const std::string& method, const json& p long http_code = 0; std::string response_data = performCall(method, params, http_code); try { - json result = parseRpcResult(http_code, response_data); + json result = parseRpcResult(http_code, response_data, /*scrubSource=*/true); std::string out; if (result.is_string()) { // Copy the secret out, then zero the json node's OWN heap buffer — parseRpcResult's diff --git a/src/rpc/rpc_client.h b/src/rpc/rpc_client.h index f21b658..eb32b45 100644 --- a/src/rpc/rpc_client.h +++ b/src/rpc/rpc_client.h @@ -270,8 +270,10 @@ private: // hold curl_mutex_ and have verified impl_->curl. std::string performCall(const std::string& method, const json& params, long& httpCodeOut); // Centralizes the HTTP-code check and JSON error->RpcError extraction, returning - // response["result"] on success. - static json parseRpcResult(long httpCode, const std::string& body); + // response["result"] on success. When scrubSource is true (secret-bearing calls), the intermediate + // parse tree's string values are zeroed before it is discarded — parseRpcResult returns a COPY of + // the result node, so its own tree would otherwise free the secret un-wiped (B7). + static json parseRpcResult(long httpCode, const std::string& body, bool scrubSource = false); // Splits a UnifiedCallback into the (Callback, ErrorCallback) pair used by doRPC. static std::pair splitUnified(UnifiedCallback cb); diff --git a/src/ui/windows/key_export_dialog.cpp b/src/ui/windows/key_export_dialog.cpp index 25ec3be..fa3f01c 100644 --- a/src/ui/windows/key_export_dialog.cpp +++ b/src/ui/windows/key_export_dialog.cpp @@ -38,7 +38,7 @@ std::string KeyExportDialog::s_error; void KeyExportDialog::releaseQr() { if (s_qr_tex) { FreeQRTexture(s_qr_tex); s_qr_tex = 0; } - s_qr_cached.clear(); + wallet::secureWipeLiteSecret(s_qr_cached); // held a plaintext copy of the key for the QR s_show_qr = false; } @@ -318,8 +318,8 @@ void KeyExportDialog::render(App* app) if (material::TactileButton(TR("close"), ImVec2(button_width, 0), S.resolveFont(closeBtn.font))) { s_open = false; - // Clear sensitive data - s_key.clear(); + // Zero the secret, don't just drop the buffer (leaves the key in freed heap otherwise). + wallet::secureWipeLiteSecret(s_key); s_show_key = false; releaseQr(); } @@ -327,9 +327,9 @@ void KeyExportDialog::render(App* app) material::EndOverlayDialog(); } - // Dialog dismissed any other way (scrim click / Esc): drop the key + its QR texture. + // Dialog dismissed any other way (scrim click / Esc): wipe the key + its QR texture. if (!s_open) { - if (!s_key.empty()) s_key.clear(); + wallet::secureWipeLiteSecret(s_key); s_show_key = false; releaseQr(); }