search

Crypho / cordova-plugin-secure-storage

Secure storage plugin for Apache Cordova
MIT License
278 stars 269 forks source link

Do not rely on internal Android API for screen unlock #163

Closed eranmes closed 5 years ago

eranmes commented 5 years ago

SecureStorage relies on internal Android API to unlock the screen: https://github.com/Crypho/cordova-plugin-secure-storage/blob/master/src/android/SecureStorage.java#L198

It should use public API instead: https://developer.android.com/reference/android/app/KeyguardManager#createConfirmDeviceCredentialIntent(java.lang.CharSequence,%20java.lang.CharSequence)

Side note: The code needs to unlock Keystore because of the use of the following option when generating the key: https://developer.android.com/reference/android/security/KeyPairGeneratorSpec.Builder.html#setEncryptionRequired()

Note that this is deprecated in favour of using KeyGenParameterSpec (https://developer.android.com/reference/android/security/keystore/KeyGenParameterSpec.Builder) which does not offer the option to encrypt the key at-rest.

You could get rid of the need to unlock the screen and the logic accompanying it by switching to KeyGenParameterSpec or at least not call setEncryptionRequired on the KeyPairGeneratorSpec.

eranmes commented 5 years ago

A bit of background: The request to encrypt the generated key (setEncryptionRequired) made sense years ago, when Android device filesystems were unencrypted. Now, with file-based encryption (or full-disk encryption), the user's filesystem data (including Keystore keys) is encrypted using the user's screen lock method and that is handled at a different layer than Keystore - so there's no benefit to encrypting Keystore keys any longer.

demetris-manikas commented 5 years ago

@eranmes Thanks for your input.

This plugin supports API 21.

KeyguardManager.createConfirmDeviceCredentialIntent was introduced in API 21 and should be used by this plugin as you suggest. A PR on that would be welcome.

KeyGenParameterSpec.Builder was introduced in API 23 so using it is not yet possible.

Dropping the call to setEncryptionRequired is out of the question since it would leave the encryption keys in plain text in the Keystore. Also note that setEncryptionRequired has the same effects to KeyGenParameterSpec.Builder.setUserAuthenticationRequired which will certainly be used when we come to the point of using KeyGenParameterSpec.Builder.

hvaughan3 commented 5 years ago

I just received some crash reports in our logs that fail when running startActivity with Intent intent = new Intent("com.android.credentials.UNLOCK");.

The crash logs are coming from a Pixel XL device running Android 10 (I guess they are running the developer preview version).

Perhaps Android 10 cut support for this?

Fatal Exception: android.content.ActivityNotFoundException: No Activity found to handle Intent { act=com.android.credentials.UNLOCK } at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:2047) at android.app.Instrumentation.execStartActivity(Instrumentation.java:1705) at android.app.Activity.startActivityForResult(Activity.java:5054) at org.apache.cordova.CordovaActivity.startActivityForResult(CordovaActivity.java:343) at android.app.Activity.startActivityForResult(Activity.java:5012) at android.app.Activity.startActivity(Activity.java:5383) at android.app.Activity.startActivity(Activity.java:5351) at com.crypho.plugins.SecureStorage.startActivity(SecureStorage.java:265) at com.crypho.plugins.SecureStorage.access$700(SecureStorage.java:21) at com.crypho.plugins.SecureStorage$6.run(SecureStorage.java:255) at android.os.Handler.handleCallback(Handler.java:873) at android.os.Handler.dispatchMessage(Handler.java:99) at android.os.Looper.loop(Looper.java:209) at android.app.ActivityThread.main(ActivityThread.java:7021) at java.lang.reflect.Method.invoke(Method.java) at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:486) at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:872)

demetris-manikas commented 5 years ago

@hvaughan3 Thanks for reporting.

lunedam-git commented 5 years ago

Just to add to the comment by @hvaughan3 , I see the same issue when using API 29 (Android 10) on an emulator:

06-05 20:30:59.050 7882 7882 E AndroidRuntime: android.content.ActivityNotFoundException: No Activity found to handle Intent { act=com.android.credentials.UNLOCK } 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Instrumentation.checkStartActivityResult(Instrumentation.java:2051) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Instrumentation.execStartActivity(Instrumentation.java:1709) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Activity.startActivityForResult(Activity.java:5173) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at org.apache.cordova.CordovaActivity.startActivityForResult(CordovaActivity.java:343) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Activity.startActivityForResult(Activity.java:5131) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Activity.startActivity(Activity.java:5502) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at android.app.Activity.startActivity(Activity.java:5470) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at com.crypho.plugins.SecureStorage.startActivity(SecureStorage.java:209) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at com.crypho.plugins.SecureStorage.access$700(SecureStorage.java:21) 06-05 20:30:59.050 7882 7882 E AndroidRuntime: at com.crypho.plugins.SecureStorage$4.run(SecureStorage.java:199)

ggozad commented 5 years ago

The plugin is no-longer maintained closing all issues that are not going to be resolved.

mibrito707 commented 5 years ago

@eranmes & @demetris-manikas In a effort to keep this project alive I created a fork of this repo using your suggestions, check this comment: https://github.com/Crypho/cordova-plugin-secure-storage/issues/169#issuecomment-509734410 for more details.

Adza93 commented 5 years ago

Guys.... 😂 Looks like Google just returned "com.android.credentials.UNLOCK" on Android Q Beta 5 build.

I have just now performed update from latest Android 9 to Android 10 qpp5 which was just released today via side loading ota file (there are some issues with Google system update ota procedure, so Google temporarily disabled it). When installation completed, I started my app just from curiosity, and this app version still initializes this specific activity.

I did not test this via debugger, which will I do today, but looking like this from release state of build, looks like Google returned what they forgot from previous version 😄.

More info later today.

eranmes commented 5 years ago

I do not believe that is correct - the com.android.credentials.UNLOCK intent has not been returned.

Adza93 commented 5 years ago

@eranmes it is definitely back. I have just tested it via debugger on my Google Pixel 3 device, and with latest blueline-ota-qpp5.190530.014-c43d3075.zip OTA software update I no longer have android.credentials.unlock exception.

I have only added printout to Android BUILD and VERSION number, so you can see that I am not wrong here. Image from debugger output https://ibb.co/6mWwHtG

I guess you have not checked, or on your device with this specific Android Q beta update you do not get this resolved?

Adza93 commented 5 years ago

I am telling this only because of backward compatibility. Otherwise it is great to move on and deprecate this chunk of code as some members already did, to prevent this happening again :)