status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.9k stars 987 forks source link

Can recover account with recover phrase containing invalid chars or words outside the dictionary #8810

Closed churik closed 5 years ago

churik commented 5 years ago

Description

Type: Bug

Summary: when I paste from keyboard or type invalid word \ letter outside Wordlist from https://github.com/status-im/status-go/blob/52cdcf8f0f1127f5ee694f39086a9e4df54174e6/extkeys/mnemonic.go#L97 I expect validation to be shown. In fact, I can recover account with seed phrase like ; two three four five six:5668 seven eight nine ten eleven twelv$$$$$$e

Expected behavior

validation like "some words can be misspelled"

Actual behavior

can recover account

Reproduction

Additional Information

@hesterbruikman would be nice to have your opinion on it

churik commented 5 years ago

"Some words to be misspelled" should be shown (without error) @andmironov are you OK with this popup? image

3esmit commented 5 years ago

Suggested solution: Allow only valid mnemonic. (Error otherwise) Its important to block users trying to use, at least for, low entropy, otherwise the community might drop support, and users might misunderstand the feature - threating the seedphrase field like a simple password. Also, if someone uses a mnemonic outside standards, other wallets (like MyCrypto and MetaMask) would not accept it. For a general rule, there is no reason for accepting an invalid mnemonic as its probably not what 99.9% of users would be trying to do.

Lessons learned: Users were using bad entropy to create accounts and store funds: https://www.parity.io/restoring-blank-seed-phrase/ MyEtherWallet removes support of Parity Phrase: https://github.com/MyCryptoHQ/support.mycrypto.com/blob/a5b2bcd34088d93a6ac4bfdaefcab40b42159485/src/content/private-keys-passwords/parity-phrases-are-no-longer-supported.md TLDR: Parity Phrases are like Mnemonic Phrases, but they are just some words hashed that generate a key, and they would allow anyone to type anything there. People were starting to type there low entropy seeds and this lead to funds loss.

However, blocking with and error would create another bug, which would be that "advanced users" cannot use custom seedphrases, or other seed phrases, or even raw private keys, but this needs to be solved with some other feature, such as an advanced menu in this screen (allowing a checkbox with [x] allow invalid mnemonic) or adding support of plugins that would install a different key system, keycard could be a plugin aswell. This would not be an priority. We can create an new issue to discuss this minor side effect .

andmironov commented 5 years ago

I'd suggest this as an option: show this custom alert message after the user hits "Next" with a custom seed phrase.

Screen Shot 2019-09-06 at 14 31 48

But if implementing that will take too much time, I'd stick to the native alert.

yenda commented 5 years ago

It's fixed since the alert was implemented, though now we are moving towards no invalid seedphrases allowed at all