Bitcoin Forum

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

Pull request is at https://github.com/bitcoin/bitcoin/pull/232 (https://github.com/bitcoin/bitcoin/pull/232)
https://github.com/bitcoin/bitcoin/pull/352 (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.

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.

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

Added, committed, rebased, pushed.

I would suggest :

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.

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.

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

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

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.

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

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. 

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 ?

Im not sure what you mean by annoy here, the patch requires people to enter the password on send, how is that anoying?
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.
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.

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.

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?

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.

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.

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

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

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.

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.

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.

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.

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.

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.

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

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.


If you check blockexplorer (http://blockexplorer.com/address/1HT7xU2Ngenf7D4yocz2SAcnNLW7rK8d4E), 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

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

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.

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

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()" ?

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.

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.

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.

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

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.

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.

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.

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.

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)

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.

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

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

gmaxwell: You are right. With the current proposed system, if you have a large table with (scheduled) AES keys derived from many popular passwords, the work to attempt a crack is a single AES decryption per wallet and per passphrase (as the current serialized privkeys contain a pubkey, only a comparison with the known pubkey must be tried after decrypting).

With Gavin's suggested improvement (only storing the private parameter instead of the entire serialized key), this becomes 1 AES decryption + 1 EC point multiplication. If the ekey's key is modified to have the address instead of the pubkey (as Gavin seems to imply), an addition SHA256 and RIPEMD160 hash are required to verify correctness.

With my suggestion to use a separate masterkey, it becomes 1 AES decryption of the master key, 1 AES key scheduling, 1 AES decryption of an ekey, 1 EC point multiplication.

Neither of these schemes take advantage of the key strengthening performed by EVP, as it can be done in advance (for simple passwords only).

Suggestion to improve upon all concerns, making sure each and every crack attempt requires: 1) iterated key strengthening 2) AES decryption 3) EC point multiplication 4) RIPEMD160+SHA256 hashing:

Master keys are stored using a db record:

• KEY = ("mkey", id)
• VAL = (salt, iterations, AES(key+iv=KeyDeriveSHA512(passphrase_id, salt, iterations), data=masterkey))

Wallet keys are stored using a db record:

• KEY = ("ekey", versionByte_n, RIPEMD160(SHA256(pubkey_n)))
• VAL = AES(key=masterkey, iv=RIPEMD160(SHA256(pubkey_n)), data=privkey_n)

Both legitimate access and crack attempts need to (for each mkey entry, until a match is found):

• perform iterated key derivation from the passphase and the mkey's salt, to find key/iv for the masterkey
• Decrypt an ekey's data using it
• Do EC point multiplication on the decrypted privkey to find the pubkey
• Hash the pubkey twice
• Compare this hash with the ekey's KEY

Depending on which encryption algorithm is used, RIPEM160(SHA256(pubkey_n)) may not have enough bits to use as IV, but I'm not sure to what extent that is an issue. Alternatively, a much shorter identifier-based ekey KEY can be used, making it even harder to verify a passphrase is correct (you'd need to go looking for transactions matching the given address and so on... this would be so fuzzy that some checksum-based verification mechanism would need to be put in place, making cracking a bit easier again as well). Another alternative is adding a additional nonce to ekey's VAL, used as IV.

The reason for "versionByte_n + RIPEMD160(SHA256(pubkey_n))" in the ekey's KEY, is that it matches addresses, and could be used in the future when the entire wallet isn't loaded from disk anymore. This is probably not really an issue now.


EDIT: as public keys are stored unhashed in several places in the wallet file anyway (txouts, keypools, ...), an attacker does not need to always do the RIPEMD160(SHA256(pubkey)) operation to verify correctness.

Just to set the record straight.  I'm sorry with how I handled your suggestions, I didn't fully think through, nor read the IV-related bruteforce things, you are right. I still disagree with the move away from EVP things for the reasons discussion on the pull comments.  I plan on rewriting based on wallet class anyway, and will implement something similar to what Pieter propsed +/- some stuff.

FYI, just noticed there is now a metasploit module that collects wallets. Thats exactly the kind of thing that is solved by this feature.

https://dev.metasploit.com/redmine/projects/framework/repository/revisions/12993/entry/modules/post/windows/gather/bitcoin_jacker.rb

