status-go icon indicating copy to clipboard operation
status-go copied to clipboard

[Onboarding] Store keys with the 6-digit pincode

Open flexsurfer opened this issue 5 years ago • 21 comments

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

flexsurfer avatar May 14 '19 10:05 flexsurfer

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

rachelhamlin avatar Jun 25 '19 08:06 rachelhamlin

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

vitvly avatar Jun 25 '19 09:06 vitvly

➤ Nabil Naghdy commented:

We should probably update the date for this then as well

0xc1c4da avatar Jun 25 '19 21:06 0xc1c4da

@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 5. 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?

dshulyak avatar Jul 03 '19 05:07 dshulyak

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

flexsurfer avatar Jul 03 '19 05:07 flexsurfer

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 avatar Jul 03 '19 07:07 dshulyak

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

dshulyak avatar Jul 03 '19 08:07 dshulyak

@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 avatar Jul 03 '19 10:07 vitvly

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

vitvly avatar Jul 03 '19 10:07 vitvly

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 avatar Jul 03 '19 10:07 dshulyak

@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

dshulyak avatar Jul 03 '19 12:07 dshulyak

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:)

hesterbruikman avatar Jul 03 '19 13:07 hesterbruikman

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

flexsurfer avatar Jul 03 '19 13:07 flexsurfer

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:

  • [ ] new keystore that will use keychain interface for encrypting files before storing them on disk
  • [ ] refactor accounts manager to work with new keystore
  • [ ] add implementation for keychain interface that will use passwords for auth, as desktop won't be always able to use keychain
  • [ ] objective c keychain implementation
  • [ ] java keychain implementation

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.

dshulyak avatar Jul 04 '19 07:07 dshulyak

cc @gravityblast

flexsurfer avatar Jul 04 '19 07:07 flexsurfer

➤ 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 avatar Jul 18 '19 10:07 0xc1c4da

➤ 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 avatar Jul 18 '19 11:07 0xc1c4da

➤ 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 avatar Jul 18 '19 11:07 0xc1c4da

➤ 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 avatar Jul 18 '19 11:07 0xc1c4da

➤ Oskar Thoren commented:

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

0xc1c4da avatar Jul 22 '19 09:07 0xc1c4da

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.

status-github-bot[bot] avatar Aug 05 '21 16:08 status-github-bot[bot]