From 6301cd5984bcaa2ba618f0fc2e78fbcbd79e95f9 Mon Sep 17 00:00:00 2001 From: DanS Date: Mon, 19 Oct 2026 18:34:59 -0500 Subject: [PATCH] fix(security): synchronize SecureVault::scope_ between the main thread and worker security ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrency audit: SecureVault::scope_ (std::string) is written by setWalletScope() on the main thread (wallet switch) while getVaultPath() — reached from retrieve()/hasVault()/changePin()/store() — runs on RPC worker threads during a send-time or startup unlock. A concurrent wallet switch and a worker vault op is a data race on the string (and could resolve the wrong vault file). Guard scope_ with a small mutex: setWalletScope() assigns under it; getVaultPath() copies it out under the lock and uses the copy. The lock is held only for the string copy — never across Argon2id or file I/O — and getVaultPath() never calls setWalletScope(), so no deadlock. Argon2id params, key derivation, AEAD, the vault file format and retrieve/store/changePin are byte-identical. SecureVault is held via unique_ptr and used by pointer, so the added (non-movable) mutex is fine. Synchronization only. Threading fix — clean build + ctest; the race is runtime-only. Co-Authored-By: Claude Opus 4.8 --- src/util/secure_vault.cpp | 17 ++++++++++++++--- src/util/secure_vault.h | 4 ++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/util/secure_vault.cpp b/src/util/secure_vault.cpp index c40d29b..1f40a5a 100644 --- a/src/util/secure_vault.cpp +++ b/src/util/secure_vault.cpp @@ -48,7 +48,11 @@ void SecureVault::setWalletScope(const std::string& walletFile) { // 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; } + if (walletFile.empty() || walletFile == "wallet.dat") { + std::lock_guard lk(scopeMutex_); + scope_.clear(); + return; + } std::string s; s.reserve(walletFile.size()); for (unsigned char c : walletFile) @@ -58,11 +62,18 @@ void SecureVault::setWalletScope(const std::string& walletFile) { 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 newScope = s + "-" + suffix; + std::lock_guard lk(scopeMutex_); + scope_ = std::move(newScope); } std::string SecureVault::getVaultPath() const { - const std::string name = scope_.empty() ? std::string("vault.dat") : ("vault-" + scope_ + ".dat"); + std::string scope; + { + std::lock_guard lk(scopeMutex_); + scope = scope_; + } + const std::string name = scope.empty() ? std::string("vault.dat") : ("vault-" + scope + ".dat"); return (fs::path(Platform::getConfigDir()) / name).string(); } diff --git a/src/util/secure_vault.h b/src/util/secure_vault.h index a2f3fbc..f0b5897 100644 --- a/src/util/secure_vault.h +++ b/src/util/secure_vault.h @@ -7,6 +7,7 @@ #include #include #include +#include namespace dragonx { namespace util { @@ -101,6 +102,9 @@ private: const uint8_t* salt, size_t saltLen, uint8_t* key, size_t keyLen) const; + // Guards scope_: setWalletScope() runs on the main thread (wallet switch) while retrieve()/hasVault()/ + // changePin() run on RPC worker threads and read it via getVaultPath(). + mutable std::mutex scopeMutex_; std::string scope_; // "" = default/legacy wallet (vault.dat); else a sanitized wallet-file tag };