EOSIO / eosio-swift-vault

Utility library for managing keys and signing with Apple's Keychain and Secure Enclave
https://eosio.github.io/eosio-swift-vault/
MIT License
12 stars 8 forks source link

Importing external key and setting BioFactor to .flex or .fixed seems to break requireBio: true, when signing. #56

Open ismyhc opened 4 years ago

ismyhc commented 4 years ago

When importing a private key using vault, Im getting some unexpected results. When setting the BioFactor to .flex (.biometryAny) or to .fixed (.biometryCurrentSet).

try vault.addExternal(eosioPrivateKey: privateKey, protection: .whenUnlocked, bioFactor: .fixed, metadata: nil)

Then when signing and broadcasting a transaction:

self.transaction.signatureProvider = EosioVaultSignatureProvider(accessGroup: "group.lynx", requireBio: true)

Im not prompted for authentication. However, if I set bioFactor to .none, it works as expected.

On another note. Im my own current keychain implementation, I use the setting .userPresence to constrain the keychain item. This sets the flag on insertion automatically requires the item to be authenticated when accessing. Doing it this way would remove the need to specify requiring bio when doing anything with the stored item. It also seems this would be more secure as the bio requirement would be at the keychain query level and storage method and not leaving it up to the the accessing method. Im by far a keychain expert, but if this is possible with the current vault implementation?

My implementation follows the section "Demand User Presence".

https://developer.apple.com/documentation/security/keychain_services/keychain_items/restricting_keychain_item_accessibility

opi-smccoole commented 4 years ago

We will have to look into why it's not presenting a challenge with an imported key.

The reason we do not constrain the keychain items at insertion is to prevent the user from losing access to the keys if they change their biometric identification information. If they update their biometric data and the identifiers that were used when the keychain item were created are no longer present or overwritten the user no longer has access to the key(s) and that could result in lost access to accounts.

ismyhc commented 4 years ago

We will have to look into why it's not presenting a challenge with an imported key.

The reason we do not constrain the keychain items at insertion is to prevent the user from losing access to the keys if they change their biometric identification information. If they update their biometric data and the identifiers that were used when the keychain item were created are no longer present or overwritten the user no longer has access to the key(s) and that could result in lost access to accounts.

Thanks for the quick response. Isn't the point of using .biometryAny or .userPresence to allow the user to change their bio/passcode information without losing access? According to apples documentation, using .userPresence, would allow the user to change the bio/passcode settings without losing access. I can confirm this, because this is how I use it in my current keychain implementation.

Here's the documentation describing .userPresence and the ability to re-enroll without losing access. https://developer.apple.com/documentation/security/secaccesscontrolcreateflags/1392879-userpresence

Im not certain, but I think this is also the case for .biometryAny. Where the user would loose access is when using . biometryCurrentSet. As this setting enforces the item to only be accessible with the current set credentials.

opi-smccoole commented 4 years ago

I don't think we want a default that falls back to passcode since it can be set very insecurely still.

ismyhc commented 4 years ago

I don't think we want a default that falls back to passcode since it can be set very insecurely still.

Sure, I understand that. But as it is now, by default I can do this:

let _ = try vault.addExternal(eosioPrivateKey: privateKey)

let key = try getVaultKey(eosioPublicKey: publicKey) <- access key without any authentication.

And never get prompted. That to me seems more insecure.

What I'd like to see is that when importing a key and passing bio argument which corresponds to .biometryAny, that it actually be used when inserting the key into the keychain. Per the apple documentation, this would allow the user to re/enroll without loss of access.

So, in this case I would expect if I do

let _ = try vault.addExternal(eosioPrivateKey: privateKey, bioFactor: .flex)

let key = try getVaultKey(eosioPublicKey: publicKey) <- When I do this it should prompt for authentication.
opi-smccoole commented 4 years ago

Sorry for the delay! I have looked into this and wanted to ask one additional question. Are these K1 keys? Thanks!

ismyhc commented 4 years ago

Sorry for the delay! I have looked into this and wanted to ask one additional question. Are these K1 keys? Thanks!

Yup k1 keys. 🙂

opi-smccoole commented 4 years ago

OK, thanks. I know what the issue is then and we can work on a fix.

opi-smccoole commented 4 years ago

Just a quick update. We are discussing how we want to address this issue. It has to do with the use of software signing for K1 keys and software biometric checks during the signing process. We want to make sure that any changes we make don't alter the signing and key access flows in ways that might be unexpected to developers. We will put another update here soon when we determine our direction.

opi-smccoole commented 4 years ago

Sorry for the long delay on this! The issue that we've encountered is that when we fix the application of the biometric factor to the imported key, the import process does several read-backs and this triggers the biometric challenges, which is not what we want to happen. We're looking into what will be necessary to remove those from the flow to avoid the issue and implement a fix.