From 1a0e40127c14cca37a430879bbe5f8b6ef896c53 Mon Sep 17 00:00:00 2001 From: UdjinM6 Date: Wed, 5 Nov 2025 22:20:30 +0300 Subject: [PATCH] fix: improve wallet encryption robustness Add defensive checks to prevent double-encryption of HD chains and improve wallet encryption robustness: - Check IsCrypted() status to prevent re-encryption attempts - Add TOCTOU protection by re-checking IsCrypted() under lock - Validate decryption keys before TopUp operations - Only decrypt HD chains when actually encrypted - Remove irrelevant HasEncryptionKeys() checks (keys passed as parameters) - Log warning when db rewrite fails after encryption --- src/wallet/scriptpubkeyman.cpp | 54 +++++++++++++++++++--------------- src/wallet/wallet.cpp | 15 ++++++++-- 2 files changed, 44 insertions(+), 25 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 37cf86672cdeb..45d8fe671f9f2 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -244,10 +244,14 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat return false; } - // must get current HD chain before EncryptKeys CHDChain hdChainCurrent; GetHDChain(hdChainCurrent); + if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) { + encrypted_batch = nullptr; + return false; + } + KeyMap keys_to_encrypt; keys_to_encrypt.swap(mapKeys); // Clear mapKeys so AddCryptedKeyInner will succeed. for (const KeyMap::value_type& mKey : keys_to_encrypt) @@ -340,10 +344,13 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata() CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { - return DecryptHDChain(encryption_key, hdChainCurrent); - })) { - throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + + if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { + throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } } CExtKey masterKey; @@ -475,10 +482,12 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) const return false; } - if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { - return DecryptHDChain(encryption_key, hdChainTmp); - })) { - return false; + if (!hdChainTmp.IsNull() && hdChainTmp.IsCrypted()) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { + return false; + } } // make sure seed matches this chain @@ -493,12 +502,9 @@ bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) const bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& chain) { LOCK(cs_KeyStore); - // should call EncryptKeys first - if (!m_storage.HasEncryptionKeys()) - return false; if (chain.IsCrypted()) - return true; + return false; // make sure seed matches this chain if (chain.GetID() != chain.GetSeedHash()) @@ -541,8 +547,6 @@ bool LegacyScriptPubKeyMan::EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, bool LegacyScriptPubKeyMan::DecryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& hdChainRet) const { LOCK(cs_KeyStore); - if (!m_storage.HasEncryptionKeys()) - return true; if (m_hd_chain.IsNull()) return false; @@ -1170,10 +1174,12 @@ bool LegacyScriptPubKeyMan::GetKey(const CKeyID &address, CKey& keyOut) const CHDChain hdChainCurrent; if (!GetHDChain(hdChainCurrent)) throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); - if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { - return DecryptHDChain(encryption_key, hdChainCurrent); - })) { - throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + if (!hdChainCurrent.IsNull() && hdChainCurrent.IsCrypted()) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainCurrent); + })) { + throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } } // make sure seed matches this chain if (hdChainCurrent.GetID() != hdChainCurrent.GetSeedHash()) @@ -1285,10 +1291,12 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& throw std::runtime_error(std::string(__func__) + ": GetHDChain failed"); } - if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { - return DecryptHDChain(encryption_key, hdChainTmp); - })) { - throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + if (!hdChainTmp.IsNull() && hdChainTmp.IsCrypted()) { + if (!m_storage.WithEncryptionKey([&](const CKeyingMaterial& encryption_key) { + return DecryptHDChain(encryption_key, hdChainTmp); + })) { + throw std::runtime_error(std::string(__func__) + ": DecryptHDChain failed"); + } } // make sure seed matches this chain if (hdChainTmp.GetID() != hdChainTmp.GetSeedHash()) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1caf30747a3b0..32d988de32bd8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -720,6 +720,12 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) { LOCK(cs_wallet); + + if (IsCrypted()) { + // verify again now that cs_wallet lock is held + return false; + } + mapMasterKeys[++nMasterKeyMaxID] = kMasterKey; WalletBatch* encrypted_batch = new WalletBatch(GetDatabase()); if (!encrypted_batch->TxnBegin()) { @@ -766,6 +772,10 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) } else if (auto spk_man = GetLegacyScriptPubKeyMan()) { // if we are not using HD, generate new keypool if (spk_man->IsHDEnabled()) { + // NOTE: using internal master key which should be populated on Unlock() + if (!spk_man->CheckDecryptionKey(vMasterKey)) { + return false; + } if (!spk_man->TopUp()) { return false; } @@ -779,8 +789,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) // Need to completely rewrite the wallet file; if we don't, bdb might keep // bits of the unencrypted private key in slack space in the database file. - GetDatabase().Rewrite(); - + if (!GetDatabase().Rewrite()) { + WalletLogPrintf("WARNING: Rewrite failed - wallet is in dangerous state!\n"); + } // BDB seems to have a bad habit of writing old data into // slack space in .dat files; that is bad if the old data is // unencrypted private keys. So: