[PULL]Working wallet private key encryption

[Matt Corallo]

This is a working form of jgarzik's wallet private key encryption patch with some added security (originally here).
https://github.com/TheBlueMatt/bitcoin/commit/0f2fec7d2fc7b6e1dac187735402a721b63ca7cf.
I didn't make this a pull request as I don't think its done in the best way possible and wanted comments.
Currently it encrypts with AES256 in the same way jgarzik's original patch does (private keys only, enter the password at startup or via WALLET_PASSPHRASE environment varible).  It does not go back and encrypt existing unencrypted wallets but any new keys will be encrypted.
Is it best to attempt to sign with keys derive the public keys from the private ones on load to test the password, or should a hash be used? All the ekey's are tested, should that be changed or kept to be safe? etc.

EDIT: Added the option to encrypt every private key already in an unencrypted wallet and bug the user on every application open to do so.
EDIT 2: Added the option to change the password for the wallet.
EDIT 3: Pull request at https://github.com/bitcoin/bitcoin/pull/203

[1714601245]

1714601245

[1714601245]

1714601245

[1714601245]

1714601245

[1714601245]

1714601245

[1714601245]

1714601245

[eof]

i've been discussing this with a friend that this is an absolutely necessary feature; thanks for your help with it.  Don't really have anything to add, just subscribing.

[BitterTea]

I thought it prompted for a password on send? This method seems more useful to me, as you can leave your client running without worrying about an attacker or malware sending all of your money.

[Matt Corallo]

I thought it prompted for a password on send? This method seems more useful to me, as you can leave your client running without worrying about an attacker or malware sending all of your money.

The problem is defining an attacker who has access to your memory sometimes, but not always.  If an attacker already has access to the memory of the bitcoin program, you are screwed no matter how the password is entered/stored.  If they don't you are safe no matter how the password is entered/stored.  I suppose it might be useful for a client which never sends coins, but then you don't need to enter the password at (or I suppose you would have to make a small modification to get that to work).  If there is, however, such an attacker, then by all means it should be changed.

[xf2_org]

I thought it prompted for a password on send? This method seems more useful to me, as you can leave your client running without worrying about an attacker or malware sending all of your money.

It prompts at startup.

As discussed in the original thread, bitcoin creates keys at odd times, so you might not be around when encryption is needed.

[caveden]

Nice! A + for each of you.

[Pieter Wuille]

It seems you are using EVP_BytesToKey, which is used to derive a key and an IV, for each encryption and decryption separately (which is very inefficient), and thereby overriding the passed IV (which is pubkey-dependant).

My suggestion would be to call EVP_BytesToKey once, in SetKey(), and store the key in a field, and discard the generated IV, since we have a better way of determining the IV ourselves. Inside Encrypt and Decrypt, you would derive the IV from the pubkey, and use the stored key.

Also, you don't need to initialize both encoding and decoding contexts when doing only one of these.

[Matt Corallo]

It seems you are using EVP_BytesToKey, which is used to derive a key and an IV, for each encryption and decryption separately (which is very inefficient), and thereby overriding the passed IV (which is pubkey-dependant).

My suggestion would be to call EVP_BytesToKey once, in SetKey(), and store the key in a field, and discard the generated IV, since we have a better way of determining the IV ourselves. Inside Encrypt and Decrypt, you would derive the IV from the pubkey, and use the stored key.

Moved back, I had some concerns about statistical analysis of ramdumps which are fairly easy to do, but I was wrong (see IRC logs for interested parties).

Also, you don't need to initialize both encoding and decoding contexts when doing only one of these.

Yep, fixed.

[cdhowie]

My only comment is to make sure that the user is informed in some way that they must absolutely not lose the password, or they will be unable to recover their money.  That there is nobody who will be able to recover their money.  Unless that's made absolutely clear, I fear that users will inevitably bitch on the forums that they lost their password and ask why we can't recover it. "I mean, PayPal has a forgot password link, why don't you?"

[Matt Corallo]

My only comment is to make sure that the user is informed in some way that they must absolutely not lose the password, or they will be unable to recover their money.  That there is nobody who will be able to recover their money.  Unless that's made absolutely clear, I fear that users will inevitably bitch on the forums that they lost their password and ask why we can't recover it. "I mean, PayPal has a forgot password link, why don't you?"

"Enter a password to encrypt/decrypt your wallet.\nWARNING: DO NOT LOSE THIS PASSWORD, YOU WILL LOSE ALL YOUR BITCOINS." good enough?

[cdhowie]

"Enter a password to encrypt/decrypt your wallet.\nWARNING: DO NOT LOSE THIS PASSWORD, YOU WILL LOSE ALL YOUR BITCOINS." good enough?

That's probably good enough.  It might also be wise to clarify that nobody, not even the authors of the software, are able to recover lost passwords.

[Matt Corallo]

That's probably good enough.  It might also be wise to clarify that nobody, not even the authors of the software, are able to recover lost passwords.

"Enter a password to encrypt/decrypt your wallet.\nWARNING: If you lose this password, no one, not even the Bitcoin developers can get you your Bitcoins back."

Also, as an implementation note, this patch will check that EACH key is properly decrypted by signing something (the public key, simply because it was handy in that code block) with it.  It's a bit slower, but on my machine it takes literally no time to sign and adds a bit more security in case someone is messing with merging wallets...does anyone have an objection to that, or is it worth keeping?

[Pieter Wuille]

Actually, I just realized, there is an even easier way. After decrypting the private key, use CKey::GetPubKey() to get its public key, and see if it matches the pubkey stored in the entry's key.

[Matt Corallo]

Actually, I just realized, there is an even easier way. After decrypting the private key, use CKey::GetPubKey() to get its public key, and see if it matches the pubkey stored in the entry's key.

God, I even looked for that in key.h last night.  I really shouldn't program past 3am.  Oh well, fixed now.

[BitterTea]

Question - how does this work with bitcoind? Does it requires a password upon startup?

[Matt Corallo]

Question - how does this work with bitcoind? Does it requires a password upon startup?

The current version requires it at startup, but I'm working on RPC password passing.