[#8968] custom seed phrase popover

[dmitryn]

fixes #8968

Summary

Show "custom seed phrase" popover when recover with seed phrase containing words not from the dictionary.

Figma https://www.figma.com/file/dEIljL7UPbXgsZUA0Q4qlE5E/Onboarding?node-id=4311%3A0&viewport=1143%2C-2667%2C0.7808234691619873

status: ready

[status-github-bot]

Pull Request Checklist

• Have you updated the documentation, if impacted (e.g. docs.status.im)?

[status-im-auto]

Jenkins Builds

Click to see older builds (22)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	d6ea8fd	#1	2019-09-18 07:28:36	~3 min	windows	📄 log
❌	d6ea8fd	#1	2019-09-18 07:29:17	~3 min	linux	📄 log
❌	d6ea8fd	#1	2019-09-18 07:29:17	~3 min	macos	📄 log
✔️	d6ea8fd	#1	2019-09-18 07:34:44	~9 min	ios	📦 ipa
✔️	d6ea8fd	#1	2019-09-18 07:39:34	~14 min	android	📦 apk
✔️	d6ea8fd	#1	2019-09-18 07:39:41	~14 min	android-e2e	📦 apk
✔️	83e7628	#3	2019-09-20 15:06:15	~8 min	ios	📦 ipa
✔️	83e7628	#3	2019-09-20 15:09:30	~11 min	macos	📦 dmg
✔️	83e7628	#3	2019-09-20 15:11:49	~13 min	linux	📦 App
✔️	83e7628	#3	2019-09-20 15:12:03	~14 min	android-e2e	📦 apk
✔️	83e7628	#3	2019-09-20 15:12:16	~14 min	android	📦 apk
✔️	83e7628	#3	2019-09-20 15:13:46	~15 min	windows	📦 exe
✔️	8ceebe9	#5	2019-09-23 11:05:45	~9 min	ios	📦 ipa
✔️	8ceebe9	#5	2019-09-23 11:09:47	~13 min	macos	📦 dmg
✔️	8ceebe9	#5	2019-09-23 11:19:36	~23 min	android-e2e	📦 apk
✔️	8ceebe9	#5	2019-09-23 11:21:27	~25 min	android	📦 apk
✔️	8ceebe9	#5	2019-09-23 11:22:43	~26 min	linux	📦 App
✔️	8ceebe9	#5	2019-09-23 11:24:19	~28 min	windows	📦 exe
✔️	1a64ebc	#4	2019-09-23 11:06:16	~13 min	android-e2e	📦 apk
✔️	1a64ebc	#4	2019-09-23 11:06:25	~13 min	android	📦 apk
✔️	1a64ebc	#4	2019-09-23 11:06:50	~14 min	linux	📦 App
✔️	1a64ebc	#4	2019-09-23 11:08:02	~15 min	windows	📦 exe

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	6f0c819	#6	2019-09-24 12:34:56	~12 min	ios	📦 ipa
✔️	6f0c819	#6	2019-09-24 12:35:37	~13 min	android	📦 apk
✔️	6f0c819	#6	2019-09-24 12:36:19	~14 min	linux	📦 App
✔️	6f0c819	#6	2019-09-24 12:36:46	~14 min	android-e2e	📦 apk
✔️	6f0c819	#6	2019-09-24 12:37:29	~15 min	windows	📦 exe
✔️	f8804dc	#7	2019-09-24 12:45:51	~8 min	ios	📦 ipa
✔️	f8804dc	#7	2019-09-24 12:48:37	~11 min	macos	📦 dmg
✔️	f8804dc	#7	2019-09-24 12:50:12	~13 min	android	📦 apk
✔️	f8804dc	#7	2019-09-24 12:50:32	~13 min	android-e2e	📦 apk
✔️	f8804dc	#7	2019-09-24 12:50:48	~13 min	windows	📦 exe
✔️	f8804dc	#7	2019-09-24 12:50:52	~13 min	linux	📦 App

[statustestbot]

98% of end-end tests have passed

Total executed tests: 46
Failed tests: 1
Passed tests: 45

Failed tests (1)

Click to expand

1. test_send_two_transactions_one_after_another_in_dapp

Device 1: Wait for EnterPasswordInput Device 1: Wait for EnterPasswordInput Device 1: 'EnterPasswordInput' is not found on the screen Device sessions Device 1: Steps, video, logs Failure screenshot

Passed tests (45)

Click to expand

1. test_block_user_from_public_chat

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

2. test_filters_from_daap

Device sessions Device 1: Steps, video, logs

3. test_copy_and_paste_messages

Device sessions Device 1: Steps, video, logs

4. test_send_transaction_from_daap

Device sessions Device 1: Steps, video, logs

5. test_deploy_contract_from_daap

Device sessions Device 1: Steps, video, logs

6. test_open_transaction_on_etherscan

Device sessions Device 1: Steps, video, logs

7. test_public_chat_messaging

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

8. test_long_press_to_delete_1_1_chat

Device sessions Device 1: Steps, video, logs

9. test_password_in_logcat_sign_in

Device sessions Device 1: Steps, video, logs

10. test_text_message_1_1_chat

Device sessions Device 2: Steps, video, logs Device 1: Steps, video, logs

11. test_add_to_contacts

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

12. test_sign_typed_message

Device sessions Device 1: Steps, video, logs

13. test_unread_messages_counter_1_1_chat

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

14. test_ens_in_public_chat

Device sessions Device 2: Steps, video, logs Device 1: Steps, video, logs

15. test_logcat_send_transaction_from_daap

Device sessions Device 1: Steps, video, logs

16. test_send_message_in_group_chat

Device sessions Device 2: Steps, video, logs Device 1: Steps, video, logs

17. test_logcat_send_transaction_from_wallet

Device sessions Device 1: Steps, video, logs

18. test_send_token_with_7_decimals

Device sessions Device 1: Steps, video, logs

19. test_offline_messaging_1_1_chat

Device sessions Device 2: Steps, video, logs Device 1: Steps, video, logs

20. test_modify_transaction_fee_values

Device sessions Device 1: Steps, video, logs

21. test_send_eth_from_wallet_to_address

Device sessions Device 1: Steps, video, logs

22. test_add_account_to_multiaccount_instance

Device sessions Device 1: Steps, video, logs

23. test_manage_assets

Device sessions Device 1: Steps, video, logs

24. test_long_press_to_delete_public_chat

Device sessions Device 1: Steps, video, logs

25. test_send_emoji

Device sessions Device 1: Steps, video, logs

26. test_search_chat_on_home

Device sessions Device 1: Steps, video, logs

27. test_logcat_recovering_account

Device sessions Device 1: Steps, video, logs

28. test_can_add_existing_ens

Device sessions Device 1: Steps, video, logs

29. test_messaging_in_different_networks

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

30. test_logcat_sign_message_from_daap

Device sessions Device 1: Steps, video, logs

31. test_switch_users_and_add_new_account

Device sessions Device 1: Steps, video, logs

32. test_send_stt_from_wallet

Device sessions Device 1: Steps, video, logs

33. test_login_with_new_account

Device sessions Device 1: Steps, video, logs

34. test_start_chat_with_ens

Device sessions Device 1: Steps, video, logs

35. test_add_contact_from_public_chat

Device sessions Device 2: Steps, video, logs Device 1: Steps, video, logs

36. test_password_in_logcat_creating_account

Device sessions Device 1: Steps, video, logs

37. test_backup_recovery_phrase

Device sessions Device 1: Steps, video, logs

38. test_offline_status

Device sessions Device 1: Steps, video, logs

39. test_open_google_com_via_open_dapp

Device sessions Device 1: Steps, video, logs

40. test_unread_messages_counter_public_chat

Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

41. test_sign_message_from_daap

Device sessions Device 1: Steps, video, logs

42. test_user_can_remove_profile_picture

Device sessions Device 1: Steps, video, logs

43. test_share_contact_code_and_wallet_address

Device sessions Device 1: Steps, video, logs

44. test_refresh_button_browsing_app_webview

Device sessions Device 1: Steps, video, logs

45. test_backup_recovery_phrase_warning_from_wallet

Device sessions Device 1: Steps, video, logs

[yenda]

Tested on Android

[churik]

@dmitryn please add accessibility-ids to "Continue" and "Cancel" buttons

[dmitryn]

@churik added

[churik]

@errorists please take a quick look, meanwhile I'll fix tests

[errorists]

left comments here, left Figma - right Screenshot

[churik]

@dmitryn now due to this popup it is possible to create account with wrong amount of words.

Steps:

• enter seed phrase, that contains 13 words and some words outside dictionary
• tap on done

Expected result:
keyboard is closed, "Next" button is disabled
Actual result:
popup "some words are misspelled"

[churik]

@dmitryn Please, take my commit when squashing

[yenda]

@dmitryn use nested-text element

[dmitryn]

@yenda thanks for the tip, i'll use it

[status-im-auto]

✔️ status-react/prs/linux/PR-9005#6 🔹 ~14 min 🔹 6f0c819 🔹 📦 linux package

[dmitryn]

@churik bug fixed by disabling ability to proceed from keyboard unless you tap Next button.
@errorists made layout fixes, please take a look.

[yenda]

@errorists please review

iPhone 6 Plus

One Plus 7 Pro

[errorists]

Looks good @yenda thanks!

[yenda]

ok looks good to merge for me
@churik if you want to do a last round of manual testing I think the following cases are way enough:

• test a a a a a a a a a a a a twice and check that it recovers the same account
• test a known seed phrase and check that it recovers the usual account

[yenda]

@churik actually I just tested that so I'll merge

[3esmit]

Why allow invalid BIP39? What's the specific use case of this? Is anyone aware of someone that would need that?

Do you trust users they will use properly this feature?

Auditors and other wallet providers will complain about this, because app gives option to user create an unsafe wallet.

This PR is solving this analogous issue: MyEtherWallet/etherwallet#485
e.g. User enter seed phrase misspelled, send funds, later never can recover.

But not this one MyEtherWallet/etherwallet#551 (comment) @tayvano said:

What I failed to realized is that new users, instead of generating a wallet on the homepage like I stupidly assumed they would do, were simply typing in whatever words they felt like into that box and getting the address and calling it a day.

e.g. App gets popular... web2 users: "So I can enter any seed phrase "paswordy thing"? this ones the app give me are too difficult, I will use here a custom seed phrase with something easier to remember..."; Later get's account mined by a low-entropy account bruteforcer.

From what we learned of Parity seed phrase case is that we should never trust the user with optional security. Instead app should enforce security.

And anyway, if someone gets hurt by this, even if is the user mistake, the ultimate fault is of the app that allowed it, and therefore the social bashing would be over the app, not the random affected user.

cc @corpetty

[3esmit]

However the parity seed phrase problem had a worse checking. Status is looking for 12 words, which could be a 12 letter password separated by spaces in worst case, so perhaps the issue I mentioned above is less problematic, but we might see people loosing funds by entering famous phrases, e.g. songs lyrics, instead of real BIP39.

This weak accounts are usually precalculated and keep being watched, so they are very quickly drained once one put values. Researchers were able to identity millions stolen this way, in both Bitcoin and Ethereum, and probably in any cryptocurrency where wallets allow users to make brain wallets.

https://www.securityweek.com/most-bitcoin-brain-wallets-drained-attackers https://pdfs.semanticscholar.org/7e89/dabb8717006590b674bfb0ba45ce2bd0c131.pdf
https://www.wired.com/story/blockchain-bandit-ethereum-weak-private-keys/

[hesterbruikman]

Thanks for jumping in here @3esmit. I've been a little out of the loop. Here a prior discussion on topic where you can find some of the reasoning: #8687 (comment) (this comment onwards)

As you point out, the 12 word restriction makes it less problematic, however, it's actually 12, 15, 18, 21, 24 words. So the chance of randomly hitting one of those word counts is higher.

Relevant last words summarizing that we do allow words that are not in our dictionary (also if they contain an typo as we won't know)
#8687 (comment)

In the decision to allow words not in our dictionary is the assumption that people now or in the future potentially have used a different dictionary than the one(s) used by Status. The underlying rationale here (which is obviously open to debate) is to mitigate risk rather than block the user.

I have 3 questions to move forward:

1. Do we want to block the user from entering a seed phrase not in our dictionary(s) altogether?
2. If we don't block the user what are all exact risks they are taking?
3. What are way to mitigate these risks if we don't block the user?

1/ Personally I prefer exploring all mitigation options before deciding to block as this is an excellent opportunity to educate. We don't know what we don't know and people will never learn if we don't tell them or allow for low risk mistakes (maybe we give this type of custom account limited options, perhaps even chat & browsing options only). IMO the problem isn't necessarily that you can recover random seed phrases, the problem is what you can do next.

2/ I think you fairly point out that we don't inform users of the risk they're taking, mainly that they will never ever be able to recover this account. What are other risks?

3/ Also, I would agree that it's a little too easy to continue given the risks. We definitely don't want to encourage unsafe behavior! Maybe the explicit 'type CONTINUE' to recover this custom seed phrase is a more suitable pattern. Something only the brave will endure to continue.

[yenda]

Since we are talking about multiaccount seed phrase validation (not wallet account), based on @3esmit solid arguments:

1. yes we want to block the user
2. otherwise he might enter shitty seedphrases and get his account stolen by bots, or mispell something and lose the account forever
3. we block the user

The only valid use case was testing by using an invalid seedphrase. Status is using the standard dictionary anyway, there is no other valid dictionary. The seedphrase point is to have maximal entropy while remaining human and typo friendly, not be pleasing and easy to remember (that would mean it is also easy to crack).

Also as mentioned we should not only consider the words and their count but also the checksum, because repeating 12 times the same word is not a valid seedphrase either.

[yenda]

For education purposes I propose this metaphor:

This is the kind of key you need:

This is the kind of key you are trying to use:

[3esmit]

@hesterbruikman

1. Yes, because it's not only a potential security issue, but also hurt an important principle: interoperability with BIP39.

2. Risk of user misspell word is mitigated by warning, but random users are known for making bad decisions to themselves as ever as possible, even when warned.

3. The best way to mitigate this without blocking custom seed phrases would be to remove it from the default user access, and provide a plugin tool which allow easy development and activation of custom key systems, therefore users that somehow created an invalid BIP39 could take an conscious decision on adding an custom system.

4. I agree that blocking can be bad because it might affect some users, and is in our principles accessibility and inclusion, however it cannot hurt interoperability. Custom seed phrase feature introduces a way of creating this invalid BIP 39 seeds, which would not be supported by other wallets, such as MetaMask, MyCrypto, or by Nano Ledger, etc

5. Two risks: Loss of recovery phrase (in case of misspell word and ignored warning) and loss of funds (in case of use of common phrases or low entropy e.g. one two three four five ...)

6. If supporting invalid BIP39 is an feature we need, and we want to make it difficult, there is a way of doing so that would be making painful to user,
   e.g.

• I accept that this account will only work in Status.im;
• I accept that this account have no safety guarantee ;
• I am probably doing something bad to myself.
  [I am genius]

Does other wallets support invalid BIP39, or how is called here, custom seed phrase?

See this comment in the original issue: #8968 (comment)

https://our.status.im/our-principles/

VIII. Inclusivity
We believe in fair and widespread access to our software, with an emphasis on ease-of-use. This also extends to social inclusivity, permissionless participation, interoperability, and investing in educational efforts.

[churik]

@hesterbruikman would you mind to create an issue?
we need to fix it for v1 audit I believe.
cc @rachelhamlin

[hesterbruikman]

Thanks for the discussion @3esmit @yenda! Reflecting I don't see a convincing current use case to support 'invalid BIP39', but want to check some other wallets to be sure @churik before I create an issue to fix.

I like the rephrasing of why should we block to why should we support invalid BIP39. From the 'blocking' persective I still stand by mitigation over blocking; but realistically, we don't have the time to do this well beyond a warning:)

Other thoughts in response:

2/ Risk of user misspell word is mitigated by warning, but random users are known for making bad decisions to themselves as ever as possible, even when warned.

Agree that warnings are mostly ineffective, depending on their design. That said it's a mitigation, we can consider others like lowering the severity by limiting options.

3/ The main way to mitigate this would be to remove it from the default user access, and provide a plugin tool which allow easy development and activation of custom key systems, therefore users that somehow created an invalid BIP39 could take an conscious decision on adding an custom system.

Love this idea!

5/ Two risks: Loss of recovery phrase (in case of misspell word and ignored warning) and loss of funds (in case of use of common phrases or low entropy e.g. one two three four five ...)

Loss off recovery phrase would only occur if people use the Recovery flow to create a new multiaccount. People are weird, but given the primary actions are on creating a new multiaccount I consider this a highly unlikely edge case.

[tayvano]

Don't allow custom seed phrases.

Period.

End of conversation.

There is no such thing.

That's called a brain wallets and we don't allow people to use those for obvious humans-cant-entropy-and-will-have-their-funds-stolen-and-they-will-blame-you-and-i-will-blame-you reasons.

The BIP32/39/44 spec has a checksum (last word) as well a limitations by wordlist. Honestly, just use the library and call it a day. You're dangerously close to rolling your own crypto here and reducing interoperability is the one place this ecosystem has it. Pretty much literally everything else you do will have a million viable paths you can go down and decisions you will have to make.

Standard bip support is not one of them.

---

Okay so now that we've accepted that.....yes bip spec has issues. Yes plausible deniability is only a feature for fucking crazy crypto fucks, not users who are error prone. Yes advanced users, like you mr. developer, want X feature. No, it does not mean you should try to "improve" or mitigate bc you can't bc it would turn into xkcd standards comic. There are 1000 initiatives already to improve it and address some common issues. Work with them if you really want to.die on this hill.