[PULL] Wallet Private Key Encryption

[Matt Corallo]

I suppose the title speaks for itself, but here is the commit blurb:

Add wallet privkey encryption.

This commit adds support for ckeys, or enCrypted private keys, to the wallet.
All keys are stored in memory in their encrypted form and thus the passphrase
is required from the user to spend coins, or to create new addresses.

Keys are encrypted with AES-256-CBC through OpenSSL's EVP library. The key is
calculated via EVP_BytesToKey using AES256 with (by default) 1000 rounds and
a random salt.

By default, the user's wallet remains unencrypted until they call the RPC
command encryptwallet <password> or, from the GUI menu, Options->
Encrypt Wallet.

When the user is attempting to call RPC functions which require the password
to unlock the wallet, an error will be returned unless they call
walletpassword <password> <time to keep key in memory> first.

A keypoolrefill command has been added which tops up the users keypool
(requiring the password via walletpassword first).
keypoolsize has been added to the output of getinfo to show the user the
number of keys left before they need to specify their password and call
topupkeypool.

A walletpasswordchange <oldpassword> <newpassword> has been added to allow
the user to change their password via RPC.

Whenever keying material (unencrypted private keys, the user's password, the
wallet's AES key) is stored unencrypted in memory, any reasonable attempt is
made to mlock/VirtualLock that memory before storing the keying material.
This is not true in several (commented) cases where mlock/VirtualLocking the
memory is not possible.

Although encryption of private keys in memory can be very useful on desktop
systems (as some small amount of protection against stupid viruses), on an
RPC server, the password is entered fairly insecurely. Thus, the only main
advantage encryption has for RPC servers is for RPC servers that do not spend
coins, except in rare cases, eg. a webserver of a merchant which only receives
payment except for cases of manual intervention.

Thanks to jgarzik for the original patch and sipa for all his input.

Pull request is at https://github.com/bitcoin/bitcoin/pull/232
https://github.com/bitcoin/bitcoin/pull/352
Now rebased on wallet class + mostly based off of Pieter's suggested model.

EDIT: New commit blurb to match changes.

[1714832448]

1714832448

[1714832448]

1714832448

[1714832448]

1714832448

[1714832448]

1714832448

[Gavin Andresen]

Random thought RE: passwords and RPC:

I was thinking a better way of handling the password might be a new RPC command:

walletpassword <password> <timeout>

... which would store <password> in memory for <timeout> seconds.  If you know your server is secure, you'd give a very long <timeout> at startup.

That same <timeout> mechanism might be very handy in the GUI (somebody who knows more about password security might have something intelligent to say about the tradeoff between the risk of storing hashed-password in memory versus the convenience of not having to constantly re-enter it).

A walletpasswordchange <oldpassword> <newpassword> seems like it would be very handy, too.

Tacking <password> onto the beginning of RPC argument lists seems like the wrong thing to do.

[Matt Corallo]

walletpassword <password> <timeout>

... which would store <password> in memory for <timeout> seconds.  If you know your server is secure, you'd give a very long <timeout> at startup.

Ah, good idea.  Hadn't really given any real thought towards RPC stuff, just kind of did it, I suppose just doing it at 1 AM is never the right approach.  The fact that that that fits perfectly with the current method of password handling is also quite nice .

[Matt Corallo]

walletpassword <password> <timeout>
A walletpasswordchange <oldpassword> <newpassword> seems like it would be very handy, too.

Added, committed, rebased, pushed.

[khal]

I would suggest :

walletpassword <timeout> [password]

If password is not on the command line, bitcoin will prompt for it. This will avoid your password to be stored in history files (or anything else) and still allow putting all in 1 command if you prefer. Same for walletpasswordchange rpc.

[Pieter Wuille]

I'd like to see a way to pass the passphrase through a file descriptor instead of on the command-line, so it isn't visible on the process list.

[Gavin Andresen]

Would it be good enough if I volunteered to write a little perl or python or php or bash script that reads the passphrase from a file descriptor and then posts the right JSON-HTTP request?

If the consensus is to teach bitcoind to read arguments from file descriptors, somebody please figure out if there's a standard for how other unix apps do that.  Here's what curl does:

If you start the data with the letter @, the rest should be a file name to read the data from, or - if you want curl to read the data from stdin.

[Matt Corallo]

This will avoid your password to be stored in history files

If you dont want something in .bash_history (probably works for others too) you can just prefix the command with a space.
Doesn't mean the point isn't valid but still...

[Luke-Jr]

This will avoid your password to be stored in history files

If you dont want something in .bash_history (probably works for others too) you can just prefix the command with a space.
Doesn't mean the point isn't valid but still...

It will still show up on the process list ('ps aux') at least for some duration.

I would be concerned with the timeout mechanism creating a race condition. A virus/trojan is going to be able to spend the coins faster than the user. But I guess it could just be added to the already-long list of JSON-RPC problems to be addressed by any contending protocol.

[Matt Corallo]

It will still show up on the process list ('ps aux') at least for some duration.

Hence the "Doesn't mean the point isn't valid but still..."

I would be concerned with the timeout mechanism creating a race condition. A virus/trojan is going to be able to spend the coins faster than the user. But I guess it could just be added to the already-long list of JSON-RPC problems to be addressed by any contending protocol.

If you are concerned about getting the security that tight, there are way more pressing problems.  Memory dumping bitcoin, replacing the bitcoin binary which makes the rpc calls, etc, etc, etc.  But yea, you would have to enter password the way they were originally, and I agree with gavin, that is not nearly as good.

[xf2_org]

If the consensus is to teach bitcoind to read arguments from file descriptors, somebody please figure out if there's a standard for how other unix apps do that.

What you're talking about is a bitcoin shell, IMO.  Just read commands from standard input, like /usr/bin/mysql or /usr/bin/sqlite3.

tcatm has already put some thought and code towards this, and I like this approach.  See https://github.com/tcatm/bitcoin/tree/cli and http://forum.bitcoin.org/index.php?topic=6110.0

[Matt Corallo]

What you're talking about is a bitcoin shell, IMO.  Just read commands from standard input, like /usr/bin/mysql or /usr/bin/sqlite3.

I agree here, making RPC Password entry more secure falls out of the scope of this patch as it requires significant changes to the way RPC is currently handled, which IMHO should not be done here.  (only partially because I'm lazy)
I agree password entry could be more secure via RPC (and GUI for that matter), but the largest target here are really merchants who need a bitcoind to receive money without needing to spend it except for manual intervention.  Here they can pre-generate a couple thousand keys for the pool, and then not have to worry except for when the pool gets low, if their server is compromised, they still don't get their bitcoins.  For GUI users, its nice as it allows them to not have to worry as much about viruses and the like stealing their wallet, but at the end of the day, making a popup that looks identical to Bitcoin asking for their password or reading Bitcon's memory while the user is entering their password is not all that hard. 
For merchants who need this kind of security, they wont be entering their password on a compromised machine anyway (I'd hope) and if they are all hope is lost anyway. 

[khal]

You annoy people for potential security problems that are 100'000 x less frequent than infected/zombie computers and you hope people "wont be entering their password on a compromised machine" ? Are you really serious, guy ?

[Matt Corallo]

You annoy people

Im not sure what you mean by annoy here, the patch requires people to enter the password on send, how is that anoying?

for potential security problems that are 100'000 x less frequent than infected/zombie computers

Again, not really sure what you mean here.  I'm assuming you mean memory stealing? or what?
We have already seen bitcoin wallet-stealing trojans.

and you hope people "wont be entering their password on a compromised machine" ? Are you really serious, guy ?

What is wrong with that hope? This was specifically in reference to people sending from servers, not clients.  Obviously clients will be, but not holding the keys except for milliseconds while they are being entered should be able to stop most basic viruses from stealing them.  That would mean that someone has to spend real time writing the virus/trojan examining memory layout and such of different bitcoin builds or atleast knowing a pretty large amount about the inner workins of wx+bitcoin.
In the case of servers, I'd hope that even if the server is compromised, the remote RPC client will not be run on the server and also, I'd hope the the merchant can find out that they have been compromised before they enter their password to send coins.

[doublec]

I don't understand the topupkeypool command. Can you explain why it is needed, and how the user is supposed to know to run it. What happens if they don't and the keypool (whatever that is) runs out?

[Matt Corallo]

I don't understand the topupkeypool command. Can you explain why it is needed, and how the user is supposed to know to run it. What happens if they don't and the keypool (whatever that is) runs out?

For users running with encryption disabled, it is not needed (as it is currently).  Bitcoin will top up the keypool all the time and it should always be full.
For users running with encryption, the password is required to add new keys to the wallet.  Thus the user needs to manually top up their key pool (otherwise it happens automatically when they enter their password to send coins or otherwise).  Thus for most users, it is not really required that they keep track of the keypool and top it up (except when backing up).  Bitcoin handles new keys when keypool is out pretty well.  For mining and change return, it uses the default address.  When a new tx is received on the client's address, no new address is generated.

[thefiatfreezone]

What would it take to put in separate passwords for each wallet 'account'?
So that for multiple user systems each user can access their own account details and functionality without affecting or having access to others accounts.

And yes, with the ability to put in a 'master' password for global wallet access?

[cloud9]

Can Private keys from a wallet be extracted and imported individually from a wallet?  That may make BTC web deployment more mobile as you would be able to cart your keys along from one web BTC client provider to another.

[twmz]

I have been running this patch for the past few days on a headless bitcoind with a custom web based GUI that uses the JSON-RPC (remotely over SSL) and it has worked very well without any functional problems.  Well done.  I think this change is excellent and really addresses a key concern with protecting wallet.dat.

The only minor thing that I noticed you might want to fix is that the walletpasswordchange RPC method was not added to the RPC help results.

[Matt Corallo]

The only minor thing that I noticed you might want to fix is that the walletpasswordchange RPC method was not added to the RPC help results.

Thanks fixed that and fixed some minor logic issues to make it a bit cleaner for -nocrypt.