From 349ed2a0eed3aaaf199ead93057c97730869c3a3 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 8 Nov 2022 01:03:07 +0100 Subject: [PATCH 1/2] wallet: throw error if legacy entries are present on loading descriptor wallets In the wallet key-value-loading routine, most legacy type entries require a LegacyScriptPubKeyMan instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. Fix this by throwing an error if if the wallet flags indicate that we have a descriptor wallet and there is a legacy entry found. --- src/wallet/wallet.cpp | 4 ++++ src/wallet/walletdb.cpp | 12 ++++++++++++ src/wallet/walletdb.h | 3 ++- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 431e970edc1..e0a3afcb944 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2919,6 +2919,10 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri "The wallet might had been created on a newer version.\n" "Please try running the latest software version.\n"), walletFile); return nullptr; + } else if (nLoadWalletRet == DBErrors::UNEXPECTED_LEGACY_ENTRY) { + error = strprintf(_("Unexpected legacy entry in descriptor wallet found. Loading wallet %s\n\n" + "The wallet might have been tampered with or created with malicious intent.\n"), walletFile); + return nullptr; } else { error = strprintf(_("Error loading %s"), walletFile); return nullptr; diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 6a8f0d24817..3e4d2d64d28 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -315,6 +315,7 @@ public: std::map m_hd_chains; bool tx_corrupt{false}; bool descriptor_unknown{false}; + bool unexpected_legacy_entry{false}; CWalletScanState() = default; }; @@ -332,6 +333,11 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, if (filter_fn && !filter_fn(strType)) { return true; } + // Legacy entries in descriptor wallets are not allowed, abort immediately + if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && DBKeys::LEGACY_TYPES.count(strType) > 0) { + wss.unexpected_legacy_entry = true; + return false; + } if (strType == DBKeys::NAME) { std::string strAddress; ssKey >> strAddress; @@ -833,6 +839,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) std::string strType, strErr; if (!ReadKeyValue(pwallet, ssKey, ssValue, wss, strType, strErr)) { + if (wss.unexpected_legacy_entry) { + strErr = strprintf("Error: Unexpected legacy entry found in descriptor wallet %s. ", pwallet->GetName()); + strErr += "The wallet might have been tampered with or created with malicious intent."; + pwallet->WalletLogPrintf("%s\n", strErr); + return DBErrors::UNEXPECTED_LEGACY_ENTRY; + } // losing keys is considered a catastrophic error, anything else // we assume the user can live with: if (IsKeyType(strType) || strType == DBKeys::DEFAULTKEY) { diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index da6efe534b5..27b5dbdd963 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -52,7 +52,8 @@ enum class DBErrors LOAD_FAIL, NEED_REWRITE, NEED_RESCAN, - UNKNOWN_DESCRIPTOR + UNKNOWN_DESCRIPTOR, + UNEXPECTED_LEGACY_ENTRY }; namespace DBKeys { From 3198e4239e848bbb119e3638677aa9bcf8353ca6 Mon Sep 17 00:00:00 2001 From: Sebastian Falbesoner Date: Tue, 8 Nov 2022 01:24:27 +0100 Subject: [PATCH 2/2] test: check that loading descriptor wallet with legacy entries throws error --- test/functional/wallet_descriptor.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py index 5dc23ba2455..295a8416ba9 100755 --- a/test/functional/wallet_descriptor.py +++ b/test/functional/wallet_descriptor.py @@ -3,6 +3,8 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test descriptor wallet function.""" +import os +import sqlite3 from test_framework.blocktools import COINBASE_MATURITY from test_framework.test_framework import BitcoinTestFramework @@ -209,5 +211,15 @@ class WalletDescriptorTest(BitcoinTestFramework): imp_addr = imp_rpc.getnewaddress(address_type=addr_type) assert_equal(exp_addr, imp_addr) + self.log.info("Test that loading descriptor wallet containing legacy key types throws error") + self.nodes[0].createwallet(wallet_name="crashme", descriptors=True) + self.nodes[0].unloadwallet("crashme") + wallet_db = os.path.join(self.nodes[0].datadir, self.chain, "wallets", "crashme", self.wallet_data_filename) + with sqlite3.connect(wallet_db) as conn: + # add "cscript" entry: key type is uint160 (20 bytes), value type is CScript (zero-length here) + conn.execute('INSERT INTO main VALUES(?, ?)', (b'\x07cscript' + b'\x00'*20, b'\x00')) + assert_raises_rpc_error(-4, "Unexpected legacy entry in descriptor wallet found.", self.nodes[0].loadwallet, "crashme") + + if __name__ == '__main__': WalletDescriptorTest().main ()