From 813a16a463326079472366aed42f5218a57db63b Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 17:44:46 -0400 Subject: [PATCH 1/3] wallet: Add HasCryptedKeys --- src/wallet/scriptpubkeyman.cpp | 12 ++++++++++++ src/wallet/scriptpubkeyman.h | 3 +++ src/wallet/wallet.cpp | 8 ++++++++ src/wallet/wallet.h | 1 + 4 files changed, 24 insertions(+) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 46ec5dc1acc..a37981ff658 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -525,6 +525,12 @@ bool LegacyScriptPubKeyMan::HavePrivateKeys() const return !mapKeys.empty() || !mapCryptedKeys.empty(); } +bool LegacyScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_KeyStore); + return !mapCryptedKeys.empty(); +} + void LegacyScriptPubKeyMan::RewriteDB() { LOCK(cs_KeyStore); @@ -2392,6 +2398,12 @@ bool DescriptorScriptPubKeyMan::HavePrivateKeys() const return m_map_keys.size() > 0 || m_map_crypted_keys.size() > 0; } +bool DescriptorScriptPubKeyMan::HaveCryptedKeys() const +{ + LOCK(cs_desc_man); + return !m_map_crypted_keys.empty(); +} + std::optional DescriptorScriptPubKeyMan::GetOldestKeyPoolTime() const { // This is only used for getwalletinfo output and isn't relevant to descriptor wallets. diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index cf7b7eaf31d..e0601785b03 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -221,6 +221,7 @@ class ScriptPubKeyMan virtual bool Upgrade(int prev_version, int new_version, bilingual_str& error) { return true; } virtual bool HavePrivateKeys() const { return false; } + virtual bool HaveCryptedKeys() const { return false; } //! The action to do when the DB needs rewrite virtual void RewriteDB() {} @@ -472,6 +473,7 @@ class LegacyScriptPubKeyMan : public LegacyDataSPKM bool Upgrade(int prev_version, int new_version, bilingual_str& error) override; bool HavePrivateKeys() const override; + bool HaveCryptedKeys() const override; void RewriteDB() override; @@ -659,6 +661,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked. std::optional GetKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + bool HaveCryptedKeys() const override; std::optional GetOldestKeyPoolTime() const override; unsigned int GetKeyPoolSize() const override; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index de565102cc2..bef42cbdc22 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3695,6 +3695,14 @@ bool CWallet::HasEncryptionKeys() const return !mapMasterKeys.empty(); } +bool CWallet::HaveCryptedKeys() const +{ + for (const auto& spkm : GetAllScriptPubKeyMans()) { + if (spkm->HaveCryptedKeys()) return true; + } + return false; +} + void CWallet::ConnectScriptPubKeyManNotifiers() { for (const auto& spk_man : GetActiveScriptPubKeyMans()) { diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d3a7208b15e..88e4778660b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -969,6 +969,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati bool WithEncryptionKey(std::function cb) const override; bool HasEncryptionKeys() const override; + bool HaveCryptedKeys() const; /** Get last block processed height */ int GetLastBlockHeight() const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet) From 2b9279b50a3682ab97308888b4f272d3ae379811 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 17:45:01 -0400 Subject: [PATCH 2/3] wallet: Remove unused encryption keys from watchonly wallets Due to a bug in earlier versions, some wallets without private keys may have an encryption key. This encryption key is unused and can lead to confusing behavior elsewhere. When such wallets are detected, those encryption keys will now be deleted from the wallet. For safety, we only do this to wallets which have private keys disabled, have encryption keys, and definitely do not have encrypted keys. --- src/wallet/walletdb.cpp | 20 ++++++++++++++++++++ src/wallet/walletdb.h | 1 + 2 files changed, 21 insertions(+) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 597a4ef9a4c..82e73fc2130 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -154,6 +154,11 @@ bool WalletBatch::WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey) return WriteIC(std::make_pair(DBKeys::MASTER_KEY, nID), kMasterKey, true); } +bool WalletBatch::EraseMasterKey(unsigned int id) +{ + return EraseIC(std::make_pair(DBKeys::MASTER_KEY, id)); +} + bool WalletBatch::WriteCScript(const uint160& hash, const CScript& redeemScript) { return WriteIC(std::make_pair(DBKeys::CSCRIPT, hash), redeemScript, false); @@ -1241,6 +1246,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) result = DBErrors::CORRUPT; } + // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is + // such a wallet and remove the encryption key records to avoid any future issues. + // Although wallets without private keys should not have *ckey records, we should double check that. + // Removing the mkey records is only safe if there are no *ckey records. + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) { + pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n"); + for (const auto& [id, _] : pwallet->mapMasterKeys) { + if (!EraseMasterKey(id)) { + pwallet->WalletLogPrintf("Error: Unable to remove extraneous encryption key '%u'. Wallet corrupt.\n", id); + return DBErrors::CORRUPT; + } + } + pwallet->mapMasterKeys.clear(); + } + return result; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index bffcc87202a..08054f42bae 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -238,6 +238,7 @@ class WalletBatch bool WriteKey(const CPubKey& vchPubKey, const CPrivKey& vchPrivKey, const CKeyMetadata &keyMeta); bool WriteCryptedKey(const CPubKey& vchPubKey, const std::vector& vchCryptedSecret, const CKeyMetadata &keyMeta); bool WriteMasterKey(unsigned int nID, const CMasterKey& kMasterKey); + bool EraseMasterKey(unsigned int id); bool WriteCScript(const uint160& hash, const CScript& redeemScript); From 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 24 Oct 2023 18:57:16 -0400 Subject: [PATCH 3/3] tests: Test cleanup of mkeys from wallets without privkeys --- test/functional/wallet_encryption.py | 61 ++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/test/functional/wallet_encryption.py b/test/functional/wallet_encryption.py index 5a5fb3e5be3..b30a1478ab5 100755 --- a/test/functional/wallet_encryption.py +++ b/test/functional/wallet_encryption.py @@ -5,7 +5,9 @@ """Test Wallet encryption""" import time +import subprocess +from test_framework.messages import hash256 from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_raises_rpc_error, @@ -100,6 +102,65 @@ def run_test(self): sig = self.nodes[0].signmessage(address, msg) assert self.nodes[0].verifymessage(address, sig, msg) + self.log.info("Test that wallets without private keys cannot be encrypted") + self.nodes[0].createwallet(wallet_name="noprivs", disable_private_keys=True) + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs") + assert_raises_rpc_error(-16, "Error: wallet does not contain private keys, nothing to encrypt.", noprivs_wallet.encryptwallet, "pass") + + if self.is_wallet_tool_compiled(): + self.log.info("Test that encryption keys in wallets without privkeys are removed") + + def do_wallet_tool(*args): + proc = subprocess.Popen( + [self.options.bitcoinwallet, f"-datadir={self.nodes[0].datadir_path}", f"-chain={self.chain}"] + list(args), + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True + ) + stdout, stderr = proc.communicate() + assert_equal(proc.poll(), 0) + assert_equal(stderr, "") + + # Since it is no longer possible to encrypt a wallet without privkeys, we need to force one into the wallet + # 1. Make a dump of the wallet + # 2. Add mkey record to the dump + # 3. Create a new wallet from the dump + + # Make the dump + noprivs_wallet.unloadwallet() + dumpfile_path = self.nodes[0].datadir_path / "noprivs.dump" + do_wallet_tool("-wallet=noprivs", f"-dumpfile={dumpfile_path}", "dump") + + # Modify the dump + with open(dumpfile_path, "r", encoding="utf-8") as f: + dump_content = f.readlines() + # Drop the checksum line + dump_content = dump_content[:-1] + # Insert a valid mkey line. This corresponds to a passphrase of "pass". + dump_content.append("046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n") + with open(dumpfile_path, "w", encoding="utf-8") as f: + contents = "".join(dump_content) + f.write(contents) + checksum = hash256(contents.encode()) + f.write(f"checksum,{checksum.hex()}\n") + + # Load the dump into a new wallet + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "createfromdump") + # Load the wallet and make sure it is no longer encrypted + with self.nodes[0].assert_debug_log(["Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys."]): + self.nodes[0].loadwallet("noprivs_enc") + noprivs_wallet = self.nodes[0].get_wallet_rpc("noprivs_enc") + assert_raises_rpc_error(-15, "Error: running with an unencrypted wallet, but walletpassphrase was called.", noprivs_wallet.walletpassphrase, "pass", 1) + noprivs_wallet.unloadwallet() + + # Make a new dump and check that there are no mkeys + dumpfile_path = self.nodes[0].datadir_path / "noprivs_enc.dump" + do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "dump") + with open(dumpfile_path, "r", encoding="utf-8") as f: + # Check theres nothing with an 'mkey' prefix + assert_equal(all([not line.startswith("046d6b6579") for line in f]), True) + if __name__ == '__main__': WalletEncryptionTest(__file__).main()