Merge #6945: fix: improve wallet encryption robustness

116fca6 refactor: drop redundant `!hdchain.IsNull()` checks (UdjinM6)
24670c7 fix: return encryption failure when database rewrite fails (UdjinM6)
98d7a6b fix: improve wallet encryption robustness (UdjinM6)

Pull request description:

  ## Issue being fixed or feature implemented
  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)
  - Fail and log an error when db rewrite fails after encryption

  ## What was done?

  ## How Has This Been Tested?
  run tests

  ## Breaking Changes
  n/a

  ## Checklist:
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  PastaPastaPasta:
    utACK 116fca6

Tree-SHA512: 5659b133ac46d425b1018d8a5e585a1702aeb452f9cb145f3659bdfbf57021b90bc9d52aabcac0dc2406598ee5afa1fee1c2ab29201e49e2c7412321b25aa22c

Author: PastaPastaPasta

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.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.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()) {
         WalletLogPrintf("%s: ERROR: no HD chain\n", __func__);
@@ -1188,10 +1192,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.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())
@@ -1303,10 +1309,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.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())

src/wallet/wallet.cpp

@@ -737,6 +737,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()) {
@@ -783,6 +789,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;
                 }
@@ -796,8 +806,10 @@ 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("ERROR: Rewrite failed - wallet is in dangerous state!\n");
+            return false;
+        }
         // 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: