Skip to content
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

SEP-0005 Key Derivation Methods for Stellar Accounts #63

Merged
merged 3 commits into from
Nov 9, 2017
Merged

Conversation

bartekn
Copy link
Contributor

@bartekn bartekn commented Nov 9, 2017

Links in "Implementation" section are currently in PRs:

@lenondupe would you mind checking if your Ledger app gives the same results as in "Test Cases" section? I'll try to get a test Ledger device by the end of next week.

@lenondupe @zulucrypto let me know if you want me to change your usernames to real names.

@irisli
Copy link
Contributor

irisli commented Nov 9, 2017

Parity recommends users to not use a mnemonic as a password and I think the reasoning can be applied here too. Lets apply murphy's law. If mnemonic was used as a passphrase and a user may think they could just think of a new mnemonic password and use it. We need a way to prevent this from happening.

Off the top of my head, a error detection word (checksum) would prevent users from creating new passwords, and a error correction word would help mitigate issues with mistyping. Of course, this issue has been thought of a lot and I'm sure many cryptography UX experts have thought about this.

The key here is to use this only as a RECOVERY method. One other thing is that it's encouraged as a recovery method because of the possibility of keyloggers, and the secret key scheme is slightly worse where most just copy and paste (vulnerable to clipboard sniffing).

One purported benefit of a mnemonic over a secret key string is that you can skip around when typing it. Naive attackers won't be able to figure it out. However, with a 12 word passphrase and given the attacker knows the words and not permutations, there are at most 12 factorial combinations leaving you with only 28 bits of security (log(12!)/log(2)) (assuming the user skips around 12 times; otherwise the bound is fac(skips)).

In summary:

  • Murphy's law: if user is allowed to make a weak mnemonic, it will happen. Additionally, we can't just say that "its their fault". We should also define a way to prevent users from making up a passphrase.
  • Mnemonics provide protection against naive attackers who don't brute force over the possible 400 million combinations (given attacker has words but not the ordering, and user skipped 12 times). Slightly better than just a secret seed for naive attackers. (Additionally, i don't like password key derivation since a GPU can run thousands times faster than JavaScript lulling us into a false sense of security. If js took 1 second and GPU had a 10,000x speedup, then it only takes 6 GPU-months to crack a passphrase potentially worth millions).

@bartekn
Copy link
Contributor Author

bartekn commented Nov 9, 2017

Yes, the mnemonic code is certainly not a password. The only context I used this word ("Test Cases" section) was a BIP-0039 passphrase. I will change it to BIP39 passphrase. As you noticed, mnemonic codes should be used mainly for recovery and to move keys between wallets (also kind of recovery 😉).

When it comes to checksums, it's implemented in BIP-0039 Generating the mnemonic section and we use it without any modifications.

@irisli
Copy link
Contributor

irisli commented Nov 9, 2017

Okay, yeah i only just commented on the mnemonic stuff. I'm sure the mnemonics themselves are secure, but I think we need to put more emphasis recommending wallet creators to use mnemonics in a responsible way.

I don't really have much comment on the deterministic key derivation since I'm not a cryptography expert, but ed25519 deterministic keys are possible (but complicated).

@ghost
Copy link

ghost commented Nov 9, 2017

@bartekn
I could successfully validate tests 2 & 3, but Ledger won't let me enter a 15 word mnemonic. Only 12 and 18. So, so far seems to work brilliantly 👍

@jedmccaleb
Copy link
Contributor

bartek I changed the process for these SEPs a bit. The idea is that first they go into /drafts and are moved over to /ecosystem once they are approved. I think this will be less confusing

@bartekn
Copy link
Contributor Author

bartekn commented Nov 9, 2017

@lenondupe that's a great news! Added additional test with 12 words.

@jedmccaleb yes, I've seen a new process, but I decided to make it "Accepted" directly because 1) it doesn't invent anything new, it's more like defining which standard to use in Stellar, 2) we don't have much room for changes/improvement here (maybe except derivation path: m/44'/148'/x') because changing anything will make it incompatible with hardware wallets. But I can follow the process if you want. Let me know.

@zulucrypto
Copy link
Contributor

This is great! Thanks a lot for the work on this and to both you and @lenondupe for getting this standard figured out so quickly.

I'll use this same implementation in my PHP library and on the Trezor.

@ghost
Copy link

ghost commented Nov 9, 2017

12 word mnemonic works fine

@jedmccaleb jedmccaleb merged commit c844d65 into master Nov 9, 2017
@JeremyRubin
Copy link
Contributor

post-merge: Spec looks good to me, didn't look at the implementation.

@bartekn bartekn deleted the sep-0005 branch November 14, 2017 14:23
@darocha
Copy link

darocha commented Mar 2, 2018

This is great! Is this already available on the js-stellar-sdk?

@bartekn
Copy link
Contributor Author

bartekn commented Mar 2, 2018

@darocha not sure if it will be ever added to js-stellar-sdk. There's already Node JS package for this: https://github.com/stellar/stellar-protocol/blob/master/ecosystem/sep-0005.md#other-implementations

@darocha
Copy link

darocha commented Mar 2, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants