[PULL] Wallet Private Key Encryption

[mrkva]

Maybe somebody will be interested in one backup&encrypt script I made?
https://github.com/mrkva/BitcoinBackup.sh

[Hans0]

Reading "CBC mode" made me remember this paper (causing many ASP.NET applications to become remotely vulnerable because they used CBC mode): http://www.isg.rhul.ac.uk/~kp/padding.pdf

I do not fully understand the problem but the CBC mode seems to be vulnerable. Maybe somebody can look at this.

[Hans0]

Idea: Launch a new process that displays the password prompt and sends it back via IPC. That way the GUI will not hold to the password as all memory is destroyed when the process exits. Reduces the chance of it going into swap or hibernation file.

[twmz]

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.

Any thoughts on how a client might detect that a particular bitcoind had encryption enabled (thereby requiring walletpassword) vs a bitcoind that doesn't have encryption enabled.

[Matt Corallo]

Any thoughts on how a client might detect that a particular bitcoind had encryption enabled (thereby requiring walletpassword) vs a bitcoind that doesn't have encryption enabled.

Its currently pretty smart.  If -nocrypt is on, walletpassword and walletpasswordchange will not appear in help.
You could check that, or just call one of the two and see if you get an error saying that you have -nocrypt on.

Idea: Launch a new process that displays the password prompt and sends it back via IPC. That way the GUI will not hold to the password as all memory is destroyed when the process exits. Reduces the chance of it going into swap or hibernation file.

At that point you are putting *way* too much effort into protecting against one attack, when others are easier to exploit anyway, such as a keylogger or memory dumper that dumps the keys out of bitcoin anyway.

Reading "CBC mode" made me remember this paper (causing many ASP.NET applications to become remotely vulnerable because they used CBC mode): http://www.isg.rhul.ac.uk/~kp/padding.pdf

I do not fully understand the problem but the CBC mode seems to be vulnerable. Maybe somebody can look at this.

That attack appears to require a padding oracle which is a function which can check if arbitrary data fits the necessary padding scheme.
For that to work, the padding oracle must have the private key, so an attacker could not exploit it without your private key.  Thus it doesnt really apply.  Also, it appears to only apply to some padding scheme, and although I didnt bother looking into that, I'd hope openssl defaults to a secure padding scheme.

[twmz]

Today I decided to try changing my password with walletpasswordchange and it didn't seem to work.   Well, the RPC call succeeded as if it had worked, but the password didn't seem to have changed.  The walletpassword RPC call still only accepted the old password and not the new one.

I should have tested this earlier, sorry.

[Matt Corallo]

Today I decided to try changing my password with walletpasswordchange and it didn't seem to work.   Well, the RPC call succeeded as if it had worked, but the password didn't seem to have changed.  The walletpassword RPC call still only accepted the old password and not the new one.

Oops, had a 1-character typo in rpc.cpp, should work now.

[lachesis]

I've found what appears to be a nasty bug, and has made me potentially lose 65 BTC so far:

I called "bitcoind getaccountaddress Testing" or something similar. It returned the address "1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E". I then sent 10BTC to this address (showing my friend how the unlock RPC command works).

A few minutes later, I called "bitcoind getaccountaddress FromMtGox" to withdraw some BTC from MtGox. It also returned "1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E" although I didn't notice it at the time.

In fact, no matter what account I ask for, I get that address. Even worse, I don't seem to have the private key for it. I don't see the recv part of any of the transactions that I just described in my listtransactions output.

Code:

bitcoind validateaddress 1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E
{
    "isvalid" : true,
    "address" : "1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E",
    "ismine" : false
}

If you check blockexplorer, you'll see that address has clearly received the coins. The 0.01 is from me as well. However, the 3.79 transaction is not from me. What's going on here!? Furthermore, who owns that address and how the hell did my client get it and decide to use it for all my accounts?

--Lachesis

[Matt Corallo]

I've found what appears to be a nasty bug, and has made me potentially lose 65 BTC so far:

If you check blockexplorer, you'll see that address has clearly received the coins. The 0.01 is from me as well. However, the 3.79 transaction is not from me. What's going on here!? Furthermore, who owns that address and how the hell did my client get it and decide to use it for all my accounts?

