wallet: fix crash on loading descriptor wallet containing legacy key type entries

theStack commented at 2:36 pm on November 6, 2022: contributor

Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a “cscript” entry (see second commit for details):

 0$ ./src/bitcoin-cli createwallet crashme
 1$ ./src/bitcoin-cli unloadwallet crashme
 2$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
 3SQLite version 3.38.2 2022-03-26 13:51:10
 4Enter ".help" for usage hints.
 5sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
 6$ ./src/bitcoin-cli loadwallet crashme
 7
 8--- bitcoind output: ---
 92022-11-06T13:51:01Z Using SQLite Version 3.38.2
102022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
112022-11-06T13:51:01Z init message: Loading wallet…
122022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
13
14Segmentation fault (core dumped)

Background: 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. E.g. for CSCRIPT: https://github.com/bitcoin/bitcoin/blob/50422b770a40f5fa964201d1e99fd6b5dc1653ca/src/wallet/walletdb.cpp#L589-L594

This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet’s functional test (fortunately Python includes sqlite3 support in the standard library).

Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I’m aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.

This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.

theStack force-pushed on Nov 6, 2022

achow101 commented at 3:19 pm on November 6, 2022: member

Meh. I think it’s reasonable to expect failures if the user is manually modifying their wallet file by doing direct database operations themselves. Since it is completely unexpected to have legacy and descriptor wallet things mixed, I don’t think we should just silently ignore such issues and instead return an error.

DrahtBot added the label Wallet on Nov 6, 2022

theStack commented at 5:35 pm on November 6, 2022: contributor

Meh. I think it’s reasonable to expect failures if the user is manually modifying their wallet file by doing direct database operations themselves. Since it is completely unexpected to have legacy and descriptor wallet things mixed, I don’t think we should just silently ignore such issues and instead return an error.

Right, throwing an error seems more reasonable. Obviously it wouldn’t be the user itself modifying an existing wallet, but e.g. an external malicious actor crafting and handing out these mixed wallets, hoping that users would open them to cause node crashes.

The PR in its current state is just the easiest “avoid crash”-remedy. Will look into your proposed solution in a bit, this probably needs a new DBErrors category.

DrahtBot commented at 11:42 pm on November 6, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, aureleoules
Concept ACK	luke-jr, furszy

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#25766 (wallet: Include a signature with encrypted keys to mitigate a wallet scam by achow101)
#24914 (wallet: Load database records in a particular order by achow101)
#24897 ([Draft / POC] Silent Payments by w0xlt)
#23544 (rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

furszy commented at 1:22 pm on November 7, 2022: member

Concept ACK in terms of not crashing, and instead abort cleanly with a message.

Even when this is an edge case; if people/attacker insert data into the wallet db, then the user might have worst problems then a simple crash… Still, for those users experimenting with our wallet db; any crash could end up corrupting other dbs and indexes that the node has, which would require a fresh resync or reindex etc.. So, better if we don’t crash and just abort cleanly.

Saying that, I would rather focus on getting #24914 merged first. The current load process is a mess, and continue adding stuff on top would just make it messier.

luke-jr commented at 11:29 pm on November 7, 2022: member

Concept ACK on throwing an error.

I would rather focus on getting #24914 merged first. The current load process is a mess, and continue adding stuff on top would just make it messier.

Bugfixes need to be backported, so this should ideally be merged first.

theStack force-pushed on Nov 8, 2022

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.

349ed2a0ee

test: check that loading descriptor wallet with legacy entries throws error 3198e4239e

theStack force-pushed on Nov 8, 2022

theStack commented at 11:31 am on November 8, 2022: contributor

Force-pushed with a new fix that throws an error now, as suggested by reviewers (the old branch with the inferior ignore approach can still be found here). Happy to improve the error message if there is any suggestions.

furszy commented at 2:34 pm on November 8, 2022: member

Concept ACK on throwing an error.

I would rather focus on getting #24914 merged first. The current load process is a mess, and continue adding stuff on top would just make it messier.

Bugfixes need to be backported, so this should ideally be merged first.

True, still I’m not sure that adding a specific DBError is the right approach here. Some sort of db checksum created from one of the wallet encrypted keys might solve tampering attempts in a more general manner. I’m speaking generally because we most likely have other ways to crash/break the wallet at startup by inserting data directly into the db. Like modifying and/or erasing the wallet flags or the min version field, or changing a spkm type which would crash during the GetNewDestination type check assertion.

achow101 commented at 9:33 pm on November 9, 2022: member

ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6

aureleoules approved

aureleoules commented at 6:06 pm on November 22, 2022: member

ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6

Tested with the steps in the PR description:

 0$ ./src/bitcoin-cli -regtest createwallet crashme
 1$ ./src/bitcoin-cli -regtest unloadwallet crashme
 2$ sqlite3 ~/.bitcoin/regtest/wallets/crashme/wallet.dat
 3sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
 4
 5$ ./src/bitcoin-cli -regtest loadwallet crashme 
 6error code: -4
 7error message:
 8Wallet loading failed. Unexpected legacy entry in descriptor wallet found. Loading wallet /home/aureleoules/.bitcoin/regtest/wallets/crashme/wallet.dat
 9
10The wallet might have been tampered with or created with malicious intent.

in src/wallet/walletdb.cpp:845 in 3198e4239e

838@@ -833,6 +839,12 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
839             std::string strType, strErr;
840             if (!ReadKeyValue(pwallet, ssKey, ssValue, wss, strType, strErr))
841             {
842+                if (wss.unexpected_legacy_entry) {
843+                    strErr = strprintf("Error: Unexpected legacy entry found in descriptor wallet %s. ", pwallet->GetName());
844+                    strErr += "The wallet might have been tampered with or created with malicious intent.";
845+                    pwallet->WalletLogPrintf("%s\n", strErr);

aureleoules commented at 6:17 pm on November 22, 2022:

I don’t think there is a need to store the error in strErr?

0                    pwallet->WalletLogPrintf("Error: Unexpected legacy entry found in descriptor wallet %s. The wallet might have been tampered with or created with malicious intent.\n", pwallet->GetName());

achow101 merged this on Dec 5, 2022

achow101 closed this on Dec 5, 2022

theStack deleted the branch on Dec 5, 2022

sidhujag referenced this in commit c9e61cf652 on Dec 6, 2022

hebasto commented at 6:35 pm on August 2, 2023: member

The test is broken on Windows:

0>test\functional\wallet_descriptor.py
1...
2PermissionError: [WinError 32] The process cannot access the file because it is being used by another process:
3...

Fixed in #28204.

wallet: fix crash on loading descriptor wallet containing legacy key type entries #26462

Reviews

Conflicts