wallet: Include a signature with encrypted keys to mitigate a wallet scam #25766
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
1 similar comment
|
Concept ACK |
7edf8d6 to
75742f2
Compare
|
Concept ACK |
| // Rewrite these encrypted keys with checksums | ||
| batch.WriteCryptedKey(vchPubKey, vchCryptedSecret, mapKeyMetadata[vchPubKey.GetID()]); | ||
| // Rewrite these encrypted keys with checksums and signatures | ||
| uint256 enckey_hash = Hash(vchCryptedSecret); | ||
| std::vector<unsigned char> sig(64); | ||
| if (!key.SignSchnorr(enckey_hash, sig, nullptr, uint256())) { | ||
| keyFail = true; | ||
| break; | ||
| } | ||
| batch.WriteCryptedKey(vchPubKey, vchCryptedSecret, mapKeyMetadata[vchPubKey.GetID()], sig); |
There was a problem hiding this comment.
Wondering if this change would better fit into the later commit 20451e7 ("wallet: Write encrypted key sig if it is not present"), where the same is (only) done for DescriptorScriptPubKeyMan?
There was a problem hiding this comment.
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.
|
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). |
75742f2 to
c966a70
Compare
c966a70 to
0267075
Compare
0267075 to
1a5cebe
Compare
1a5cebe to
1c82695
Compare
1c82695 to
2963813
Compare
2963813 to
19900f0
Compare
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.
In DescriptorScriptPubKeyMan, we now set m_decryption_thoroughly_checked to false when the encrypted key signature is not present.
Instead of copying and overwriting wallet files directly, we can use the backupwallet and restorewallet RPCs.
19900f0 to
76c5cb6
Compare
76c5cb6 to
e6b2c43
Compare
This PR adds a signature to the encrypted key records (
ckeyandwalletdescriptorckey) 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.