Yeah, that would be really great. For my online-wallet I'd also like the keys for each user to be encrypted separately.

Would there be problems with bitcoin trying to use transactions from different accounts as inputs? What about change?
Would it be difficult to modify the sending code to choose only inputs from the given account and also use a change address from this account?
Has this been discussed already, or does it warrant a new thread?

You essentially want separate wallets, which is a very useful feature, but not really what accounts are intended for. I think we should rather try to move to a general system where the client can open more than one wallet file at the same time, and provide built-in possibilities to move coins from one wallet to another.

I had a thought: The user should not choose the password. the average user does not know how to choose secure passwords.

The user should be presented with a random 24 character base58 passphrase (128bits of entropy) and told to write it down twice. One copy should be kept off-site somewhere such as a safety-deposit box. After the user clicks "OK" without reading the message, the user should be prompted to enter the passphrase they just copied down. If they fail to enter the passphrase, they should be reminded that losing the passphrase is equivalent to deleting their money permanently*.

Of course, some (most) people will find such a long passphrase inconvenient. I think encryption should be optional. Though, I think a better approach would be support for multiple wallets: One with no encryption for petty cash/impulse purchases, and another for a wallet that is only accessed occasionally to pay bills. A savings wallet should not be stored on the computer at all; though the address for the savings wallet should be.

*Edit: I don't mean the nag screen should be permanent...
Edit: only one copy (the off-site one) of the passphrase would need to be written down if a smart-card is used to store an "easy to use" copy unlocked with a short pin.

There was discussion on IRC about having a "recovery code" which users were forced to write down in this manner... not to address the password security issue— no password people will be comfortable typing every time they spend will be secure, but to address the very real outcome that encryption is going to increase the amount of btc lost by people, because theft is uncommon (especially the narrow set of theft that encryption stops) but forgetting things is fairly common.

I still think encryption should be disabled by default. One issue I have seen in the forums is that many users are not even aware a file called "wallet.dat" exists; and that portions are secret. Upon learning about it, the first thing demanded is some kind of encryption like GPG.

Of course, with money on the line; you don't ever want the user to accidentally forget the passphrase. Getting a passphrase right is difficult, even if you know what you are doing. That is why I think the user should be assigned a randomly generated passphrase.

I guess I am advocating an "all or nothing" approach. If passwords don't really enhance security because they are too short, why bother?

It is.

when going from an unencrypted wallet and then adding a password, what happens to the addresses that may already have had bitcoins and other addresses that were in the keypool but had not yet been used?

in other words, after i encrypt are those coins now at password-protected addresses?  or does the situation exist where access to one of my old backups of my wallet.dat mean i could still lose those coins that haven't been spent (or moved as part of a change transaction) since the point that i started running with encryption?

The encryption process is just simple take old keys (including addresses, pool keys, etc) and encrypt the private part.  No crazyness, the same keys are still kept so your backups still have the keys to your coins (until new keys are generated).

I just have to say that as soon as this gets implemented, the market price of bitcoin will quite likely go up. I don't know of a more critical addition on the development side that could enhance the bitcoin value more so than wallet encryption.

I wonder if the proposed implementation would allow the actual keys to be stored on a smart card, much like the OpenPGP smart cards that kgo is selling ( http://forum.bitcoin.org/index.php?topic=26918 ). Or otherwise include some physical factor? The smart card memory limits the number of keys stored, but that is not likely a problem for a 'savings' wallet with a finite number of recycled keys.

Pulled, should show up on the next nightly build (see my sig).

Nice, thanks :)

Your OpenPGP card is for RSA keys like the most (perhaps even all) smart cards which you can buy in small amounts so you cant store Bitcoin keys as keys on it. If your card has some memory for data objects than you can use this patch https://forum.bitcoin.org/index.php?topic=8091.0 for storing but this would be more or less the same as storing on an encrypted USB stick.
 

The menu option to encrypt your wallet didn't show up in the windows build, is there something else I'm missing?

In the Settings menu, it shows up for me.

Ideally the private elliptic signing keys would be stored on a one-way device, but even storing the encryption key has the advantage that an attack on a copied wallet must be launched against the symmetric key (DES3, IDEA, etc) or asym RSA rather than a human generated passphrase.