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

User enters seed - add warning if word not in dictionary #8968

Closed
guylouis opened this issue Sep 11, 2019 · 11 comments · Fixed by #9005
Closed

User enters seed - add warning if word not in dictionary #8968

guylouis opened this issue Sep 11, 2019 · 11 comments · Fixed by #9005

Comments

@guylouis
Copy link
Contributor

guylouis commented Sep 11, 2019

Situation

When a user imports a multiaccount by typing in his 12 words seed phrase:

  • We do check the words against our dictionary.
  • Even if they don't match our dictionary, we currently allow the user to proceed without warning.
  • If there is a mispelling or use of unsupported language, the user will end up with a new, empty account
  • If the seed phrase IS valid, intentionally does not match our dictionary, and comes from another wallet, the user will see the funds from that account inside Status.

Improvement

Upon detection of a word that is not in our dictionnary, we will show a warning pop-up.

This pop-up will thus show if there is a mispelling, or the user types a seed phrase in a language we don't support

Definition of the popup

WARNING : exact copy is not ok on figma or image below. Copy should be :
"This looks like a custom seed phrase and doesn't match the Status dictionary. This could also mean some words are misspelled. If so, you'll end up creating a new account.
Continue
Cancel"

https://www.figma.com/file/dEIljL7UPbXgsZUA0Q4qlE5E/Onboarding?node-id=927%3A14702

Onboarding_–_Figma

cc @rachelhamlin @andmironov @hesterbruikman

@rachelhamlin
Copy link
Contributor

rachelhamlin commented Sep 11, 2019

Copy suggestion, now reflected in Figma:

This looks like a custom seed phrase and doesn't match the Status dictionary. This could also mean some words are misspelled. If so, you'll end up creating a new account.

@3esmit
Copy link
Member

3esmit commented Sep 12, 2019

Duplicate of #8810

By default, words outside the dictionary should not be allowed because it will diverge from standard, making it only recoverable in Status, not in MetaMask or any other following the standard.

Also, it's a security weakness to allow users to enter a low entropy seed, read more about this here https://github.com/MyCryptoHQ/support.mycrypto.com/blob/a5b2bcd34088d93a6ac4bfdaefcab40b42159485/src/content/private-keys-passwords/parity-phrases-are-no-longer-supported.md#but-whyyyyy

@rachelhamlin
Copy link
Contributor

But what of this use case @3esmit?

If the seed phrase IS valid, intentionally does not match our dictionary, and comes from another wallet, the user will see the funds from that account inside Status.

Can't the user import a valid seed phrase from another wallet?

@3esmit
Copy link
Member

3esmit commented Sep 13, 2019

Can't the user import a valid seed phrase from another wallet?

Yes, this should happen.

If the seed phrase IS valid, intentionally does not match our dictionary, and comes from another wallet, the user will see the funds from that account inside Status.

I don't see how does a word would "intentionally does not match our dictionary, and comes from another wallet", if that's the case is the other wallet which would not be following the mnemonic standard.

Personally I am not against allowing custom words in the dictionary, but the standard is: it would diverge from standard and they would only work in Status (or other wallets supporting this custom dictionary mnemonic).
The main problem would be if this feature would allow a low entropy seed phrases to be used.

@rachelhamlin
Copy link
Contributor

So I'll admit, I have no idea how derivation paths or seed phrases work.

But it sounds like Status dictionary = common. And if a word doesn't match our dictionary, it can't also be a correct mnemonic coming from MEW or Trust or another wallet either?

In which case, this should really only be an error about misspelling/incorrect seed phrase. And should be prevented.

@guylouis
Copy link
Contributor Author

guylouis commented Sep 13, 2019

so it seems the question we have to answer is : do we keep on accepting a seed which includes word which are outside of BIP39 'official' lists here : https://github.com/bitcoin/bips/blob/master/bip-0039/bip-0039-wordlists.md
right ?

Two options
1/ today we accept such a seed (going against BIP39 then), and just warn the user about this. Risk is letting the user setting up low entropy seed phrases.
2/ we don't accept anymore such seed (I checked and Trust and Metamask don't accept it) and just need to warn the user when his seed phrase is wrong

I don't know if there was a particular reason why we let the user use any word in his list of 12 words. @dmitryn do you know ?

cc @gravityblast

@3esmit
Copy link
Member

3esmit commented Sep 13, 2019

But it sounds like Status dictionary = common. And if a word doesn't match our dictionary, it can't also be a correct mnemonic coming from MEW or Trust or another wallet either?

If Status, MEW and Trust are following the mnemonic specification correctly, no. If a word doesn't match BIP39 dictionary, it is not a valid BIP39 (or whatever is supported) seed phrase; it can be a valid mnemonic of other specification, which isn't supported.

If Status want to support multiple specifications, would be a positive aggregation, but for me seems like everyone is already using BIP39.

Opening words outside dictionary of BIP39, i.e. allowing invalid BIP39 seed phrases, does not seem to bring benefit to users, and will probably cause interoperability issues, recovery mistakes and low entropy seeds (with potential loss of funds).

Instead of this, I suggest:

  • auto-completion (@yenda suggestion);
  • error "red underline" in unrecognized words, which would suggest similar words when tapped (just like any spell checker tool);
  • plan to allow plugins/extensions to enable different types of keystore/mnemonic specifications.

dmitryn added a commit that referenced this issue Sep 18, 2019
dmitryn added a commit that referenced this issue Sep 20, 2019
dmitryn added a commit that referenced this issue Sep 20, 2019
dmitryn added a commit that referenced this issue Sep 24, 2019
dmitryn added a commit that referenced this issue Sep 24, 2019
yenda pushed a commit that referenced this issue Sep 25, 2019
Signed-off-by: yenda <eric@status.im>
@gravityblast
Copy link
Member

@guylouis @rachelhamlin @3esmit
there's another thing, checking only the words is not enough. we should check the checksum of the mnemonic phrase to check if it's really a valid phrase generated with the right algorithm.
For example the following mnemonics contain valid words, but only the first one is valid:

1: abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon about

2: abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon abandon

the other wallets check this, and don't allow importing the second one.

@guylouis
Copy link
Contributor Author

@yenda
given @gravityblast are you ok we open this again ?

@3esmit
Copy link
Member

3esmit commented Sep 26, 2019

I guess is a different issue now.

@yenda
Copy link
Contributor

yenda commented Sep 26, 2019

Ok I agree with Ricardo we should not allow it

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 a pull request may close this issue.

6 participants