epicshaggy / capacitor-native-biometric

185 stars 95 forks source link

Android: Able to get credentials without verifying/authorizing user with biometrics #80

Open brian-weasner opened 1 year ago

brian-weasner commented 1 year ago

Your Environment

Expected Behavior

I should get errors when trying to get/set/delete credentials if I have not yet verified my identity

Actual Behavior

I am able to get/set/delete credentials without verifying my identity.

I know that I can add in a validation check and then not get/set/delete credentials, however with web inspection it is very easy to change continue to get credentials regardless of validation success/failure.

Steps to Reproduce

  1. Call setCredentials, getCredentials, or deleteCredentials without first calling verifyIdentity
  2. See that the functions execute successfully

For more proof that I've not yet authorized with biometrics, simply close all apps, reboot the phone, login for the first time on boot and see that the credentials can be set, retrieved, and deleted without verifying identity with biometrics.

Context

I don't want to be able to get credentials without verifyIdentity passing successfully.

Potential Fix

Based on my research into this we may need to call more functions when using KeyGenPaameterSpec.Builder Specifically:

Optionally:

Tramb-dev commented 1 year ago

I agree with you, it is a security concern that someone can access user's credentials without biometric authentication.

brian-weasner commented 1 year ago

The way it currently works is that the credential values are encrypted and saved in an app specific private "Shared Preferences".

The Encryption key is saved in an app specific KeyStore. So unless you are the app you shouldn't be able to access the Encryption Key or the Shared Preferences where these secrets are located.

The way you would want to code this is to make sure identity has been verified before performing any of the get/set calls. AND making sure that you do NOT have webContentsDebuggingEnabled within your Capacitor Config. This should be defaulted to false on production builds by default and should "help" harden security a bit against users that know how to inspect webviews on mobile devices.

CharlieJ213 commented 1 year ago

@brian-weasner if it helps, I've written my own plugin which uses this native flow. It doesn't allow access to any Keychain/Keystore properties until biometric authentication has been completed

brian-weasner commented 1 year ago

@CharlieJ213 does your plugin also allow for multiple key values? I see that you have key and value in your api documentation, but not sure if it works similarly to this plugin where the key is the "server" key for the record

CharlieJ213 commented 1 year ago

@brian-weasner

For Android "key" is used as the literal key in shared preferences, on iOS it uses the kSecAttrAccount for kSecClassGenericPassword.

I designed it to work like the Capacitor Preferences API so you can store { key: string, value: string }. If you try to get/overwrite on iOS you will need to use FaceID/TouchID. For android any interaction (except delete) with the keystore will require biometric authentication

As you can see though the plugin only has 1 iteration so I am actively seeking advice / issues / suggestions to improve because I realise there may be things I missed or things I could have done better

brian-weasner commented 1 year ago

@CharlieJ213 I'll take a look at it once I get back around to implementing biometrics. I didn't want to go forward with any of the biometric stuff in my app until my pr for this plugin was commented on by the plugin author.