Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
brian-weasner opened this issue Nov 29, 2022 · 6 comments

Comments

@brian-weasner
Copy link
Contributor

Your Environment

  • Plugin version: 4.1.3
  • Platform: Android
  • Device OS version: 13
  • Device manufacturer / model: Google Pixel 6a
  • XCode version: N/A
  • Capacitor info (npx cap doctor)
       Capacitor Doctor
    
    Latest Dependencies:
    
      @capacitor/cli: 4.5.0
      @capacitor/core: 4.5.0
      @capacitor/android: 4.5.0
      @capacitor/ios: 4.5.0
    
    Installed Dependencies:
    
      @capacitor/cli: 4.5.0
      @capacitor/core: 4.5.0
      @capacitor/android: 4.5.0
      @capacitor/ios: 4.5.0
    

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:

@bertrand-ravier
Copy link

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

@brian-weasner
Copy link
Contributor Author

brian-weasner commented Dec 7, 2022

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
Copy link

@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
Copy link
Contributor Author

@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
Copy link

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants