From fac02452971f7eb60442bcfa529fde7dbc6145aa Mon Sep 17 00:00:00 2001 From: DanS Date: Thu, 16 Jul 2026 17:11:31 -0500 Subject: [PATCH] fix(chat,rpc): address adversarial review of the chat backlog MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four confirmed findings from the review pass: SECURITY — B7 was incomplete: - The single-key Export dialog (key_export_dialog) still used plain call() for z_exportkey/dumpprivkey/z_exportviewingkey — a live spending-key leak on the most common per-address export path, missed by the B7 commit. - callSecret() zeros the raw body but the parsed json holds its OWN heap copy of the secret; several callers did .get() on a temporary json and freed that copy un-wiped. Fix: add RPCClient::callSecretString() — returns the bare-string result with BOTH the raw body AND the json node zeroed, so callers can't forget. Route key_export_dialog (×2), exportPrivateKey, and export_all_keys (×2) through it; scrub the z_exportmnemonic json node in seed_wallet_creator (object result); also wipe the transient key copies, the displayed s_key on reset, and the aggregated export-all `keys` buffer. CHAT: - Jump-to-latest pill: SetCursorScreenPos moved the parent cursor and never restored it, so the composer footer rendered ~8px too high while scrolled up. Save + restore the cursor around the pill. - New-message toast: gating on a chatUnreadCount() watermark delta could be swallowed when an outgoing echo (wall-clock) pushed the seen-watermark past a later reply's block time. ingest() now reports the cids it appended; the toast fires when any is a non-muted conversation — skew-proof, still mute-aware. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/app_network.cpp | 28 +++++++++++++--------- src/chat/chat_service.cpp | 6 ++++- src/chat/chat_service.h | 6 ++++- src/daemon/seed_wallet_creator.cpp | 8 +++++-- src/rpc/rpc_client.cpp | 29 +++++++++++++++++++++++ src/rpc/rpc_client.h | 9 +++++++ src/ui/windows/chat_tab.cpp | 6 ++++- src/ui/windows/export_all_keys_dialog.cpp | 28 +++++++++++----------- src/ui/windows/key_export_dialog.cpp | 17 ++++++------- 9 files changed, 99 insertions(+), 38 deletions(-) diff --git a/src/app_network.cpp b/src/app_network.cpp index 4e2028b..837b1f1 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -1689,11 +1689,14 @@ void App::refreshTransactionData() !result.hushChatMetadata.empty()) { std::unordered_map chatTxTimes; for (const auto& tx : result.transactions) chatTxTimes[tx.txid] = tx.timestamp; - const int unreadBefore = chatUnreadCount(); - const int newMsgs = chat_service_.ingest(result.hushChatMetadata, chatTxTimes, std::time(nullptr)); - // Toast only when a non-muted conversation gained an unread message — chatUnreadCount() - // already skips muted cids, so a delta cleanly respects mute (Q10 + Q4). - if (newMsgs > 0 && current_page_ != ui::NavPage::Chat && chatUnreadCount() > unreadBefore) + std::vector newChatCids; + chat_service_.ingest(result.hushChatMetadata, chatTxTimes, std::time(nullptr), &newChatCids); + // Toast when a NON-muted conversation received a new message and we're off the Chat tab. + // Gate on the ingest-reported cids, not a seen-watermark delta — the latter can be swallowed + // by block-time vs wall-clock (echo) skew (Q10 + Q4). + if (current_page_ != ui::NavPage::Chat && + std::any_of(newChatCids.begin(), newChatCids.end(), + [this](const std::string& cid){ return !(settings_ && settings_->isChatMuted(cid)); })) ui::Notifications::instance().info(TR("chat_new_message_toast")); } NetworkRefreshService::applyTransactionRefreshResult( @@ -1752,11 +1755,14 @@ void App::refreshRecentTransactionData() !result.hushChatMetadata.empty()) { std::unordered_map chatTxTimes; for (const auto& tx : result.transactions) chatTxTimes[tx.txid] = tx.timestamp; - const int unreadBefore = chatUnreadCount(); - const int newMsgs = chat_service_.ingest(result.hushChatMetadata, chatTxTimes, std::time(nullptr)); - // Toast only when a non-muted conversation gained an unread message — chatUnreadCount() - // already skips muted cids, so a delta cleanly respects mute (Q10 + Q4). - if (newMsgs > 0 && current_page_ != ui::NavPage::Chat && chatUnreadCount() > unreadBefore) + std::vector newChatCids; + chat_service_.ingest(result.hushChatMetadata, chatTxTimes, std::time(nullptr), &newChatCids); + // Toast when a NON-muted conversation received a new message and we're off the Chat tab. + // Gate on the ingest-reported cids, not a seen-watermark delta — the latter can be swallowed + // by block-time vs wall-clock (echo) skew (Q10 + Q4). + if (current_page_ != ui::NavPage::Chat && + std::any_of(newChatCids.begin(), newChatCids.end(), + [this](const std::string& cid){ return !(settings_ && settings_->isChatMuted(cid)); })) ui::Notifications::instance().info(TR("chat_new_message_toast")); } NetworkRefreshService::applyTransactionRefreshResult( @@ -2639,7 +2645,7 @@ void App::exportPrivateKey(const std::string& address, std::functioncallSecret(method, {address}).get(); // zero the raw body too (B7) + key = rpc_->callSecretString(method, {address}); // scrubs raw body + json node (B7) } catch (const std::exception& e) { err = e.what(); } diff --git a/src/chat/chat_service.cpp b/src/chat/chat_service.cpp index ac3ba8d..c994cd6 100644 --- a/src/chat/chat_service.cpp +++ b/src/chat/chat_service.cpp @@ -25,7 +25,8 @@ void ChatService::clearIdentity() { int ChatService::ingest(const std::vector& metadata, const std::unordered_map& txTimestamps, - std::int64_t fallbackTimestamp) { + std::int64_t fallbackTimestamp, + std::vector* newIncomingCids) { if (!has_identity_) return 0; int added = 0; @@ -59,6 +60,9 @@ int ChatService::ingest(const std::vector& metadata if (store_.append(message)) { if (db_) db_->append(message); ++added; + // Every ingested message is incoming — report its cid so the caller can notify without + // relying on a seen-watermark delta (which block-time vs wall-clock skew can swallow). + if (newIncomingCids) newIncomingCids->push_back(message.conversation_id); } } return added; diff --git a/src/chat/chat_service.h b/src/chat/chat_service.h index d386eed..2366606 100644 --- a/src/chat/chat_service.h +++ b/src/chat/chat_service.h @@ -40,9 +40,13 @@ public: // when the txid isn't present. Newly-added messages are also persisted (if a database is // attached). Returns the number of NEW messages added; 0 with no identity. Undecryptable // Messages are dropped silently (no logging of memo/plaintext). + // When `newIncomingCids` is non-null it is filled with the conversation ids of the genuinely-new + // incoming (non-request) messages appended this call — a reliable "a new message arrived here" + // signal for notifications that doesn't depend on any timestamp/seen-watermark comparison. int ingest(const std::vector& metadata, const std::unordered_map& txTimestamps, - std::int64_t fallbackTimestamp = 0); + std::int64_t fallbackTimestamp = 0, + std::vector* newIncomingCids = nullptr); // Attach a persistent backing store (Phase 2). Not owned. Pass nullptr to detach. New messages // from ingest() are written through; loadFromDatabase() rehydrates the in-memory store from it. diff --git a/src/daemon/seed_wallet_creator.cpp b/src/daemon/seed_wallet_creator.cpp index 9d0d992..05bbc57 100644 --- a/src/daemon/seed_wallet_creator.cpp +++ b/src/daemon/seed_wallet_creator.cpp @@ -111,8 +111,12 @@ SeedWalletResult SeedWalletCreator::create(bool keepDatadir, // 6. Export the new seed phrase + a fresh shielded receive address (the future sweep target). try { auto m = cli.callSecret("z_exportmnemonic"); // zero the raw body too (B7) - if (m.contains("mnemonic") && m["mnemonic"].is_string()) - r.seedPhrase = m["mnemonic"].get(); + if (m.contains("mnemonic") && m["mnemonic"].is_string()) { + // Take our copy, then scrub the json node's own copy so it isn't freed in the clear (B7). + auto& mn = m["mnemonic"].get_ref(); + r.seedPhrase = mn; + if (!mn.empty()) sodium_memzero(&mn[0], mn.size()); + } r.destAddress = cli.call("z_getnewaddress").get(); r.ok = !r.seedPhrase.empty() && !r.destAddress.empty(); if (!r.ok) r.error = "The isolated node returned an empty seed or address."; diff --git a/src/rpc/rpc_client.cpp b/src/rpc/rpc_client.cpp index ee9df39..0303c11 100644 --- a/src/rpc/rpc_client.cpp +++ b/src/rpc/rpc_client.cpp @@ -391,6 +391,35 @@ json RPCClient::callSecret(const std::string& method, const json& params) } } +std::string RPCClient::callSecretString(const std::string& method, const json& params) +{ + std::lock_guard lk(curl_mutex_); + if (!impl_->curl) { + throw std::runtime_error("Not connected"); + } + + long http_code = 0; + std::string response_data = performCall(method, params, http_code); + try { + json result = parseRpcResult(http_code, response_data); + std::string out; + if (result.is_string()) { + // Copy the secret out, then zero the json node's OWN heap buffer — parseRpcResult's + // json::parse allocates this independently of response_data, so scrubbing the raw body + // alone would leave it behind (B7). `out` is now the only live copy; the caller wipes it. + auto& s = result.get_ref(); + out = s; + if (!s.empty()) sodium_memzero(&s[0], s.size()); + } + if (!response_data.empty()) sodium_memzero(&response_data[0], response_data.size()); + if (!result.is_string()) throw std::runtime_error("RPC result is not a string"); + return out; + } catch (...) { + if (!response_data.empty()) sodium_memzero(&response_data[0], response_data.size()); + throw; + } +} + json RPCClient::call(const std::string& method, const json& params, long timeoutSec) { std::lock_guard lk(curl_mutex_); diff --git a/src/rpc/rpc_client.h b/src/rpc/rpc_client.h index a1335fc..f21b658 100644 --- a/src/rpc/rpc_client.h +++ b/src/rpc/rpc_client.h @@ -144,6 +144,15 @@ public: */ json callSecret(const std::string& method, const json& params = json::array()); + /** + * @brief callSecret() for RPCs whose result is a bare secret string (z_exportkey / dumpprivkey / + * z_exportviewingkey). Returns the result string with BOTH the raw response body AND the + * parsed json's own copy of the secret zeroed — so no un-scrubbed heap copy survives the + * call. The returned std::string is the only remaining copy; the caller owns it and must + * wipe it (sodium_memzero) when done. Throws if the result is not a JSON string. + */ + std::string callSecretString(const std::string& method, const json& params = json::array()); + /** * @brief Make a raw RPC call with a custom timeout * @param method RPC method name diff --git a/src/ui/windows/chat_tab.cpp b/src/ui/windows/chat_tab.cpp index dfea350..36a4f2c 100644 --- a/src/ui/windows/chat_tab.cpp +++ b/src/ui/windows/chat_tab.cpp @@ -525,14 +525,18 @@ void RenderChatTab(App* app) atBottom = ImGui::GetScrollY() >= ImGui::GetScrollMaxY() - 4.0f; } ImGui::EndChild(); - // Jump-to-latest pill when scrolled up (Q9). + // Jump-to-latest pill when scrolled up (Q9). SetCursorScreenPos moves the parent cursor to + // the child's bottom edge; save + restore it so the composer footer below stays anchored + // (otherwise it renders ~8px too high only while the pill is shown). if (!atBottom) { + const ImVec2 savedCursor = ImGui::GetCursorScreenPos(); const ImVec2 pill(msgWinMin.x + msgWinSize.x - 84.0f * dp, msgWinMin.y + msgWinSize.y - 34.0f * dp); ImGui::SetCursorScreenPos(pill); ImGui::PushStyleColor(ImGuiCol_Button, ImGui::ColorConvertU32ToFloat4(material::WithAlpha(material::Primary(), 220))); ImGui::PushStyleColor(ImGuiCol_ButtonHovered, ImGui::ColorConvertU32ToFloat4(material::Primary())); if (ImGui::Button(TR("chat_jump_latest"), ImVec2(74.0f * dp, 26.0f * dp))) s_scroll_to_cid = sel->cid; ImGui::PopStyleColor(2); + ImGui::SetCursorScreenPos(savedCursor); } // Composer footer: message input + send (only once we know the peer's key), else a hint. diff --git a/src/ui/windows/export_all_keys_dialog.cpp b/src/ui/windows/export_all_keys_dialog.cpp index 254f335..699418c 100644 --- a/src/ui/windows/export_all_keys_dialog.cpp +++ b/src/ui/windows/export_all_keys_dialog.cpp @@ -4,6 +4,7 @@ #include "export_all_keys_dialog.h" #include "../../app.h" +#include // sodium_memzero — wipe exported key material from memory (B7) #include "../../rpc/rpc_client.h" #include "../../rpc/rpc_worker.h" #include "../../util/i18n.h" @@ -180,13 +181,12 @@ void ExportAllKeysDialog::render(App* app) for (const auto& addr : z_addrs) { try { rpc::RPCClient::TraceScope trace("Settings / Export all keys"); - auto result = rpc->callSecret("z_exportkey", {addr}); // zero the raw body too (B7) - if (result.is_string()) { - keys += "# Address: " + addr + "\n"; - keys += result.get() + "\n\n"; - exported++; // count only real successes (locked/failed keys don't count) - } - } catch (...) {} + std::string k = rpc->callSecretString("z_exportkey", {addr}); // scrubs raw body + json node (B7) + keys += "# Address: " + addr + "\n"; + keys += k + "\n\n"; + exported++; // count only real successes (locked/failed keys don't count) + if (!k.empty()) sodium_memzero(&k[0], k.size()); // wipe the transient copy + } catch (...) {} // non-string / locked key → skip (not counted) } } @@ -196,13 +196,12 @@ void ExportAllKeysDialog::render(App* app) for (const auto& addr : t_addrs) { try { rpc::RPCClient::TraceScope trace("Settings / Export all keys"); - auto result = rpc->callSecret("dumpprivkey", {addr}); // zero the raw body too (B7) - if (result.is_string()) { - keys += "# Address: " + addr + "\n"; - keys += result.get() + "\n\n"; - exported++; // count only real successes (locked/failed keys don't count) - } - } catch (...) {} + std::string k = rpc->callSecretString("dumpprivkey", {addr}); // scrubs raw body + json node (B7) + keys += "# Address: " + addr + "\n"; + keys += k + "\n\n"; + exported++; // count only real successes (locked/failed keys don't count) + if (!k.empty()) sodium_memzero(&k[0], k.size()); // wipe the transient copy + } catch (...) {} // non-string / locked key → skip (not counted) } } @@ -219,6 +218,7 @@ void ExportAllKeysDialog::render(App* app) writeOk = true; } } + if (!keys.empty()) sodium_memzero(&keys[0], keys.size()); // don't leave every key in freed heap return [exported, total, filepath, writeOk]() { s_exported_count = exported; diff --git a/src/ui/windows/key_export_dialog.cpp b/src/ui/windows/key_export_dialog.cpp index 7520c3c..25ec3be 100644 --- a/src/ui/windows/key_export_dialog.cpp +++ b/src/ui/windows/key_export_dialog.cpp @@ -5,6 +5,7 @@ #include "key_export_dialog.h" #include "../../app.h" #include "../../wallet/lite_wallet_controller.h" +#include // sodium_memzero — wipe transient secret copies (B7) #include #include "../../rpc/rpc_client.h" #include "../../rpc/rpc_worker.h" @@ -49,7 +50,7 @@ void KeyExportDialog::show(const std::string& address, KeyType type) releaseQr(); s_key_type = type; s_address = address; - s_key.clear(); + wallet::secureWipeLiteSecret(s_key); // zero any prior secret before reuse s_error.clear(); } @@ -62,7 +63,7 @@ void KeyExportDialog::hide() { s_open = false; s_fetching = false; - s_key.clear(); + wallet::secureWipeLiteSecret(s_key); // zero the displayed secret, don't just drop the buffer s_show_key = false; s_error.clear(); releaseQr(); @@ -187,18 +188,18 @@ void KeyExportDialog::render(App* app) std::string error; try { rpc::RPCClient::TraceScope trace("Settings / Export key"); - auto result = rpc->call(method, {addr}); - key = result.get(); + key = rpc->callSecretString(method, {addr}); // scrubs raw body + json node (B7) } catch (const std::exception& e) { error = e.what(); } - return [key, error]() { + return [key = std::move(key), error]() mutable { if (error.empty()) { s_key = key; s_show_key = false; // Don't show by default } else { s_error = error; } + if (!key.empty()) sodium_memzero(&key[0], key.size()); // wipe the last transient copy s_fetching = false; }; }); @@ -213,18 +214,18 @@ void KeyExportDialog::render(App* app) std::string error; try { rpc::RPCClient::TraceScope trace("Settings / Export viewing key"); - auto result = rpc->call("z_exportviewingkey", {addr}); - key = result.get(); + key = rpc->callSecretString("z_exportviewingkey", {addr}); // scrubs raw body + json node (B7) } catch (const std::exception& e) { error = e.what(); } - return [key, error]() { + return [key = std::move(key), error]() mutable { if (error.empty()) { s_key = key; s_show_key = true; // Viewing keys are less sensitive } else { s_error = error; } + if (!key.empty()) sodium_memzero(&key[0], key.size()); // wipe the last transient copy s_fetching = false; }; });