wallet: Allow users to create a wallet that encrypts all database records

achow101 commented at 9:15 pm on July 24, 2023: member

A couple of users have requested that we support wallets that encrypt everything, even if the wallet is watch-only, in order to have better privacy if the wallet is stolen. This PR introduces an EncryptedDatabase backend for the wallet which encrypts/decrypts each key-value record individually before reading from or writing to the database.

EncryptedDatabase is only supported for SQLite databases and descriptor wallets. This was done in order to have an easier way to get downgrade protection that also does not involve writing an existing record in plaintext (e.g. minversion or flags). SQLite has a fixed field “application id” that we already use for cross-network protection. This is reused to detect if a sqlite database is an encrypted wallet, and thus it prevents older software from attempting to open such wallets.

In order to read records from the database, EncryptedDatabase will cache the decrypted key in memory so that it can lookup the encrypted key in the database. Values will always be decrypted when read.

The encryption scheme is the same one that we use for encrypting private keys. It’s not that great, but I didn’t feel like re-implementing it, and it seems good enough. The encryption key itself is encrypted with the passphrase and stored in a new record.

Wallets with encrypted databases cannot be loaded on start. They can only be loaded by explicit user action through loadwallet which now has a db_passphrase parameter to allow the decryption of these wallets. createwallet also has a db_passphrase to create these wallets. For now, there is no way to encrypt the database after it has been created, nor is there a way to change the passphrase.

DrahtBot commented at 9:15 pm on July 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
Concept NACK	luke-jr
Concept ACK	stevenroose

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:

#29129 (wallet, rpc: add BIP44 account in createwallet by brunoerg)
#29119 (refactor: Use std::span over Span by maflcko)
#29112 (sqlite: Disallow writing from multiple SQLiteBatchs by achow101)
#29071 (refactor: Remove Span operator==, Use std::ranges::equal by maflcko)
#29054 (wallet: reenable sethdseed for descriptor wallets by achow101)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#22341 (rpc: add path to gethdkey 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.

DrahtBot added the label Wallet on Jul 24, 2023

achow101 force-pushed on Jul 24, 2023

achow101 commented at 9:22 pm on July 24, 2023: member

cc @stevenroose

DrahtBot added the label CI failed on Jul 24, 2023

in src/wallet/sqlite.h:106 in d97c315f6d outdated

100@@ -99,7 +101,7 @@ class SQLiteDatabase : public WalletDatabase
101     SQLiteDatabase() = delete;
102 
103     /** Create DB handle to real database */
104-    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false);
105+    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});

maflcko commented at 2:00 pm on July 25, 2023:

0    SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock = false, std::array<const std::byte, 4> app_id_xor = {});

Nit: I think you can just let the compiler fill in the zeros for you by using {}.

in src/wallet/sqlite.h:153 in d97c315f6d outdated

147@@ -146,7 +148,7 @@ class SQLiteDatabase : public WalletDatabase
148     bool m_use_unsafe_sync;
149 };
150 
151-std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
152+std::unique_ptr<SQLiteDatabase> MakeSQLiteDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::array<const std::byte, 4> app_id_xor = {std::byte{0}, std::byte{0}, std::byte{0}, std::byte{0}});

maflcko commented at 2:00 pm on July 25, 2023:

same

luke-jr commented at 7:05 pm on July 25, 2023: member

Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don’t think this full-encryption you’re proposing has a real use case.

(What might make sense is to support fully encrypting backups.)

achow101 force-pushed on Aug 4, 2023

stevenroose commented at 6:03 pm on August 10, 2023: contributor

Concept NACK, I think. Wallet encryption is not going to help you if someone steals your wallet, only if someone gains very temporary access to your keyboard/mouse only. I don’t think this full-encryption you’re proposing has a real use case.

(What might make sense is to support fully encrypting backups.)

I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn’t be encrypted. I want to use the wallet for a watch-only wallet though. Unfortunately the wallet files live alongside the public blockchain data so I would have to store the wallet on an unencrypted drive (that I occasionally want to borrow out to friends for them to copy the blockchain in order not to have to download it). That would reveal my entire wallet to anyone that gets hold of this drive.

Same would hold for a laptop. The blockchain is public data, encrypting it entirely is just a pointless performance loss. So it should always be stored on an unencrypted disk for performance reasons. But then you can’t use the wallet. Even an encrypted wallet (currently only encrypting the private keys) leaks your entire wallet’s history.

stevenroose commented at 6:04 pm on August 10, 2023: contributor

Concept ACK :) (Concept Request, in fact)

furszy commented at 8:08 pm on August 10, 2023: member

I requested this because I have a real use case.. I run a full node that is on an unencrypted external drive. Bitcoin data is public data, it shouldn’t be encrypted. I want to use the wallet for a watch-only wallet though. Unfortunately the wallet files live alongside the public blockchain data so I would have to store the wallet on an unencrypted drive (that I occasionally want to borrow out to friends for them to copy the blockchain in order not to have to download it). That would reveal my entire wallet to anyone that gets hold of this drive.

Why not just load the wallet from a different directory? It is as simple as typing loadwallet <absolute_wallet_dir_path> true once.

Much safer to not share the wallet data at all than sharing it encrypted.

stevenroose commented at 3:48 pm on August 11, 2023: contributor

@furszy That would work. My experience though is that touching anything inside Core’s directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too.. I’ll do that for now. But anyway, someone getting hold of your hard drive shouldn’t learn your wallet metadata, I think that’s a good principle.

furszy commented at 9:27 pm on August 11, 2023: member

@furszy That would work. My experience though is that touching anything inside Core’s directories can get me in trouble. I once renamed a wallet file and Qt broke. It would only work again if I went into some JSON file to rename the wallet there too..

Well, that is not the case for loading the wallet from an external directory. No need to touch anything on your system. Just run the RPC command.

That being said, if you can trigger any crash by manually changing something on your system, feel more than welcome to create as many issues as you need until all crashes are resolved. We should be able to fix them all. Core shouldn’t crash (unless you modify something when is running..).

maflcko commented at 9:46 am on August 23, 2023: member

Maybe mark as draft while CI is red?

achow101 force-pushed on Aug 23, 2023

DrahtBot removed the label CI failed on Aug 24, 2023

DrahtBot added the label CI failed on Sep 6, 2023

DrahtBot removed the label CI failed on Sep 10, 2023

DrahtBot added the label Needs rebase on Sep 14, 2023

achow101 force-pushed on Sep 14, 2023

DrahtBot removed the label Needs rebase on Sep 14, 2023

DrahtBot added the label Needs rebase on Dec 11, 2023

crypter: Separate setting IV from setting key 486a203df1

crypter: Make sure IV is set too c0e1a7e538

achow101 force-pushed on Dec 11, 2023

DrahtBot removed the label Needs rebase on Dec 11, 2023

walletdb: Add WalletBatch::Read overload for DataStream

It's useful to be able to just read a record without the batch doing any
sort of deserialization. The new overload of Read will just place the
record's value into the provided DataStream.

c76438d40a

walletdb: Add EncryptedDatabase and its batch and cursor classes

EncryptedDatabase is a WalletDatabase that encrypts the records before
writing them to an underlying WalletDatabase. This encryption occurs
transparently to the higher level application logic so the wallet does
not need to be concerned about whether the data it is writing is
encrypted.

In order to work with prefix matching and cursor iteration in an order
that we are expecting, EncryptedDatabase maintains a map of the
unencrypted record keys to the encrypted record keys. When given the
plaintext record key to pull up, it can retrieve the encrypted record
key and then retrieve the encrypted record from the underlying database.

11436dd1da

walletdb: Use a different SQLite application id for encrypted db

EncrytpedDB wallets will use sqlite but with a different application id.
This provides downgrade protection in addition to easy identification of
encrypted dbs. The application id will be the network magic XOR'd with
0x36932d47 (randomly generated value).

c239ee306a

walletdb: Have MakeDatabase also create encrypted dbs 754c055b29

wallet, rpc: Be able to create and load wallets with encrypted dbs c6aed1e70c

wallet: Skip loading on start of wallets with encrypted databases

Wallets with encrypted databases need the user to provide their database
passphrase which cannot be done on start, so skip any such wallets on
startup.

8ac458adcf

test: Add functional test for wallets with encrypted db 9b845cc3e7

achow101 force-pushed on Dec 11, 2023

achow101 commented at 0:03 am on January 6, 2024: member

Marking as up for grabs.

achow101 closed this on Jan 6, 2024

achow101 added the label Up for grabs on Jan 6, 2024

wallet: Allow users to create a wallet that encrypts all database records #28142

Code Coverage

Reviews

Conflicts