-
Notifications
You must be signed in to change notification settings - Fork 993
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Wallet Seed Encryption + BIP39 #2004
Conversation
Ready for a review/comment, but I still need to think about how to convert old seeds to the new BIP39-compatible format before merging. |
Think I'm going to include BIP39 in this as well, to minimise disruption caused by changes to the seed format. |
I'll review the code more thoroughly tmrw. But from first glance the biggest concern I have is with using sha256 as your pbkdf. Even with 100 iterations, that's not a very strong hash, especially considering how much hardware support it has. Most secure wallets use something stronger like Scrypt to derive the key. It's slower (which is the whole point), but the parameters are configurable for different hardware (mobile, for example). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. My only comment would be that src/bin/cmd/wallet.rs
is getting quite large, it may be worth breaking some of it out into a utility file or in the wallet crate itself.
@DavidBurkett Looks like this does 100 rounds of SHA512. We could look for an improvement with scrypt, bcrypt or even argon2 but at least in Rust there's always the risk of running into insecure implementations, so 100x SHA512 seems like a conservative choice right now. It's also the most standard. We could look at stronger functions in another PR but it'll require quite a bit of crypto library hunting I think.
@ignopeverell That's fair. I certainly understand the hesitation to include a third party crypto library without thorough investigation. 100 rounds seems reasonable to me, but I know I proposed a similar scheme for a previous project, and when I discussed it with a few cryptographers, they unanimously informed me it's too weak. The problem, they explained, was the combination of a fast hash (sha512), and fast verification. Since we're using authenticated encryption, we'll know if the key is correct as soon as we try to decrypt, meaning we also have the fast verification problem. I looked into what bitcoin core does, and it appears they get around this in 3 ways:
What we're doing in this PR is certainly more convenient than the other options, so if it is secure enough, I don't suggest changing it. It's entirely possible that the cryptographers I spoke with before were just being overly cautious, or that I just entirely misunderstood them, so I messaged @apoelstra to see what his thoughts were (he's still involved with the project, right?). I'll respond here if he gets back to me. |
Edit: Tested and should be good to go now. Major changes are:
wallet.seed
as the new seed. Note since these seeds are 64 bytes long, there's no recovery phrase option for them. The code to do this conversion is intended to be temporary.At present, the 'password' passed in via the command line is hashed with the wallet seed, essentially creating a new wallet for every seed/password combination. The intention here is to modify this behaviour to be more in line with other wallets.
In anticipation of integration BIP39 recovery phrases, the wallet should always be completely recoverable from the word list. Hashing the resulting entropy with a separate password is obviously inconsistent with this. Something similar is allowed in the BIP39 spec, but discouraged, and note this is more of an 'extension word' than a password in the way we're using it.
The seed should be entirely recoverable from a word list, so grin's wallet password should be used to encrypt wallet data. This PR modifies the saving of the wallet seed to use ring's implementation of AEAD encryption to store the encrypted seed in the wallet.seed file, along with random salt and nonce (in case anyone wants to store many of these in a database or something).
The password is then needed to decrypt the contents of the seed file. If the user forgets the password, the seed can still be recovered with the (coming soon) BIP39 recovery phrase (which will generate a new wallet.seed file with a new nonce, salt, password). Data stored in wallet_data should still work (if it exists), or the user can restore from there.
Also, since decryption will fail if the password is wrong, we won't be seeing any more issues where users are confused looking at the 'wrong' wallet because they forgot they used a password.