Wallet: Store hash for encrypted keys #6943

pstratem commented at 9:46 PM on November 4, 2015: contributor

Currently when encrypted wallets are unlocked the encrypted private keys are checked against the public key by deriving the public key.

The existing process is robust but very expensive and cannot detect wallet corruption on wallet load.

Adding H(pubkey, cryptedprivkey) to the wallet entry allows for rapid verification and corruption detection on wallet load.

laanwj added the label Wallet on Nov 4, 2015

jgarzik commented at 10:22 PM on November 4, 2015: contributor

concept ACK - though it's a bit disappointing to add this checksumming only for one type of record as a special case

TheBlueMatt commented at 10:25 PM on November 4, 2015: member

The lack of such a checksum is very deliberate as it significantly increases the cost of brute forcing. Not necessarily disagreeing with such a change, just pointing out historical context.

On November 4, 2015 1:46:56 PM PST, Patrick Strateman notifications@github.com wrote:

Currently when encrypted wallets are unlocked the encrypted private keys are checked against the public key by deriving the public key.

The existing process is robust but very expensive and cannot detect wallet corruption on wallet load.

Adding H(pubkey, cryptedprivkey) to the wallet entry allows for rapid verification and corruption detection on wallet load. You can view, comment on, or merge this pull request online at:

#6943

-- Commit Summary --

Move CryptedKeyMap to boost::tuple from std::pair

Add vchHash field to CryptedKeyMap

Require vchHash for ckey wallet entries

-- File Changes --

M src/keystore.h (3) M src/wallet/crypter.cpp (23) M src/wallet/crypter.h (11) M src/wallet/wallet.cpp (15) M src/wallet/wallet.h (4) M src/wallet/walletdb.cpp (18) M src/wallet/walletdb.h (2)

-- Patch Links --

#6943.patch #6943.diff

Reply to this email directly or view it on GitHub: #6943

pstratem commented at 11:38 PM on November 4, 2015: contributor

@TheBlueMatt the hash is over the encrypted private key, not the decrypted private key

pstratem commented at 11:39 PM on November 4, 2015: contributor

@jgarzik i'd like to add this for all the entries, but i dont see any reasonable way to do that

gmaxwell commented at 11:50 PM on November 4, 2015: contributor

@TheBlueMatt also that original rational doesn't actually work, due to the AES padding scheme used.

(The john the ripper bitcoin core wallet support doesn't even bother checking anything beyond the AES padding. :( )

laanwj commented at 9:55 AM on November 5, 2015: member

@TheBlueMatt also that original rational doesn't actually work, due to the AES padding scheme used.

Luckily we perform quite a lot of rounds of key stretching, so it should be quite expensive to bruteforce.

in src/wallet/wallet.cpp:None in c8a3f8778a outdated

 166 | +bool CWallet::LoadCryptedKey(const CPubKey &vchPubKey, const std::vector<unsigned char> &vchCryptedSecret, const std::vector<unsigned char> &vchHash)
 167 |  {
 168 | -    return CCryptoKeyStore::AddCryptedKey(vchPubKey, vchCryptedSecret);
 169 | +    std::vector<unsigned char> vchCheckHash;
 170 | +    CCryptoKeyStore::CalculateCryptedHash(vchPubKey, vchCryptedSecret, vchCheckHash);
 171 | +    if (!std::equal(vchHash.begin(), vchHash.end(), vchCheckHash.begin()))

jonasschnelli commented at 10:12 AM on November 5, 2015:

Is there a reason why you preferred const std::vector<unsigned char> &vchHash over our uint256 type?

pstratem commented at 7:49 PM on November 5, 2015:

not a strong one, i used uint256 for the hash over unencrypted keys

i'll switch to uint256

jonasschnelli commented at 10:18 AM on November 5, 2015: contributor

concept ACK.

There is an vector index issue somewhere (could't trace it down so far): https://travis-ci.org/bitcoin/bitcoin/jobs/89320045#L3784
I think this would require to bump the nWalletVersion (even if this is backward compatible, we need a way to identify wallet versions in case of later migrations, etc.).

TheBlueMatt commented at 7:31 PM on November 5, 2015: member

@gmaxwell Indeed :( (though I thought at some point we fixed that...guess not)

Move CryptedKeyMap to boost::tuple from std::pair ee0dc94f9b

Add vchHash field to CryptedKeyMap e3601081b4

Require vchHash for ckey wallet entries e3abc590ea

Switch to uint256 from std::vector<unsigned char> 6e147d9986

Skip thorough key validation when the hash matches 4486f02bfb

pstratem commented at 1:47 AM on November 6, 2015: contributor

With patch 0.270 seconds wall clock time to unlock 160116 encrypted keys. Without patch 1m29.203s wall clock time to unlock 160116 encrypted keys.

Completely check at least one key 2fc1e534c8

Drop nominal support for multiple master key entries af43e1617f

Remove whitespace 3142c32014

pstratem commented at 1:33 AM on November 7, 2015: contributor

@jonasschnelli We do not need to bump the wallet version, the change is entirely backwards compatible.

pstratem renamed this:
~~[WIP] Wallet: Store hash for encrypted keys~~
Wallet: Store hash for encrypted keys
on Nov 7, 2015

jonasschnelli commented at 7:08 PM on November 7, 2015: contributor

@pstratem: Recently I encountered the same problem with backward compatibility and wallet_version (https://github.com/bitcoin/bitcoin/pull/5916). Someone (I think @luke-jr) convinced me that even if it's backward compatibility and version bump is required (or very useful) for future migration handling, etc. Lets say we once switch to a different wallet database. Then it would be helpful to know what type of scheme (determined by the wallet version) the local wallet.dat follows.

gmaxwell commented at 7:25 PM on November 7, 2015: contributor

We can bump the version without failing to use the checksum in older versions, which is useful to signal if all the keys in your wallet would have been encoded this this way. Potentially in the newer wallet version presence of the checksum can be mandated. Check on decrypt could still be used in the old version, etc.

I don't, however, think we want to decline to use this protection for newly written keys just because the wallet version is old.

pstratem closed this on Dec 13, 2015

DrahtBot locked this on Sep 8, 2021