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

pull achow101 wants to merge 7 commits into bitcoin:master from achow101:enckeys-with-sigs changing 9 files +266 −99
  1. 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.

  2. fanquake added the label Wallet on Aug 1, 2022
  3. DrahtBot commented at 3:01 AM on August 2, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

    Concept ACK

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

    Concept ACK

  6. benthecarman cross-referenced this on Aug 2, 2022 from issue August Topics by benthecarman
  7. achow101 force-pushed on Aug 2, 2022
  8. DrahtBot cross-referenced this on Aug 4, 2022 from issue wallet: Load database records in a particular order by achow101
  9. DrahtBot cross-referenced this on Aug 4, 2022 from issue wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101
  10. DrahtBot cross-referenced this on Aug 4, 2022 from issue RPC/Wallet: Add "use_txids" to output of getaddressinfo by luke-jr
  11. DrahtBot cross-referenced this on Aug 4, 2022 from issue wallet: Migrate legacy wallets to descriptor wallets by achow101
  12. theStack commented at 1:13 PM on August 7, 2022: contributor

    Concept ACK

  13. 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.

  14. 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.

  15. 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).

  16. DrahtBot cross-referenced this on Aug 11, 2022 from issue rpc: add path to gethdkey by Sjors
  17. DrahtBot cross-referenced this on Aug 23, 2022 from issue wallet: rpc to add automatically generated descriptors by achow101
  18. DrahtBot cross-referenced this on Sep 6, 2022 from issue wallet: bugfix, load a wallet with an unknown/corrupt descriptor causes a fatal error by furszy
  19. DrahtBot added the label Needs rebase on Sep 13, 2022
  20. achow101 force-pushed on Sep 19, 2022
  21. DrahtBot removed the label Needs rebase on Sep 19, 2022
  22. achow101 force-pushed on Sep 20, 2022
  23. DrahtBot cross-referenced this on Oct 6, 2022 from issue rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors
  24. DrahtBot cross-referenced this on Nov 8, 2022 from issue scripted diff: fix E275 lint by willcl-ark
  25. DrahtBot cross-referenced this on Nov 9, 2022 from issue wallet: fix crash on loading descriptor wallet containing legacy key type entries by theStack
  26. DrahtBot cross-referenced this on Nov 19, 2022 from issue wallet: bugfix, invalid crypted key "checksum_valid" set by furszy
  27. bitcoin deleted a comment on Nov 19, 2022
  28. DrahtBot cross-referenced this on Nov 29, 2022 from issue wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101
  29. DrahtBot added the label Needs rebase on Nov 30, 2022
  30. achow101 force-pushed on Nov 30, 2022
  31. DrahtBot removed the label Needs rebase on Nov 30, 2022
  32. DrahtBot cross-referenced this on Nov 30, 2022 from issue tests: Run both descriptor and legacy tests within a single test invocation by achow101
  33. DrahtBot added the label Needs rebase on Dec 5, 2022
  34. achow101 force-pushed on Dec 6, 2022
  35. DrahtBot removed the label Needs rebase on Dec 6, 2022
  36. DrahtBot cross-referenced this on Dec 15, 2022 from issue refactor: walletdb: drop unused `FindWalletTx` parameter and rename by theStack
  37. DrahtBot cross-referenced this on Dec 20, 2022 from issue wallet: Have the wallet store the key for automatically generated descriptors by achow101
  38. DrahtBot added the label Needs rebase on Jan 3, 2023
  39. walletdb: Add walletdescriptor(c)key to IsKeyType b36796bfae
  40. achow101 force-pushed on Jan 3, 2023
  41. DrahtBot removed the label Needs rebase on Jan 3, 2023
  42. achow101 force-pushed on Jan 24, 2023
  43. DrahtBot cross-referenced this on Jan 25, 2023 from issue wallet: group independent db writes on single batched db transaction by furszy
  44. 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
  45. 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
  46. wallet: Write encrypted key sig if it is not present f62f2ce2da
  47. wallet: Warn when encrypted key sigs are missing ca8ad28dd9
  48. 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
  49. achow101 force-pushed on Jan 25, 2023
  50. test: Test that encrypted wallets are upgraded with sigs e6b2c43676
  51. achow101 force-pushed on Jan 25, 2023
  52. DrahtBot cross-referenced this on Jan 26, 2023 from issue [Draft / POC] Silent Payments by w0xlt
  53. DrahtBot cross-referenced this on Mar 21, 2023 from issue wallet: Keep track of the wallet's own transaction outputs in memory by achow101
  54. achow101 closed this on Apr 25, 2023
  55. bitcoin locked this on Apr 24, 2024
Contributors
achow101DrahtBotghostbenthecarmantheStackluke-jr
Labels
Wallet
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC