wallet: Cleanup accidental encryption keys in watchonly wallets #28724

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:cleanup-accidental-watchonly-mkeys changing 7 files +105 −0
  1. achow101 commented at 11:02 pm on October 24, 2023: member

    An earlier version allowed users to create watchonly wallets (wallets without private keys) that were “encrypted”. Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.

    We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.

  2. DrahtBot commented at 11:02 pm on October 24, 2023: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Stale ACK laanwj, willcl-ark

    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:

    • #28710 (Remove the legacy wallet and BDB dependency 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.

  3. DrahtBot added the label Wallet on Oct 24, 2023
  4. DrahtBot added the label CI failed on Oct 25, 2023
  5. achow101 force-pushed on Oct 25, 2023
  6. DrahtBot removed the label CI failed on Oct 25, 2023
  7. DrahtBot added the label CI failed on Oct 26, 2023
  8. DrahtBot removed the label CI failed on Oct 26, 2023
  9. laanwj approved
  10. laanwj commented at 1:40 pm on October 31, 2023: member
    Code review ACK aaa6b709d164052c8b2496cbedccf8ccf48c4aa2
  11. in test/functional/wallet_encryption.py:161 in aaa6b709d1 outdated 
    156+
157+            # Make a new dump and check that there are no mkeys
158+            dumpfile_path = self.nodes[0].datadir_path / "noprivs_enc.dump"
159+            do_wallet_tool("-wallet=noprivs_enc", f"-dumpfile={dumpfile_path}", "dump")
160+            with open(dumpfile_path, "r", encoding="utf-8") as f:
161+                assert_equal(all([not line.startswith("046d6b6579") for line in f]), True)
    kevkevinpal commented at 10:40 pm on October 31, 2023:

    why do we only have the first few digits here, I tried with 046d6b657901000000 and it worked fine

    assert_equal(all([not line == "046d6b657901000000" for line in f]), True)

    achow101 commented at 2:18 am on November 1, 2023:
    This prefix matches all possible mkey records so it can make sure that there are no mkeys slipping in elsewhere.
    willcl-ark commented at 11:35 am on November 1, 2023:
    Perhaps a comment to that effect would be useful here as it’s not clear directly from the code?
    achow101 commented at 8:18 pm on November 30, 2023:
    Will add a comment if I re-touch
  12. in src/wallet/walletdb.cpp:1243 in 4db9b63fdc outdated 
    1231@@ -1227,6 +1232,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1232         result = DBErrors::CORRUPT;
