From 63dcda0ad1332b49781c433a1d8aa6fa4ab4c444 Mon Sep 17 00:00:00 2001 From: DanS Date: Wed, 15 Jul 2026 13:06:56 -0500 Subject: [PATCH] fix(wallet-switch): close gaps found verifying clusters A/B/C MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial verification of the audit fixes found real gaps in them: - HIGH throw-safety: the switch worker and the encryption-restart worker set daemon_restarting_=true but reset it only on their normal/early-return paths — a throw from stop/startEmbeddedDaemon left the flag stuck true, wedging reconnect and every future switch/rescan/encryption. Both now reset it on all paths (try/catch); a throw during a switch is treated as a failed switch and triggers the revert. - HIGH residual chat leak: resetChatSession() cleared the flags but an already- posted z_exportmnemonic worker job still held wallet A's secret, and its completion callback (guarded only by isLocked(), false for an unencrypted wallet) would provision A's identity under B. Add a chat_session_generation_ epoch bumped on every wallet change; the fetch captures it and its callback discards the (previous-wallet) secret if the epoch no longer matches. - LOW vault-scope collision: the per-wallet vault tag was a lossy char-substitution (two distinct files could map to one vault). Append an 8-hex FNV-1a of the raw filename so distinct wallets never share a vault. +unit test. - LOW seed-adopt: removeVault() on adopt so the legacy wallet's PIN passphrase isn't left associated with the new seed wallet (same file name). - LOW: clear lock_unlock_in_progress_ on switch too. Deferred (documented): the 1.5s start grace can't catch a wallet that fails LATE in daemon init (the existing crash-wedge detection still applies); beginShutdown's join of the switch task can briefly freeze the UI during quit (necessary to avoid orphaning the daemon). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/app.h | 4 ++++ src/app_network.cpp | 50 ++++++++++++++++++++++++++++----------- src/app_security.cpp | 16 ++++++++----- src/util/secure_vault.cpp | 12 ++++++++-- tests/test_phase4.cpp | 30 +++++++++++++++++++++++ 5 files changed, 90 insertions(+), 22 deletions(-) diff --git a/src/app.h b/src/app.h index 58b4086..f6fbd2d 100644 --- a/src/app.h +++ b/src/app.h @@ -655,6 +655,10 @@ private: chat::ChatDatabase chat_db_; // persistent backing (seed-derived encryption at rest) bool chat_identity_provisioned_ = false; // identity set on the service this session bool chat_identity_fetch_in_flight_ = false; // a z_exportmnemonic worker job is pending + // Bumped by resetChatSession() on every wallet change; an in-flight identity fetch captures the + // value at post time and its completion callback discards its (previous-wallet) secret if the id + // no longer matches — so wallet A's seed can't be provisioned under wallet B via a stale job. + int chat_session_generation_ = 0; bool chat_identity_unavailable_ = false; // provisioning failed definitively (e.g. non-mnemonic wallet) // Provision the chat identity once the wallet seed is reachable+unlocked (per-tick, both // variants); derives via deriveChatIdentityFromSecret and wipes the secret. No-op when the diff --git a/src/app_network.cpp b/src/app_network.cpp index cab54c5..2d10097 100644 --- a/src/app_network.cpp +++ b/src/app_network.cpp @@ -1069,6 +1069,7 @@ void App::switchToWallet(const std::string& walletFile) lock_lockout_timer_ = 0.0f; lock_error_msg_.clear(); lock_error_timer_ = 0.0f; + lock_unlock_in_progress_ = false; util::SecureVault::secureZero(lock_passphrase_buf_, sizeof(lock_passphrase_buf_)); util::SecureVault::secureZero(lock_pin_buf_, sizeof(lock_pin_buf_)); @@ -1086,23 +1087,29 @@ void App::switchToWallet(const std::string& walletFile) ui::Notifications::instance().info("Switching wallet — the node will restart…"); async_tasks_.submit("Switch wallet", [this, needRescan](const util::AsyncTaskManager::Token&) { - // Stop the node fully so it releases the datadir .lock + RPC port before we relaunch. - stopEmbeddedDaemon(); - for (int i = 0; i < 60 && isEmbeddedDaemonRunning(); ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(500)); - if (needRescan && daemon_controller_) daemon_controller_->setRescanOnNextStart(true); - bool ok = shutting_down_ ? true : startEmbeddedDaemon(); - // A missing/corrupt wallet makes dragonxd exit during init — confirm the process survived a - // moment; if not, the wallet is bad. (A valid launch keeps the process alive while it syncs.) - if (ok && !shutting_down_) { - std::this_thread::sleep_for(std::chrono::milliseconds(1500)); - if (!isEmbeddedDaemonRunning()) ok = false; + bool ok = false; + try { + // Stop the node fully so it releases the datadir .lock + RPC port before we relaunch. + stopEmbeddedDaemon(); + for (int i = 0; i < 60 && isEmbeddedDaemonRunning(); ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(500)); + if (needRescan && daemon_controller_) daemon_controller_->setRescanOnNextStart(true); + ok = shutting_down_ ? true : startEmbeddedDaemon(); + // A missing/corrupt wallet makes dragonxd exit during init — confirm the process survived a + // moment; if not, the wallet is bad. (A valid launch keeps the process alive while it syncs.) + if (ok && !shutting_down_) { + std::this_thread::sleep_for(std::chrono::milliseconds(1500)); + if (!isEmbeddedDaemonRunning()) ok = false; + } + } catch (...) { + ok = false; // a throw during stop/start is a failed switch — revert rather than wedge } if (ok) { daemon_restarting_ = false; // re-arm reconnect once the new daemon is up } else { // Leave the reconnect GATED (daemon_restarting_ stays true) and hand the revert to the main - // thread — settings writes + vault re-scope must not run on this worker thread. + // thread — settings writes + vault re-scope must not run on this worker thread. The revert + // re-arms the gate, so it can never get stuck true. wallet_switch_failed_.store(true); } }); @@ -2574,6 +2581,9 @@ void App::resetChatSession() chat_identity_provisioned_ = false; chat_identity_fetch_in_flight_ = false; chat_identity_unavailable_ = false; + // Invalidate any in-flight identity fetch that captured the PREVIOUS wallet's secret, so its + // completion callback can't provision that secret under the new wallet. + ++chat_session_generation_; } void App::maybeProvisionChatIdentity() @@ -2611,8 +2621,9 @@ void App::maybeProvisionChatIdentity() if (!state_.connected || !rpc_ || !worker_) return; if (!state_.encryption_state_known) return; // don't fetch before we know the lock state const std::string fallbackZaddr = chatReplyZaddr(); // main thread: stable identity source + const int fetchGen = chat_session_generation_; // wallet epoch this secret belongs to chat_identity_fetch_in_flight_ = true; - worker_->post([this, fallbackZaddr]() -> rpc::RPCWorker::MainCb { + worker_->post([this, fallbackZaddr, fetchGen]() -> rpc::RPCWorker::MainCb { std::string secret; // the mnemonic (portable) OR a spending key (legacy fallback) bool transientFail = false; // connection blip — allow a later retry bool unavailable = false; // no usable secret — stop retrying this session @@ -2642,7 +2653,14 @@ void App::maybeProvisionChatIdentity() } catch (const std::exception&) { transientFail = true; } - return [this, secret = std::move(secret), transientFail, unavailable]() mutable { + return [this, secret = std::move(secret), transientFail, unavailable, fetchGen]() mutable { + // The wallet changed while this fetch ran (resetChatSession bumped the generation): this + // secret belongs to the PREVIOUS wallet — wipe it and do nothing so it can't provision an + // identity under the new wallet. Don't touch the flags either (they belong to the new epoch). + if (fetchGen != chat_session_generation_) { + if (!secret.empty()) sodium_memzero(&secret[0], secret.size()); + return; + } chat_identity_fetch_in_flight_ = false; if (unavailable) chat_identity_unavailable_ = true; // Provision only on success AND while still unlocked: a lock (e.g. auto-lock) can @@ -3496,6 +3514,10 @@ void App::beginAdoptSeedWallet() // Adopting swaps in a brand-new seed wallet — drop the legacy wallet's chat identity + store so // it re-derives from the new seed (the file name is unchanged, so nothing else detects the swap). resetChatSession(); + // The PIN vault is name-scoped, and the file name is unchanged, so the legacy wallet's stored + // passphrase would otherwise stay associated with the new seed wallet — remove it (the new wallet + // has its own passphrase; the user can re-enable PIN quick-unlock for it). + if (vault_) vault_->removeVault(); const std::string base = seed_migration_temp_dir_; async_tasks_.submit("Adopt seed wallet", [this, base](const util::AsyncTaskManager::Token&) { namespace fs = std::filesystem; diff --git a/src/app_security.cpp b/src/app_security.cpp index cb7f65c..e493162 100644 --- a/src/app_security.cpp +++ b/src/app_security.cpp @@ -247,12 +247,16 @@ void App::restartDaemonAfterEncryption(const char* taskName, bool announceRestar // Give daemon a moment to shut down, then restart // (do this off the main thread to avoid stalling the UI) async_tasks_.submit(taskName, [this](const util::AsyncTaskManager::Token& token) { - for (int i = 0; i < 20 && !token.cancelled() && !shutting_down_; ++i) - std::this_thread::sleep_for(std::chrono::milliseconds(100)); - if (token.cancelled() || shutting_down_) { daemon_restarting_ = false; return; } - stopEmbeddedDaemon(); - if (token.cancelled() || shutting_down_) { daemon_restarting_ = false; return; } - startEmbeddedDaemon(); + // daemon_restarting_ MUST be cleared on every exit (incl. an early-out or a throw), else it + // stays stuck true and wedges reconnect + all future switch/rescan/encryption operations. + try { + for (int i = 0; i < 20 && !token.cancelled() && !shutting_down_; ++i) + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + if (!token.cancelled() && !shutting_down_) { + stopEmbeddedDaemon(); + if (!token.cancelled() && !shutting_down_) startEmbeddedDaemon(); + } + } catch (...) {} daemon_restarting_ = false; // re-arm reconnect (tryConnect runs from the update loop) }); } else { diff --git a/src/util/secure_vault.cpp b/src/util/secure_vault.cpp index 04ea30a..34b4dc6 100644 --- a/src/util/secure_vault.cpp +++ b/src/util/secure_vault.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include "../util/logger.h" @@ -45,13 +46,20 @@ SecureVault::~SecureVault() = default; void SecureVault::setWalletScope(const std::string& walletFile) { // The default wallet keeps the legacy global "vault.dat" (empty scope) for backward compatibility; - // any other wallet gets a filesystem-safe per-wallet tag so its vault is separate. + // any other wallet gets a per-wallet tag. A readable sanitized prefix PLUS an 8-hex FNV-1a of the + // RAW filename — the hash disambiguates so two distinct files can never share a vault even if their + // sanitized forms collide (e.g. "wallet foo.dat" vs "wallet_foo.dat"). if (walletFile.empty() || walletFile == "wallet.dat") { scope_.clear(); return; } std::string s; s.reserve(walletFile.size()); for (unsigned char c : walletFile) s += (std::isalnum(c) || c == '-' || c == '_') ? static_cast(c) : '_'; - scope_ = s; + uint64_t h = 1469598103934665603ULL; // FNV-1a of the raw name + for (unsigned char c : walletFile) { h ^= c; h *= 1099511628211ULL; } + char suffix[9]; + std::snprintf(suffix, sizeof(suffix), "%08x", static_cast(h & 0xffffffffu)); + if (s.size() > 48) s.resize(48); + scope_ = s + "-" + suffix; } std::string SecureVault::getVaultPath() const { diff --git a/tests/test_phase4.cpp b/tests/test_phase4.cpp index b42b904..ebc317f 100644 --- a/tests/test_phase4.cpp +++ b/tests/test_phase4.cpp @@ -6,6 +6,7 @@ #include "data/transaction_history_cache.h" #include "data/address_book.h" #include "data/wallet_index.h" +#include "util/secure_vault.h" #include "daemon/lifecycle_adapters.h" #include "data/wallet_state.h" #include "rpc/connection.h" @@ -2139,6 +2140,34 @@ void testOperationStatusPollParsing() EXPECT_TRUE(malformed.staleOpids.empty()); } +void testSecureVaultScope() +{ + using dragonx::util::SecureVault; + auto endsWith = [](const std::string& s, const std::string& suf) { + return s.size() >= suf.size() && s.compare(s.size() - suf.size(), suf.size(), suf) == 0; + }; + + // Default wallet (empty or "wallet.dat") keeps the legacy global vault.dat for back-compat. + EXPECT_TRUE(endsWith(SecureVault("").getVaultPath(), "vault.dat")); + EXPECT_TRUE(endsWith(SecureVault("wallet.dat").getVaultPath(), "vault.dat")); + + // A non-default wallet gets its own, distinct vault (not the legacy path). + SecureVault vb("wallet-savings.dat"); + const std::string pb = vb.getVaultPath(); + EXPECT_TRUE(pb.find("vault-") != std::string::npos); + EXPECT_TRUE(pb != SecureVault("").getVaultPath()); + EXPECT_TRUE(pb != SecureVault("wallet-cold.dat").getVaultPath()); + + // The FNV suffix disambiguates sanitize-colliding names, so distinct files never share a vault. + EXPECT_TRUE(SecureVault("wallet foo.dat").getVaultPath() != SecureVault("wallet_foo.dat").getVaultPath()); + + // Re-scoping moves the path; scoping back to the default returns to vault.dat. + vb.setWalletScope("wallet-cold.dat"); + EXPECT_TRUE(vb.getVaultPath() == SecureVault("wallet-cold.dat").getVaultPath()); + vb.setWalletScope("wallet.dat"); + EXPECT_TRUE(endsWith(vb.getVaultPath(), "vault.dat")); +} + void testWalletSecurityController() { using dragonx::services::WalletSecurityController; @@ -6390,6 +6419,7 @@ int main() testNetworkRefreshRpcCollectors(); testNetworkRefreshResultModels(); testOperationStatusPollParsing(); + testSecureVaultScope(); testWalletSecurityController(); testWalletSecurityWorkflow(); testWalletSecurityWorkflowExecutor();