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

theStack · 2022-11-06T14:36:17Z

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):

$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme

--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900

Segmentation 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:

bitcoin/src/wallet/walletdb.cpp

Lines 589 to 594 in 50422b7

    
           } else if (strType == DBKeys::CSCRIPT) { 
        
               uint160 hash; 
        
               ssKey >> hash; 
        
               CScript script; 
        
               ssValue >> script; 
        
               if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCScript(script))

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.

achow101 · 2022-11-06T15:19:34Z

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.

theStack · 2022-11-06T17:35:56Z

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 · 2022-11-06T23:42:12Z

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 · 2022-11-07T13:22:09Z

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 · 2022-11-07T23:29:26Z

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.

…or 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.

… error

theStack · 2022-11-08T11:31:45Z

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 · 2022-11-08T14:34:55Z

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 · 2022-11-09T21:33:33Z

ACK 3198e42

aureleoules

ACK 3198e42

Tested with the steps in the PR description:

$ ./src/bitcoin-cli -regtest createwallet crashme
$ ./src/bitcoin-cli -regtest unloadwallet crashme
$ sqlite3 ~/.bitcoin/regtest/wallets/crashme/wallet.dat
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');

$ ./src/bitcoin-cli -regtest loadwallet crashme 
error code: -4
error message:
Wallet loading failed. Unexpected legacy entry in descriptor wallet found. Loading wallet /home/aureleoules/.bitcoin/regtest/wallets/crashme/wallet.dat

The wallet might have been tampered with or created with malicious intent.

aureleoules · 2022-11-22T18:17:23Z

src/wallet/walletdb.cpp

+                    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);


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

Suggested change

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);

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());

…ontaining legacy key type entries 3198e42 test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner) 349ed2a wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner) Pull request description: 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): ``` $ ./src/bitcoin-cli createwallet crashme $ ./src/bitcoin-cli unloadwallet crashme $ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat SQLite version 3.38.2 2022-03-26 13:51:10 Enter ".help" for usage hints. sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00'); $ ./src/bitcoin-cli loadwallet crashme --- bitcoind output: --- 2022-11-06T13:51:01Z Using SQLite Version 3.38.2 2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme 2022-11-06T13:51:01Z init message: Loading wallet… 2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900 Segmentation 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. ACKs for top commit: achow101: ACK 3198e42 aureleoules: ACK 3198e42 Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c

hebasto · 2023-08-02T18:35:31Z

The test is broken on Windows:

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

Fixed in #28204.

703b758 qa: Close SQLite connection properly (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#26462 that introduced a bug on Windows: ``` >test\functional\wallet_descriptor.py ... PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ... ``` From `sqlite3` Python module [docs](https://docs.python.org/3/library/sqlite3.html#how-to-use-the-connection-context-manager): > `Connection` object used as context manager only commits or rollbacks transactions, so the connection object should be closed manually. ACKs for top commit: MarcoFalke: lgtm ACK 703b758 theStack: utACK 703b758 Tree-SHA512: 35b1403507be06d1fc04e7e07ff56af5bcfe5013024671f0c1d9f3c41aacc4c777bcc6376ce82d720394e27450415d50ff5d5834ed388ec3f21503f86f1a42a5

703b758 qa: Close SQLite connection properly (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin#26462 that introduced a bug on Windows: ``` >test\functional\wallet_descriptor.py ... PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ... ``` From `sqlite3` Python module [docs](https://docs.python.org/3/library/sqlite3.html#how-to-use-the-connection-context-manager): > `Connection` object used as context manager only commits or rollbacks transactions, so the connection object should be closed manually. ACKs for top commit: MarcoFalke: lgtm ACK 703b758 theStack: utACK 703b758 Tree-SHA512: 35b1403507be06d1fc04e7e07ff56af5bcfe5013024671f0c1d9f3c41aacc4c777bcc6376ce82d720394e27450415d50ff5d5834ed388ec3f21503f86f1a42a5

theStack force-pushed the 202211-wallet_fix_crash_on_descriptor_wallet_load branch from f938a1d to 42d61d2 Compare November 6, 2022 14:49

DrahtBot added the Wallet label Nov 6, 2022

This was referenced Nov 7, 2022

wallet: rpc to add automatically generated descriptors #25907

Closed

wallet: Load database records in a particular order #24914

Merged

theStack force-pushed the 202211-wallet_fix_crash_on_descriptor_wallet_load branch from 42d61d2 to 3c6ff49 Compare November 8, 2022 11:20

theStack added 2 commits November 8, 2022 12:29

test: check that loading descriptor wallet with legacy entries throws…

3198e42

… error

theStack force-pushed the 202211-wallet_fix_crash_on_descriptor_wallet_load branch from 3c6ff49 to 3198e42 Compare November 8, 2022 11:29

This was referenced Nov 10, 2022

wallet: Include a signature with encrypted keys to mitigate a wallet scam #25766

Closed

refactor: Use util::Result class for wallet loading #25722

Draft

[Draft / POC] Silent Payments #24897

Closed

aureleoules approved these changes Nov 22, 2022

View reviewed changes

aureleoules reviewed Nov 22, 2022

View reviewed changes

DrahtBot mentioned this pull request Nov 30, 2022

rpc, wallet: addhdseed, infer seed when importing descriptor with xpub #23544

Closed

achow101 merged commit 2ce3d26 into bitcoin:master Dec 5, 2022

theStack deleted the 202211-wallet_fix_crash_on_descriptor_wallet_load branch December 5, 2022 22:42

hebasto mentioned this pull request Aug 2, 2023

qa: Close SQLite connection properly #28204

Merged

bitcoin locked and limited conversation to collaborators Aug 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

theStack commented Nov 6, 2022 •

edited

Loading

Uh oh!

achow101 commented Nov 6, 2022

Uh oh!

theStack commented Nov 6, 2022

Uh oh!

DrahtBot commented Nov 6, 2022 •

edited

Loading

Uh oh!

furszy commented Nov 7, 2022 •

edited

Loading

Uh oh!

luke-jr commented Nov 7, 2022

Uh oh!

theStack commented Nov 8, 2022

Uh oh!

furszy commented Nov 8, 2022 •

edited

Loading

Uh oh!

achow101 commented Nov 9, 2022

Uh oh!

aureleoules left a comment •

edited

Loading

Uh oh!

aureleoules Nov 22, 2022

Uh oh!

hebasto commented Aug 2, 2023

Uh oh!

Uh oh!

	} else if (strType == DBKeys::CSCRIPT) {
	uint160 hash;
	ssKey >> hash;
	CScript script;
	ssValue >> script;
	if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadCScript(script))

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

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

Uh oh!

Conversation

theStack commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Nov 6, 2022

Uh oh!

theStack commented Nov 6, 2022

Uh oh!

DrahtBot commented Nov 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

furszy commented Nov 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Nov 7, 2022

Uh oh!

theStack commented Nov 8, 2022

Uh oh!

furszy commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Nov 9, 2022

Uh oh!

aureleoules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aureleoules Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 2, 2023

Uh oh!

Uh oh!

theStack commented Nov 6, 2022 •

edited

Loading

DrahtBot commented Nov 6, 2022 •

edited

Loading

furszy commented Nov 7, 2022 •

edited

Loading

furszy commented Nov 8, 2022 •

edited

Loading

aureleoules left a comment •

edited

Loading