status-im / status-go

The Status module that consumes go-ethereum
https://status.im
Mozilla Public License 2.0
728 stars 246 forks source link

[Onboarding] Store keys with the 6-digit pincode #1463

Closed flexsurfer closed 10 months ago

flexsurfer commented 5 years ago

It should be possible to use 6-digit pincode instead of a password

Actual behaviour 5 accounts are generated and 5 public keys sent to status-react -> user chooses one -> enters password -> chosen account is stored with the password

Expected behaviour 5 accounts are generated and 5 public keys sent to status-react -> user chooses one -> enters 6-digit pincode -> chosen account is stored with the pincode

Implementation notes There is a pincode doc describing updated account flows: https://docs.google.com/document/d/1IrjvvAuAQbYsr3xqZvXfgAeeyDZRl_x9KRxWnLcwVuI/edit?ts=5cc831fb When using pins instead of a password, Ethereum key pair will have to be encrypted/decrypted using a keypair generated inside device's TEE, as pins do not have enough enthropy. This needs to happen on Go side as having access to it from JS context is insecure due to multiple dependencies.

status-go should receive an object with 3 methods (initialize, encrypt, decrypt) with native implementations for iOS/Android.

An Android implementation but for JS already exists in our fork of react-native-keychain (https://github.com/status-im/react-native-keychain/commit/43e5512cabb8ee064fd9e503be943dcf2c7d7669), as for iOS, inspiration can be taken from https://github.com/keybase/go-keychain.

Also a nice overview is given here

rachelhamlin commented 5 years ago

@gravityblast @siphiuel hmm found this issue in Wrike and don't know how to organize it. Is it still relevant?

vitvly commented 5 years ago

Yes it is, i've been working on the status-react counterpart to this.

0xc1c4da commented 5 years ago

➤ Nabil Naghdy commented:

We should probably update the date for this then as well

dshulyak commented 5 years ago

@flexsurfer are you the author of the doc? i have a couple of questions, maybe you can help

  1. The app (status-go) creates an Ethereum HD key pair (eE, dE) and encrypts it with e0
    1. The app (status-go) uses Ethereum encryption key (eE) to encrypt the db.

we cannot use asymmetric key to encrypt database. sqlite doesn't support it in any way, and i don't understand how it can. do you propose to derive secret from eE? what should i use as a key material?

  1. The app (status-react) creates a key pair in the secure element (e0, d0)

why do we need assymetric key in secure element? as i understand the only purpose of this key is to encrypt/decrypt HD key. if so then symmetric key is well suited for this task. The documentation for API is somewhat confusing but it looks like i can generate random aes key with specified length and then use it for encryption/decryption. SecKeyCreateEncryptedData behavior changes based on the key type. so if it is possible, i will use symmetric key, is that fine or you see some issues?

flexsurfer commented 5 years ago

hey @dmitryn , no, author is @mandrigin , maybe @siphiuel can answer your question, he researched this document

dshulyak commented 5 years ago

An Android implementation but for JS already exists in our fork of react-native-keychain (status-im/react-native-keychain@43e5512)

i think this implementation uses different API, that is based on password. as i understood we need to generate random key instead of it. or am i wrong?

how secure element handles authorization? how to make key unavailable to any other application? i am trying to find this info in docs, but maybe someone had experience with this already?

dshulyak commented 5 years ago

another question that came up while reading docs, is that why do we need to encrypt HD pair at all if later we will decrypt it and store in memory? we can add any blob of data to secure storage and then retrieve it when needed (e.g on login). if pincode is not correct that blob of data can be removed, so all other workflows described in the doc are exactly the same

vitvly commented 5 years ago

@dshulyak thanks for reviewing the doc! I'll try to answer some of your concerns to the best of my ability

  1. The app (status-react) creates a key pair in the secure element (e0, d0)

You're right, symmetric AES encryption should be enough I think

  1. The app (status-go) uses Ethereum encryption key (eE) to encrypt the db.

It should be derived from Ethereum key pair, not sure how.

vitvly commented 5 years ago

@corpetty maybe you will be able to answer @dshulyak 's questions

dshulyak commented 5 years ago

It should be derived from Ethereum key pair, not sure how.

i will be going for hkdf with sha256 as a hasher and private key as secret material for derivation. let me know if there are concerns about it.

so the last question is pretty much what we will store in secure storage, ethereum key or key that was used to encrypt ethereum key. in my current understanding storing additional key is a redundant step that provides no additional security guarantees. if secure storage is compromised and encryption key is available then attacker can decrypt ethereum key as well.

I made several flow charts to better understand the whole process:

  1. Create account

image

Security.SecItemAdd needs to be made with status application group and unique user tag so that this item can be found later.

  1. Login

image

Authorization is managed using tag and access groups. Per IOS security guide

Keychain items can only be shared between apps from the same developer. Thisis managed by requiring third-party apps to use access groups with aprefix allocated to them through the Apple Developer Program through application groups.

  1. Account locking

image

After several invalid pin attempts we delete key from secure storage, after that user will have to follow recovery process.

  1. Recovery

image

Recovery is made 12-word pass phrase that was returned to user during onboarding.

Both security concerns identified in original doc are addressed.

dshulyak commented 5 years ago

@siphiuel question about pincode. is it a system wide passcode (pincode/fingeprint/face id) or some kind of our pincode? asking to understand what kind of auth options we need to use for ios/adnroid keychain

hesterbruikman commented 5 years ago

FYI here's so more guidance Igor left: https://www.notion.so/Encryption-in-a-secure-element-ccb8778651024440a81f4f1e626a4342 Leaving it up to you to judge the relevance:)

flexsurfer commented 5 years ago

@dshulyak it's our pincode same as password, just 6 digits so we'll have it in status-react as a string "123456"

dshulyak commented 5 years ago

FYI here's so more guidance Igor left: https://www.notion.so/Encryption-in-a-secure-element-ccb8778651024440a81f4f1e626a4342 Leaving it up to you to judge the relevance:)

thank you thats relevant, i just wanted to collect more information, cause it seems like a big change. below is the scope of changes as i see them:

following interface needs to be implemented by java and objective c and provided to golang before node is started:

type Keychain interface {
    SecurityLevel() int
    CreateKey(auth string) error
    DeleteKey(auth string) error
    Encrypt(auth string, dec []byte) (enc []byte, err error)
    Decrypt(auth string, enc []byte) (dec []byte, err error)
}

auth must be unique identifier for the key. it will be used as a tag for ios and alias for android keychains.

flexsurfer commented 5 years ago

cc @gravityblast

0xc1c4da commented 5 years ago

➤ Oskar Thoren commented:

Rachel HamlinCorey Petty What was the outcome of recent call? I'm still confused as to why this is necessary, considering we do the same operation with password right now. It'd be great to have a doc where there's a more clear rationale, considering how this delays v1

0xc1c4da commented 5 years ago

➤ Rachel Hamlin commented:

Oskar Thoren it's not in scope! Missed this task. I'll remove it from code freeze. Corey Petty reached out to his community to see if we could find someone, but the assumption is we can't build it in time.

0xc1c4da commented 5 years ago

➤ Rachel Hamlin commented:

Okay, here's an example of a Wrike issue that I can't remove from its original placement. I just relocated it to v1 - release (optimistically), but I can't delete it from v1 code freeze. The 'Onboarding' label is grayed out.

0xc1c4da commented 5 years ago

➤ Corey Petty commented:

Oskar Thoren Do we actually use TEE where available with current password workflow? I was under the impression we "store" the password there for certain reasons, but encrypt with the password itself, not material generated from the TEE. If this is the case, then the pin was a much more secure flow.

0xc1c4da commented 5 years ago

➤ Oskar Thoren commented:

Corey Petty not familiar enough with the implementation details of it unfortunately to answer that question

status-github-bot[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.