1233     }
1234 
1235+    // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is
1236+    // such a wallet and remove the encryption key records to avoid any future issues.
1237+    // Although wallets without private keys should not have *ckey records, we should double check that.
1238+    // Removing the mkey records is only safe if there are no *ckey records.
1239+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) {
    willcl-ark commented at 11:02 am on November 1, 2023:

    I notice that nothing in this block requires any locks (even though cs_wallet will be locked at this stage in this function).

    For my own understanding, is this because the db operations, i.e. writing to the on-disk file, have their synchronisation handled by the db’s transaction system?

    I see for example in LoadEncryptionKey that we lock cs_wallet to load the keys into the wallet, which makes intuitive sense. But it feels that conversely there should be something asserting the lock is held here too, as we remove encryption keys from the wallet? Or at least when pwallet->mapMasterKeys.clear(); is called.

    Perhaps it doesn’t matter as it’s done during loading from the db, so nothing else will have started to access the Wallet?

    achow101 commented at 4:54 pm on November 1, 2023:
    The lock is already being held from the top of the function.
  13. in src/wallet/walletdb.cpp:1244 in 4db9b63fdc outdated 
    1231@@ -1227,6 +1232,21 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
1232         result = DBErrors::CORRUPT;
1233     }
1234 
1235+    // Since it was accidentally possible to "encrypt" a wallet with private keys disabled, we should check if this is
1236+    // such a wallet and remove the encryption key records to avoid any future issues.
1237+    // Although wallets without private keys should not have *ckey records, we should double check that.
1238+    // Removing the mkey records is only safe if there are no *ckey records.
1239+    if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && pwallet->HasEncryptionKeys() && !pwallet->HaveCryptedKeys()) {
1240+        pwallet->WalletLogPrintf("Detected extraneous encryption keys in this wallet without private keys. Removing extraneous encryption keys.\n");
    willcl-ark commented at 11:22 am on November 1, 2023:
    I was wondering if if could be a good idea to log the removed keys at this point as a backup (safe-ish seeing as we are deleting), but I’m struggling to think of a scenario where a user with this type of wallet might somehow need these keys again…
    furszy commented at 10:05 pm on November 1, 2023:
    I’m still not sure about #28142 but, if we ever do something like that, we could have an encrypted wallet without encrypted private keys. So, might want to change the “without private keys” to “without any encrypted record”.
    achow101 commented at 8:19 pm on November 30, 2023:
    I don’t think it would be useful to log them.
    achow101 commented at 8:20 pm on November 30, 2023:
    I think this is fine as is. It can be updated in the future if it becomes outdated.
  14. willcl-ark approved
  15. willcl-ark commented at 11:57 am on November 1, 2023: member

    ACK aaa6b709d164052c8b2496cbedccf8ccf48c4aa2

    I left a few comments, but they’re mainly thoughts I had and questions for my own understanding.

    Also checked the test manually with --legacy-wallet and --descriptors and lightly inspected the dumps to ensure the records were being removed:

     0$ test/functional/wallet_encryption.py
 12023-11-01T11:51:31.078000Z TestFramework (INFO): PRNG seed is: 196471712041506435
 22023-11-01T11:51:31.079000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_hm1fob6j
 32023-11-01T11:51:35.739000Z TestFramework (INFO): Check a timeout less than the limit
 42023-11-01T11:51:35.850000Z TestFramework (INFO): Check a timeout greater than the limit
 52023-11-01T11:51:36.579000Z TestFramework (INFO): Test that wallets without private keys cannot be encrypted
 62023-11-01T11:51:36.595000Z TestFramework (INFO): Test that encryption keys in wallets without privkeys are removed
 7{'walletname': 'noprivs', 'walletversion': 169900, 'format': 'sqlite', 'balance': Decimal('0E-8'), 'unconfirmed_balance': Decimal('0E-8'), 'immature_balance': Decimal('0E-8'), 'txcount': 0, 'keypoolsize': 0, 'keypoolsize_hd_internal': 0, 'paytxfee': Decimal('0E-8'), 'private_keys_enabled': False, 'avoid_reuse': False, 'scanning': False, 'descriptors': True, 'external_signer': False, 'blank': False, 'lastprocessedblock': {'hash': '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206', 'height': 0}}
 8orig_dump=['BITCOIN_CORE_WALLET_DUMP,1\n', 'format,sqlite\n', '0776657273696f6e,4c1e0400\n', '0a6d696e76657273696f6e,ac970200\n', '0962657374626c6f636b,8011010000\n', '1262657374626c6f636b5f6e6f6d65726b6c65,801101000106226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f\n', '05666c616773,0400000005000000\n', 'checksum,058c1f6af6fcbbf22056c1f690d7b9fa27ec2a37dec2b0bcadca89977666634f\n']
 9after_dump=['BITCOIN_CORE_WALLET_DUMP,1\n', 'format,sqlite\n', '0776657273696f6e,4c1e0400\n', '0a6d696e76657273696f6e,ac970200\n', '0962657374626c6f636b,8011010000\n', '1262657374626c6f636b5f6e6f6d65726b6c65,801101000106226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f\n', '05666c616773,0400000005000000\n', '046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n', 'checksum,e63f7200466a4d2d6e05b505be87b148a96ae57702590f0fc284fbbeac332036\n']
10Result of set(orig_dump) ^ set(after_dump):
11{'046d6b657901000000,300dc926f3b3887aad3d5d5f5a0fc1b1a4a1722f9284bd5c6ff93b64a83902765953939c58fe144013c8b819f42cf698b208e9911e5f0c544fa300000000cc52050000\n', 'checksum,058c1f6af6fcbbf22056c1f690d7b9fa27ec2a37dec2b0bcadca89977666634f\n', 'checksum,e63f7200466a4d2d6e05b505be87b148a96ae57702590f0fc284fbbeac332036\n'}
122023-11-01T11:51:37.068000Z TestFramework (INFO): Stopping nodes
132023-11-01T11:51:37.223000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_hm1fob6j on exit
142023-11-01T11:51:37.223000Z TestFramework (INFO): Tests successful
  16. furszy commented at 11:44 pm on November 30, 2023: member

    As the process of reading wallet records from db disregards unknown record types, what if the encryption key is ever used for something else?. Wouldn’t that mean that opening such wallet on any previous version (post this PR) would erase the encryption key forever?

    The erase process can only verify the private keys disabled flag and whether there is any crypted key or not. But it cannot verify if the encryption key was used for something else.

    Guessing that in the case of adding a new type of encrypted record in the future for private keys disabled wallets, we would be forced to add downgrade protection if we merge this PR?

  17. achow101 commented at 11:26 pm on January 2, 2024: member

    Guessing that in the case of adding a new type of encrypted record in the future for private keys disabled wallets, we would be forced to add downgrade protection if we merge this PR?

    Yes, but I think it’s unlikely such usage of encryption keys would be added, and if it were, it could still be added in a way that does not interact with this PR.

    For example,although #28142 allows for encryption of records regardless of the wallet flags, it does this at the database level and uses separate records, so this PR doesn’t affect it at all.

  18. DrahtBot added the label CI failed on Jan 17, 2024
  19. DrahtBot added the label Needs rebase on Mar 29, 2024
  20. wallet: Add HasCryptedKeys 6cef0aa1a6
  21. wallet: Remove unused encryption keys from watchonly wallets 
    Due to a bug in earlier versions, some wallets without private keys may
have an encryption key. This encryption key is unused and can lead to
confusing behavior elsewhere. When such wallets are detected, those
encryption keys will now be deleted from the wallet. For safety, we only
do this to wallets which have private keys disabled, have encryption keys,
and definitely do not have encrypted keys.
    f6fc316af5
  22. tests: Test cleanup of mkeys from wallets without privkeys e975301dbf
  23. achow101 force-pushed on Mar 29, 2024
  24. DrahtBot removed the label Needs rebase on Mar 29, 2024
  25. DrahtBot removed the label CI failed on Mar 29, 2024


achow101 DrahtBot laanwj kevkevinpal willcl-ark furszy

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: 2024-06-29 07:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me