[PULL] Wallet Private Key Encryption - page 3.

Hans0

member

Activity: 91

Merit: 10

Quote from: Matt Corallo on June 11, 2011, 06:37:50 PM

Quote from: lachesis on June 11, 2011, 01:48:11 PM

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 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.

Matt Corallo

hero member

Activity: 755

Merit: 515

Quote from: lachesis on June 11, 2011, 01:48:11 PM

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 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.

lachesis

full member

Activity: 210

Merit: 104

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

hero member

Activity: 755

Merit: 515

Quote from: twmz on June 09, 2011, 08:54:12 AM

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.

twmz

hero member

Activity: 737

Merit: 500

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

hero member

Activity: 755

Merit: 515

Quote from: twmz on June 06, 2011, 05:39:45 PM

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.

Quote from: Hans0 on June 06, 2011, 06:32:51 AM

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.

Quote from: Hans0 on June 06, 2011, 06:20:58 AM

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

hero member

Activity: 737

Merit: 500

Quote from: Matt Corallo on June 03, 2011, 04:42:37 PM

Quote from: twmz on June 03, 2011, 02:13:47 PM

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.

Hans0

member

Activity: 91

Merit: 10

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.

Hans0

member

Activity: 91

Merit: 10

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.

mrkva

newbie

Activity: 8

Merit: 0

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

Matt Corallo

hero member

Activity: 755

Merit: 515

Quote from: twmz on June 03, 2011, 02:13:47 PM

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.

twmz

hero member

Activity: 737

Merit: 500

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.

cloud9

member

Activity: 126

Merit: 10

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.

thefiatfreezone

newbie

Activity: 30

Merit: 0

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?

Matt Corallo

hero member

Activity: 755

Merit: 515

Quote from: doublec on May 23, 2011, 02:28:17 AM

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.

doublec

legendary

Activity: 1078

Merit: 1005

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

hero member

Activity: 755

Merit: 515

Quote from: khal on May 18, 2011, 04:09:50 PM

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?

Quote from: khal on May 18, 2011, 04:09:50 PM

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.

Quote from: khal on May 18, 2011, 04:09:50 PM

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.

khal

hero member

Activity: 540

Merit: 500

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

hero member

Activity: 755

Merit: 515

Quote from: xf2_org on May 18, 2011, 12:21:43 PM

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.

xf2_org

member

Activity: 98

Merit: 13

Quote from: Gavin Andresen on May 18, 2011, 09:05:49 AM

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

Topic: [PULL] Wallet Private Key Encryption - page 3. (Read 16634 times)