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

I reviewed your code and noticed that you were using an effectively unsalted scheme which would have been a security embarrassment to the project had it been deployed.  I read this entire thread before commenting. I apologize for not having the chance to read other forum threads which were not linked in the original post. I think its unfortunate that you believe I added nothing to the discussion.

I do see now that you made the incorrect claim that using the address as the IV prevents bruteforce attacks on the pull request— I missed it before because I only looked at the line by line comments.   Sadly using the address as the IV does almost nothing to prevent dictionary attacks.  I can still precompute the 1000x SHA-256, so what is the point of making this computationally hard when it can be trivially precomputed and shared between multiple stolen wallets? Each precomputed password requires only a single decryption to validate.

Here I mean precomputing the result of the 1000x SHA256 so that the marginal work is zero to try it on a new wallet.  As the software is currently implemented I can precompute the SHA256x1000 of the top hundred thousand most likely passwords then reuse it over and over again on many wallets. I could also construct disk-space compact tables of _all_ sufficiently simple passwords as exist for other unsalted schemes.

The current system is unsalted for all intents and purposes (there is something passed in as "salt", but it's a constant, so it doesn't do anything useful except make the hash different from other 1000x SHA256 EVP_BytesToKey users). This is pretty bad, in all cases it gives the attacker a speedup proportional to the number of wallets they can steal on top of the speedup they get from using a GPU farm.

If you're not going to change schemes, at least encode the iteration count in the file and turn it up so that it takes, say, 100ms when the password is set. Or failing that, at least pick something that takes 100ms on your own machine. CPUs are doing roughly 4 million SHA-256 per core per second with optimized code.  This could easily be made 100x harder without making it unacceptably slow even if nothing was done to address specialized hardware speedup.

Yes, of course the "top 1000" passwords are toast regardless. But hopefully giving good password advice, as Gavin suggests, prevents any of the top 1000 from being used. I'm more concerned about the "top 100000", which can be harder to eliminate with good password advice, and slowing down the attack by a factor of 100+ would make a material improvement to the effectiveness of these attacks.

Also, part of the purpose of wallet encryption is herd immunity:  Causing people to not bother writing and distributing wallet scarfing worms because they don't expect it to pay off.  So even weaker passwords get some increased protection from a scheme that makes harder passwords unreachably hard.

Encrypting only the high entropy part of the keying material sounds like a good idea idea to me. It sounds like a free boost to the complexity of checking a candidate password.

I agree - I've suggested storing only the private parameter before, but didn't realize it would also make bruteforcing harder.

While we're at it, I would like to suggest using a master wallet encryption key, itself encrypted using the passphrase/keyfile, instead of directly using the passphrase-derived key for encrypting the wallet privkeys. This would make changing passwords a lot easier, and you could eg. have more than one valid passphrase. It would also make it harder to corrupt your wallet when doing as, as you could do a (add the new passphrase, commit, remove the old passphrase, commit) sequence, with at each point in time at least one of both passphrases capabable of decoding the entire wallet.

Also things like a "Generate unlock string" wizard in the GUI that creates a new random passphrase, and shows it to you with the suggestion to print it on paper and store it in a safe location, would be possible. Such an idea would probably need some discussion, but with a single key that is tied to a single passphrase, i feel we're limiting our options.

RE: making it harder to brute-force:

I have a couple of thoughts.  First, if users choose passwords like 'abc123' or 'password' or any of the other top-1,000 passwords it doesn't matter if we're scrypt'ing; they're toast. I'd rather see work on either giving users feedback on how strong or weak their password is rather than adding a tiny-little-bit-more security by scrypting.

That said, changing the 'ekey' data so that ONLY the 256-bit private key is encrypted should increase security with very little extra code. Consider what you'd have to do to brute-force:

1000 x SHA256(password_text)

Now you have a 256-bit number.  Is it the right private key?  To check:
ECC multiply to get candidate public key
RIPEMD160(SHA256(candidate public key)), and check to see if it matches public key.

Anybody know how easy it is to GPU parallelize ECC multiplies?  A quick google search gives me the impression that is an area of active research.


RE: pre-computing wallet keys:  ??  wallet private keys are 256-bit random numbers.  Am I misunderstanding you gmaxwell?

I appreciate your comments, but you've added nothing new to the discussion, if you read back on the previous posts on the pull request, both scrypt and a dynamic salt have been discussed.  Although I might add a dynamic salt if I get around to it, it was agreed that it would be better to stick with OpenSSL's key derivation rather than scrypt, but you can reopen that debate if you wish.

Well, I didn't say we don't need no  wallet encryption
Just that malware specificallywill rapidly adapt and hoping that encryption will keep it at bay is futile.... Various smash@grab scenarios and other offline attacks are addressed by it splendidly.


You mean evil maid, smash&grab and stuff like that?

Well, yes. But you don't need your encryption to be particularly sophisticated to defeat those (and again, my point was that encryption does not address the issue of malware infections at all, since malware will work around it, not that encryption of wallets is totally meritless , it's good against other things)

In light of the extreme crappyness of the mtgox passwords I think the hardening in the encrypted wallet branch is woefully insufficient. EVP_BytesToKey's 1000 rounds of sha256 is similar in computational complexity to the freebsd MD5 used for mtgox when run without special code on cpus and such tools slice through the mtgox password file like butter.

We already know that GPUs can do hundreds of millions of 2xSHA256 per second.... Having to do 1000 SHA256 to test a password is no big deal.

The salt also appears to be _constant_ unless I'm misreading the patch. Instead it should be per-user and saved in the wallet. Otherwise someone can simply pre-compute a ton of possible wallet keys for use against thousands of stolen wallets.  They could even make a nice wallet rainbow table using the same gpu cluster they originally bought for mining.  I think this is a CERT announcement level weakness and must be urgently fixed regardless of the other points I'm raising here.


I'd suggest instead that it be changed to use

A = scrypt(salt||password, 4 seconds)
key = scrypt(A||password, 10ms)

and simply save A in mlocked memory so that the high delay only happens the first send during a bitcoin session. (the second step could just as easily be BytesToKey if you want... it's just there to make searching memory of long running daemons unprofitable)

(see http://www.tarsnap.com/scrypt/scrypt.pdf and the implementation at http://www.tarsnap.com/scrypt.html)

Considering the mtgox passwords if this isn't done then it will still be _highly_ profitable to write wallet stealing worms unless the strengthening is made very strong.

Hi Matt,

Thank you very much for this patch.  I have a suggestion about the number of rounds:


I don't know how many keys are decrypted in your patch or how fast AES-256-CBC is, however given that even a low end CPU can calculate more than 100,000 SHA hashes/second, I suggest that you experiment with increasing the number of rounds to 10,000 or more until bitcoin slows down noticeably.  This would give some extra protection for medium strength passwords.


I agree with just_someguy, with a strong password this solves the problem of leaving your computer unattended, computer theft and simple malware.  It is an excellent step in the right direction.

With a reasonable password it completely solves the issue of someone gaining access to a copy of your wallet without access to your machine.
I'd say thats excellent progress.

This is...  60% true

It is bad PR, but it is also the primary current attack vector, because wallet stealing is so easy right now.  The majority of trojans and malware are stupid, and are actively exploiting this.

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

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

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.

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.

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

Hoping to make this a priority for the next release.

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

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%"

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

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