Looks like the bug was a missing { which caused it to return the address of a blank pubkey (which is impossible to generate).  Those coins are gone. Really sorry about this, just poor coding on my part. I posted a separate commit so that the fix is clearly visible.
If you want proof I didn't steal the coins, you can change rpc.cpp:381 (of the new version) to

Code:

    vector<unsigned char> vchBlankVector;
    strAddress = PubKeyToAddress(vchBlankVector);

and you will notice that it always returns that address (an address derived from pubkey of length 0 which cannot exist).
Again, Im really sorry about all of this, Ill be sending Lachesis a couple coins, and if anyone else has a valid problem with this as well, Ill send you a couple, just pm me.  It was just really poor syntax in my coding, I really need to stop coding after 2 am.

[Hans0]

I've found what appears to be a nasty bug, and has made me potentially lose 65 BTC so far:

If you check blockexplorer, you'll see that address has clearly received the coins. The 0.01 is from me as well. However, the 3.79 transaction is not from me. What's going on here!? Furthermore, who owns that address and how the hell did my client get it and decide to use it for all my accounts?

Looks like the bug was a missing { which caused it to return the address of a blank pubkey (which is impossible to generate).  Those coins are gone. Really sorry about this, just poor coding on my part. I posted a separate commit so that the fix is clearly visible.
If you want proof I didn't steal the coins, you can change rpc.cpp:381 (of the new version) to

Code:

    vector<unsigned char> vchBlankVector;
    strAddress = PubKeyToAddress(vchBlankVector);

and you will notice that it always returns that address (an address derived from pubkey of length 0 which cannot exist).
Again, Im really sorry about all of this, Ill be sending Lachesis a couple coins, and if anyone else has a valid problem with this as well, Ill send you a couple, just pm me.  It was just really poor syntax in my coding, I really need to stop coding after 2 am.

Don't send him your coins. You are not his insurance against software bugs. We are grateful that you help develop bitcoin and that must be enough for the guy who lost his coins.

[lachesis]

Don't send him your coins. You are not his insurance against software bugs. We are grateful that you help develop bitcoin and that must be enough for the guy who lost his coins.

Nobody's forcing him to do anything. I didn't ask him for coins - he offered.

[Hans0]

Don't send him your coins. You are not his insurance against software bugs. We are grateful that you help develop bitcoin and that must be enough for the guy who lost his coins.

Nobody's forcing him to do anything. I didn't ask him for coins - he offered.

I know. I am advising him. It is not his duty to undo damages caused by unintentional bugs. Mistakes happen.

[Matt Corallo]

Don't send him your coins. You are not his insurance against software bugs. We are grateful that you help develop bitcoin and that must be enough for the guy who lost his coins.

That said, I feel bad that he lost a couple coins due to a poor coding mistake.
I'm not sending him the full amount (largely because I dont have much left myself) but still, I fucked up, and, if nothing else, should pay him in appreciation of his willingness to test my code.
In any case, this is a good opportunity to reiterate the standard "this is code written once and only mostly tested, it has not been looked over by others, and not even much by me since I wrote it, it is beta and should not be expected to work 100%"

[jgarzik]

Hoping to make this a priority for the next release.

Review requested!  https://github.com/bitcoin/bitcoin/pull/232

[Hans0]

Hoping to make this a priority for the next release.

Review requested!  https://github.com/bitcoin/bitcoin/pull/232

Oh, the missing braces were a nasty bug! What about adding an assert to PubKeyToAddress that ensures "!vector.empty()" ?

[passerby]

Okay, maybe I'm being dumb like a rock here, but do I understand correctly that the threat model in the context of which this is supposed to operate  is bitcoin-hunting malware infection?

Y/N?

If Y, then what prevents malware from stealing the crypto key material from memory when user authenticates or just plain intercept authentication (assuming no HSM/smartcards being employed to store valet away from malware tampering)?

If N, then sorry my bad.

[jgarzik]

Nobody claims this will solve malware infections.

A simple keylogger can defeat this.

But we need to raise bar so that "cat wallet.dat | mail" thefts will not work.

[passerby]

I think it's pretty measly a defense that would be primarily a PR tool (SEE? WE DO CARE ABOUT SIMPLETON J. USER!) and not an actual security measure.

To really secure a wallet against malware, you'd be best to implement some smartcard/HSM support or something, so that wallet is manipulated in a strongly isolated environment.

[bzzard]

Nobody claims this will solve malware infections.

A simple keylogger can defeat this.

Maybe some screen keyboard with A-Z keys generated at random positions?
But then still some malware can do a screenshot

File as a key can be also good solution (like in Truecrypt).

[passerby]

Nobody claims this will solve malware infections.

A simple keylogger can defeat this.

Maybe some screen keyboard with A-Z keys generated at random positions?
But then still some malware can do a screenshot

File as a key can be also good solution (like in Truecrypt).

I don't want to sound like some patronizing ass, especially since I can't code 'fo shit, but I really think people should read some books by Bruce Schneier - our IT security guy had me do it after we got into an argument over somewhat similar type of thing and it indeed changed the way I see those things.

 What you are currently doing is concocting a very specific countermeasure against a very narrow implementation of a certain attack vector.
It is futile.

If worst comes to worst, as long as the cryptographic material shares the same memory with malware, you can't really expect to be able to keep the bad guys at bay.

The only way to reliably ward off such an attack is to keep the wallet or some part thereof on an isolated module 100% of the time, such as smart card or that USB security dongle thingus. Or implement some mindboggling virtualization set-up which no sane user would actually use.


Having the wallet "encrypted" is, however, important for PR (Otherwise people will claim Devs don't care about "average joe six-pack" and his hard earned bitcoins).


Thus, the simplest possible encryption/password scheme should be implemented, so that as little time as possible is lost on such technologically pointless endeavor.


Also, someone of those people who have thousands upon thousands of dollars worth of coins in their wallet should post a bounty for development of smartcard/HSM support of some sort.
Srsly, that's in their own enlightened self-interest  ...