[Onboarding] Store keys with the 6-digit pincode

[flexsurfer]

Keycard is not in the scope of this PR

Acceptance criteria:

1. pixel perfect
2. store with 6-digit pincode

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

status-go: https://github.com/status-im/status-go/issues/1463

[rachelhamlin]

Hey @dshulyak, could you share some info on what's been done for pin code storage on Android? I heard you'd worked on that.

[dshulyak]

@rachelhamlin there are two PRs linked to https://github.com/status-im/status-go/issues/1463

https://github.com/status-im/status-react/pull/8525/ implements module to work with android keychain api and https://github.com/status-im/status-go/pull/1515 is a prototype for keystore that can use keychain api

[yenda]

https://docs.google.com/document/d/1Mp6Dr_n0h_xwyKBiLcdU4TBggXRDlT4ShmOVnMgqzC8/edit?usp=sharing

require some very complex changes in status-go and native code.

The only argument to not do it in status-react was that it is risky because of dependencies, but it's kind of ridiculous considering that the seedphrase is going through status-react anyway so until we have some kind of 100% native element that handle it it's overkill.

So to me it seems like there was a much simpler solution to implement that, and there is no point doing the encryption/decryption of the ethereum key in the secure enclave unless you are going to keep the ethereum keys there, what is the point if you don't keep them in the secure enclave to sign stuff, why do you protect the encryption key more than the keys you are encrypting?

for that reason I'd like to check out what people think of this flow for devices that have a secure enclave:

1. the app uses the sha3 of your seed phrase as encryption key (what we call password right now) and store it in the secure enclave (keystore in Android), using setInternetCredential method like we currently do for the password
2. when the app needs to unlock your multiaccount it asks for pin code, if you fail 5 times it deletes everything related to the account in the keystore and the account is locked until you type in the seed phrase
3. you can use biometrics instead of seed phrase to unlock, I would assume that we can store auth-method and pin code in the keystore with setInternetCredential as well, that is the part I would ask Trail of Bit if that makes sense. This is accessible only from the app, I don't think hashing and salting would change anything
4. you can choose to only ask biometrics/seedphrase for using wallet keys and auto login (same as save password we have now, except that it uses a much better encryption for the files)

So just like biometrics, the pin code is just a yes/no for releasing the encryption key That also means the user can now change their PIN code

The flow described above requires minimal changes because basically the key is the previous password used to login and sign stuff. The PIN code input is the same as for keycard The biometric is already in a PR

To me it seems like it is a low hanging fruit with high return, it is much easier to implement than what is described in the google doc without requiring any changes on status-go side (and actually, status-go already uses the password sha3 not the password directly, but password sha3 is still much easier to bruteforce than seedphrase)

related to previous proposal:

• discussion on status-go implementation https://github.com/status-im/status-go/issues/1463#issuecomment-508042399
• initial android attempt https://github.com/status-im/status-react/pull/8525
• last document left by Igor https://www.notion.so/Encryption-in-a-secure-element-ccb8778651024440a81f4f1e626a4342

[yenda]

@rachelhamlin @siphiuel @corpetty @flexsurfer @andremedeiros @andmironov

TLDR: So the simple approach is just about decoupling the user secret (previously password, after that PIN or biometrics) and the encryption key of the db+keys by using the hash of the seedphrase as encryption key, stored in the keystore, and releasing it on correct pin code or biometric (the simple user secret). My gutsfeeling is that we gave up too soon because the previous proposal was too much work but it was also a lot of unnecessary complexity that doesn't bring anything and we must do the simple thing for the audit if it makes sense.

[rachelhamlin]

@yenda thanks for bringing some critical thinking here :)

I don't have much POV to offer on the implementation - but because this is not included for v1, I'd suggest we discuss it during the core sync on Monday, and later rank its priority during our Oct 15 core planning session.

[yenda]

@yenda thanks for bringing some critical thinking here :)

I don't have much POV to offer on the implementation - but because this is not included for v1, I'd suggest we discuss it during the core sync on Monday, and later rank its priority during our Oct 15 core planning session.

after v1 is too late, we should get this as part of the audit, the implementation is negligible we need to get the concept audited as well as the validity of the use of .setInternetCredentials to protect a secret in keystore, which is also critical as of now anyway since it is used for storing the password

[rachelhamlin]

I spoke with @cammellos about this as well. What do you think of spending a day next week to spike on it, ensure that there's a clear migration path for existing users (which should be easy, we have done it before, but best to spend some time ensuring that, since a lot of things have changed) and then marking it as nice-to-have?

I don't want to get caught up in thinking that everything account related (or security critical) must be done before this one audit deadline. We have a relationship with TOB and can integrate security reviewed work in the future as well. Moreover, I don't like to rush things, no matter how simple.

Looking forward to @corpetty's input on Monday too.

[yenda]

@rachelhamlin yes if @corpetty validates the concept I can already do the underlying part that doesn't have any UI changes but ensures that there will be no need to change the encryption key of the db and keyfiles, which is not that simple I believe because otherwise we would have the "change password" feature for a long time already. That means storing the sha3 of the mnemonic in the secure enclave and use it as encryption key.

[cammellos]

@yenda just some additional conxtex about re-encrypting the db:

We already had data migrations re-encrypting the db, basically what we do is use the filename as version, i.e {installation-id}.v1.db. Both keys are then passed old-key new-key, if a v1.db is found, we copy to v2.db, open with old-key, re-encrypt with new-key, delete v1.db. Once the migration is done, old-key can be discarded.

[yenda]

@cammellos thanks for the info I didn't know it was feasible now already without changes. However there is still the issue that if we want to use the sha3 of the mnemonic and the user already backed it up, we need to design and implement a specific flow for the migration, which I'd rather avoid considering we could just implement the decoupling of password/encryption key now (for devices with keystore)

[cammellos]

yes, that's a good point, so at least that needs taking care of if we want to have a smooth transition

On Sat, Sep 28, 2019, 12:12 yenda notifications@github.com wrote:

@cammellos https://github.com/cammellos thanks for the info I didn't know it was feasible now already without changes. However there is still the issue that if we want to use the sha3 of the mnemonic and the user already backed it up, we need to design and implement a specific flow for the migration, which I'd rather avoid considering we could just implement the decoupling of password/encryption key now (for devices with keystore)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/status-im/status-react/issues/8535?email_source=notifications&email_token=AAHYJMDQNRX4BI42RSYK36LQL4UYBA5CNFSM4H626YY2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD72VRCI#issuecomment-536172681, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHYJMCONZRQRRIONMA5IEDQL4UYBANCNFSM4H626YYQ .

[cammellos]

@hesterbruikman @errorists are we still going for this or we should close the issue?

[cammellos]

Closing for now as seems like there's no activity and probably stale