{"action":"created","issue":{"url":"https://api.github.com/repos/mimblewimble/grin/issues/2004","repository_url":"https://api.github.com/repos/mimblewimble/grin","labels_url":"https://api.github.com/repos/mimblewimble/grin/issues/2004/labels{/name}","comments_url":"https://api.github.com/repos/mimblewimble/grin/issues/2004/comments","events_url":"https://api.github.com/repos/mimblewimble/grin/issues/2004/events","html_url":"https://github.com/mimblewimble/grin/pull/2004","id":383101196,"node_id":"MDExOlB1bGxSZXF1ZXN0MjMyNjUwMTIz","number":2004,"title":"Wallet Seed Encryption + BIP39","user":{"login":"yeastplume","id":7074070,"node_id":"MDQ6VXNlcjcwNzQwNzA=","avatar_url":"https://avatars3.githubusercontent.com/u/7074070?v=4","gravatar_id":"","url":"https://api.github.com/users/yeastplume","html_url":"https://github.com/yeastplume","followers_url":"https://api.github.com/users/yeastplume/followers","following_url":"https://api.github.com/users/yeastplume/following{/other_user}","gists_url":"https://api.github.com/users/yeastplume/gists{/gist_id}","starred_url":"https://api.github.com/users/yeastplume/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/yeastplume/subscriptions","organizations_url":"https://api.github.com/users/yeastplume/orgs","repos_url":"https://api.github.com/users/yeastplume/repos","events_url":"https://api.github.com/users/yeastplume/events{/privacy}","received_events_url":"https://api.github.com/users/yeastplume/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-11-21T12:43:09Z","updated_at":"2018-11-23T22:02:39Z","closed_at":null,"author_association":"MEMBER","pull_request":{"url":"https://api.github.com/repos/mimblewimble/grin/pulls/2004","html_url":"https://github.com/mimblewimble/grin/pull/2004","diff_url":"https://github.com/mimblewimble/grin/pull/2004.diff","patch_url":"https://github.com/mimblewimble/grin/pull/2004.patch"},"body":"Edit: Tested and should be good to go now. Major changes are:\r\n\r\n* Wallet seed is encrypted with a password (SHA512 pbkdf), salt and nonce (salt and nonce are contained in the wallet.seed file) \r\n* BIP39 recovery phrases are provided at wallet init\r\n* A new 'wallet recover' command that either restores a seed file, or displays the current seed.\r\n* Wallet seed, once encrypted, is used 'as is', e.g. there's no further hashing step with a password to create the 'real' seed. This is to ensure recovery phrases will always work without extra info.\r\n* Older-style wallet.seed files are converted by reading them, hashing as before with whatever password is provided, and storing the resulting seed in `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.\r\n* Providing a password is required at wallet init. This can be blank (but it's the user's choice to leave this blank).\r\n* The wallet prompts the user for the password when required. (It can still be specified on the command line if desired)\r\n\r\n****\r\nAt 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. \r\n\r\nIn 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.\r\n\r\nThe 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).\r\n\r\nThe 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.\r\n\r\nAlso, 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.\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/mimblewimble/grin/issues/comments/441322956","html_url":"https://github.com/mimblewimble/grin/pull/2004#issuecomment-441322956","issue_url":"https://api.github.com/repos/mimblewimble/grin/issues/2004","id":441322956,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MTMyMjk1Ng==","user":{"login":"DavidBurkett","id":24623306,"node_id":"MDQ6VXNlcjI0NjIzMzA2","avatar_url":"https://avatars2.githubusercontent.com/u/24623306?v=4","gravatar_id":"","url":"https://api.github.com/users/DavidBurkett","html_url":"https://github.com/DavidBurkett","followers_url":"https://api.github.com/users/DavidBurkett/followers","following_url":"https://api.github.com/users/DavidBurkett/following{/other_user}","gists_url":"https://api.github.com/users/DavidBurkett/gists{/gist_id}","starred_url":"https://api.github.com/users/DavidBurkett/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/DavidBurkett/subscriptions","organizations_url":"https://api.github.com/users/DavidBurkett/orgs","repos_url":"https://api.github.com/users/DavidBurkett/repos","events_url":"https://api.github.com/users/DavidBurkett/events{/privacy}","received_events_url":"https://api.github.com/users/DavidBurkett/received_events","type":"User","site_admin":false},"created_at":"2018-11-23T22:02:39Z","updated_at":"2018-11-23T22:02:39Z","author_association":"NONE","body":"@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:\r\n1. A dynamic number of SHA512 rounds based on hardware speed to help alleviate the \"fast hash\" part while minimizing impact to end-users.\r\n2. Using AES256-CBC instead of an authenticated encryption scheme, which alleviates the \"fast verification\" part, at the cost of extra validation required to determine if the correct password was entered.\r\n3. An extra layer of encryption where the private keys are actually encrypted (using AES256-CBC) with an entirely random key, and the random key encrypted using the password and pbkdf. This helps alleviate both parts, although I haven't done the math to determine how much it actually helps.\r\n\r\nWhat 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.\r\n\r\nSource: https://en.bitcoin.it/wiki/Wallet_encryption"}}
{"actor":{"display_login":"DavidBurkett"}}