diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 23e2257b1e7..cc115008442 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); @@ -2411,6 +2417,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 d8b6c90178a..9b62a35beda 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() {} @@ -473,6 +474,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; @@ -661,6 +663,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 bc22dcd7040..e7e06aec01a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3712,6 +3712,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 c6a45e9a148..f8b914dd8ff 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -973,6 +973,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) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 368415da129..801c2ab97b8 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 32c3c29b5e3..70d69870126 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -243,6 +243,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); 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()