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

achow101 commented at 9:17 PM on August 1, 2022: member

This PR adds a signature to the encrypted key records (ckey and walletdescriptorckey) which acts as an additional checksum. The signature is produced by the private key, and signs the encrypted private key data. It is a schnorr signature. The signature is verified when the encrypted key record is loaded, and if it fails to verify then the loading fails with a wallet corrupted error.

The purpose of doing this is to mitigate a common scam where users are given/sold a wallet file that contains "encrypted" private keys that correspond to several high valued UTXOs. However the "encrypted" keys are not actually encrypted, they are just garbage data. The signature allows the wallet to ensure that the private keys for the stated pubkeys was known at the time of encryption, so it should help mitigate this scam by making it harder for scammers to make high value UTXOs appear to be IsMine.

There is, of course, a downgrade attack where the scammer can continue to do this with a wallet that does not have signatures over the encrypted keys. To mitigate this, the user will get a warning when they open a wallet that has encrypted keys without signatures. When the wallet is next unlocked, the signatures will be generated and written to the wallet file.

fanquake added the label Wallet on Aug 1, 2022

DrahtBot commented at 3:01 AM on August 2, 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
Concept ACK	1440000bytes, benthecarman, theStack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#25297 (wallet: group independent db writes on single batched db transaction by furszy)
#24914 (wallet: Load database records in a particular order by achow101)
#22341 (rpc: add getxpub by Sjors)
#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

ghost commented at 3:18 AM on August 2, 2022: none

Concept ACK

benthecarman commented at 4:19 AM on August 2, 2022: contributor

Concept ACK

achow101 force-pushed on Aug 2, 2022

theStack commented at 1:13 PM on August 7, 2022: contributor

Concept ACK

in src/wallet/walletdb.cpp:526 in 7cd6ff5636 outdated

 513 | +                    std::vector<unsigned char> sig;
 514 | +                    ssValue >> sig;
 515 | +                    // Our checksum is already the hash of the entire private key
 516 | +                    // so we can just use it as our message
 517 | +                    XOnlyPubKey xonly{vchPubKey};
 518 | +                    if (!xonly.VerifySchnorr(checksum, sig)) {

theStack commented at 3:04 PM on August 7, 2022:

If the deserialized signature doesn't have a size of 64 bytes, it would lead to a crash as the following assertion in XOnlyPubKey::VerifySchnorr will fail, https://github.com/bitcoin/bitcoin/blob/b1a2021f78099c17360dc2179cbcb948059b5969/src/pubkey.cpp#L208 i.e. a signature size check before verifying should be added. As the same is needed below for WALLETDESCRIPTORCKEY, probably it would make sense to put the verification (including signature size check) into a helper function to deduplicate?

achow101 commented at 11:00 PM on January 24, 2023:

Added that as well deduplicated.

in src/wallet/scriptpubkeyman.cpp:259 in 7cd6ff5636 outdated

 253 | +                std::vector<unsigned char> sig(64);
 254 | +                if (!key.SignSchnorr(enckey_hash, sig, nullptr, uint256())) {
 255 | +                    keyFail = true;
 256 | +                    break;
 257 | +                }
 258 | +                batch.WriteCryptedKey(vchPubKey, vchCryptedSecret, mapKeyMetadata[vchPubKey.GetID()], sig);

theStack commented at 3:09 PM on August 7, 2022:

Wondering if this change would better fit into the later commit 20451e785f6478b5daf3b06bb2f638bc3662cf9f ("wallet: Write encrypted key sig if it is not present"), where the same is (only) done for DescriptorScriptPubKeyMan?

achow101 commented at 11:01 PM on January 24, 2023:

No, it cannot be moved to later since WriteCryptedKey also changes in this commit. The later commit is for DescriptorSPKM only because it doesn't already have an existing upgrading mechanism that needed changes.

luke-jr commented at 12:34 AM on August 10, 2022: member

I suspect this might actually make the problem worse. With this, a degree of trust in wallets having valid encrypted keys could develop, and it doesn't actually prevent the scam (the scammer just needs access to sufficient funds which remain not at risk).

DrahtBot added the label Needs rebase on Sep 13, 2022

achow101 force-pushed on Sep 19, 2022

DrahtBot removed the label Needs rebase on Sep 19, 2022

achow101 force-pushed on Sep 20, 2022

bitcoin deleted a comment on Nov 19, 2022

DrahtBot added the label Needs rebase on Nov 30, 2022

achow101 force-pushed on Nov 30, 2022

DrahtBot removed the label Needs rebase on Nov 30, 2022

DrahtBot added the label Needs rebase on Dec 5, 2022

achow101 force-pushed on Dec 6, 2022

DrahtBot removed the label Needs rebase on Dec 6, 2022

DrahtBot added the label Needs rebase on Jan 3, 2023

walletdb: Add walletdescriptor(c)key to IsKeyType b36796bfae

achow101 force-pushed on Jan 3, 2023

DrahtBot removed the label Needs rebase on Jan 3, 2023

achow101 force-pushed on Jan 24, 2023

wallet: Write encrypted keys with a signature

In order to mitigate scams, we write a signature created by the now
encrypted key, with a message of the encrypted key data. This signature
will be verified on loading.

a3b2bc2e91

wallet: Set m_decryption_thoroughly_checked based on checksums

In DescriptorScriptPubKeyMan, we now set m_decryption_thoroughly_checked
to false when the encrypted key signature is not present.

6510405d8a

wallet: Write encrypted key sig if it is not present f62f2ce2da

wallet: Warn when encrypted key sigs are missing ca8ad28dd9

test: Use backupwallet and restorewallet for wallet_upgradewallet

Instead of copying and overwriting wallet files directly, we can use the
backupwallet and restorewallet RPCs.

e4fa83d14b

achow101 force-pushed on Jan 25, 2023

test: Test that encrypted wallets are upgraded with sigs e6b2c43676

achow101 force-pushed on Jan 25, 2023

achow101 closed this on Apr 25, 2023

bitcoin locked this on Apr 24, 2024