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

[WIP] Android fingerprint #148

Closed
wants to merge 13 commits into from
Closed

Conversation

LinusU
Copy link

@LinusU LinusU commented Sep 12, 2018

Opening early for feedback.

Fixes #116

@vonovak vonovak self-requested a review September 12, 2018 13:32
.setEncryptionPaddings(ENCRYPTION_PADDING)
.setRandomizedEncryptionRequired(true)
.setUserAuthenticationRequired(true)
.setUserAuthenticationValidityDurationSeconds(-1)
Copy link
Contributor

@Jyrno42 Jyrno42 Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be at least 2 seconds. Otherwise the decryption will always fail.

To work around the issue we need to pass in a CryptoObject to the .authenticate call (as mentioned here #116 (comment)) but I could not figure out myself how to do it. I can't seem to be able to figure out how to pass the Cipher to CryptoObject since it needs to be initialized, and to initialize it user needs to be authenticated... Hopefully @cladjules will figure that one out.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why we need to get the crypto object out of encryptBytes,
getting the instance of the Cipher should be fine.

Then, try to call cipher.init, that should throw the UserNotAuthenticatedException (or not).

I will give it a go on that PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional ... setIsStrongBoxBacked

@Jyrno42
Copy link
Contributor

Jyrno42 commented Sep 12, 2018

FYI: Found some issues w/ upgrading and when user removes their lock screen:

thorgate@1f9bcdb

E: Updated the commit

Jyrno42 and others added 2 commits September 18, 2018 11:07
Since AES and RSA share the same key space the upgrade path
fails cause the RSA class gets a false positive for containsAlias call.

Currently I just remove the AES key before encrypting with rsa key, however
this could potentially result in a situation where the aes key gets removed
but RSA key generation/encryption fails. This would cause data loss.

I though about an alternative prefix solution for the server but maybe theres even
 a better way to do it.

Also:

- Handle case where key has been permanently invalidated
@oblador
Copy link
Owner

oblador commented Sep 30, 2018

So amazing seeing progress on this feature. What's the current status?

@cladjules
Copy link

@oblador I will finish integration next week. Sorry been so busy last 2 weeks...

@oblador
Copy link
Owner

oblador commented Sep 30, 2018

Know that all to well, didn't mean to rush you 😺

@Jyrno42
Copy link
Contributor

Jyrno42 commented Oct 11, 2018

I noticed that with my original implementation if the user fails fingerprint authentication (e.g. uses wrong finger) the app will crash with CalledFromWrongThreadException. Not sure if that is also true for the implementation here, but something to beware of.

android.view.CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views.
    at android.view.ViewRootImpl.checkThread(ViewRootImpl.java:8530)
    at android.view.ViewRootImpl.requestLayout(ViewRootImpl.java:1432)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.view.View.requestLayout(View.java:23221)
    at android.widget.TextView.checkForRelayout(TextView.java:9789)
    at android.widget.TextView.setText(TextView.java:6023)
    at android.widget.TextView.setText(TextView.java:5849)
    at android.widget.TextView.setText(TextView.java:5806)
    at moe.feng.support.biometricprompt.BiometricPromptApi23Impl$FingerprintManagerAuthenticationCallbackImpl.onAuthenticationHelp(BiometricPromptApi23Impl.java:238)
    at android.hardware.fingerprint.FingerprintManager$MyHandler.sendAcquiredResult(FingerprintManager.java:1364)
    at android.hardware.fingerprint.FingerprintManager$MyHandler.handleMessage(FingerprintManager.java:1220)
    at android.os.Handler.dispatchMessage(Handler.java:105)
    at android.os.Looper.loop(Looper.java:164)
    at android.app.ActivityThread.main(ActivityThread.java:6944)
    at java.lang.reflect.Method.invoke(Method.java)
    at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:327)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1374)

E: I have not yet been able to resolve that, so tips are welcome.

@LinusU
Copy link
Author

LinusU commented Oct 11, 2018

E: I have not yet been able to resolve that, so tips are welcome.

I solved that by posting runnables to anything that's visible in the interface, don't remember exactly where I saw that trick 🤔

https://github.com/LinusU/react-native-biopass/blob/60c9513668639358b864c03235c6112669b00f65/android/src/main/java/com/reactlibrary/RNBioPassDialog.java#L171-L176

@Jyrno42
Copy link
Contributor

Jyrno42 commented Oct 17, 2018

E: I have not yet been able to resolve that, so tips are welcome.

I solved that by posting runnables to anything that's visible in the interface, don't remember exactly where I saw that trick

https://github.com/LinusU/react-native-biopass/blob/60c9513668639358b864c03235c6112669b00f65/android/src/main/java/com/reactlibrary/RNBioPassDialog.java#L171-L176

Yup that worked, thanks for the tip.

see: thorgate/BiometricPromptCompat@776801e

@LinusU
Copy link
Author

LinusU commented Oct 18, 2018

Google has now released an official compat version of BiometricPrompt 🎉 🙌

https://developer.android.com/reference/androidx/biometrics/BiometricPrompt

We should probably try and use that one instead 👍

@cladjules
Copy link

Sorry for not being active recently.
That's a really good news as I had some issue integrating the compat from github.
I will definitely give it a go with that library by the end of the week.

@Jyrno42
Copy link
Contributor

Jyrno42 commented Oct 18, 2018

Sorry for not being active recently.
That's a really good news as I had some issue integrating the compat from github.
I will definitely give it a go with that library by the end of the week.

Looking forward to that. Tried integrating with it in my temporary branch myself, but had some issues.

In the end I just ended up fixing the crash issue I had with BiometricPromptCompat for now.

@cladjules
Copy link

Google has now released an official compat version of BiometricPrompt 🎉 🙌

https://developer.android.com/reference/androidx/biometrics/BiometricPrompt

We should probably try and use that one instead 👍

Apparently, we need to migrate the project to use AndroidX. I am giving it a go and will see how it goes :)

throw new CryptoFailedException("Unknown error: " + e.getMessage(), e);
}
}

Copy link

@earonesty earonesty Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... be cool to provide a getPublicKey function, so the user can pass in properly formatted public key encrypted bytes from, say, another device. Separate this into its own public function call.

public byte[] getPublicKey(@NonNull String service) { 
   KeyStore keyStore = getKeyStoreAndLoad();
   generateKeyAndStoreUnderAlias(service);
   KeyFactory keyFactory = KeyFactory.getInstance(ENCRYPTION_ALGORITHM);
   PublicKey publicKey = keyStore.getCertificate(service).getPublicKey();
   return publicKey.getEncoded();
}

public static final String KEYSTORE_TYPE = "AndroidKeyStore";
public static final String ENCRYPTION_ALGORITHM = KeyProperties.KEY_ALGORITHM_RSA;
public static final String ENCRYPTION_BLOCK_MODE = KeyProperties.BLOCK_MODE_ECB;
public static final String ENCRYPTION_PADDING = KeyProperties.ENCRYPTION_PADDING_RSA_PKCS1;
Copy link

@earonesty earonesty Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually, these really should be options. OAEP is the default on many systems now, and is considered more secure, for example. Likewise 1024 bit RSA is ... well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Google has now released an official compat version of BiometricPrompt 🎉 🙌
https://developer.android.com/reference/androidx/biometrics/BiometricPrompt
We should probably try and use that one instead 👍

Apparently, we need to migrate the project to use AndroidX. I am giving it a go and will see how it goes :)

Still trying to get it working with androidx. I am not sure if we can just get that library upgraded using androidx, or we need the whole project, React-native doesn't support it yet in any case.

I get a lot of multidex error atm.

@Timm0
Copy link

Timm0 commented Nov 14, 2018

Any further progress here?

@cladjules
Copy link

Any further progress here?

Still working on integrating the new compat library.

It's quite a big change for the whole Android project to move to androidx and get rid of the compat library.

We need to decide whether or not, we will want to force users to move their Android project to use AndroidX.

Android encourages the developer to move their project anyway, but React-native didn't indicate anything regarding this yet.

If we don't go to the route using androidx, we will need to stick to the old compat library (which is now deprecated and suggest using androidx biometric).

@cladjules
Copy link

Ok, I finally managed to do something.
As it was too difficult to convert the whole React-native project to AndroidX, I decided to extract the library from the source code and integrate it and refactor it using android support v4.

It's now fully working and show dialog for both Android P and prior to that.

I have committed all the code to my fork, I am not sure what is the best way to merge back into that PR, there are few commits on that PR that are not in my fork.
Anyone can advise?

You can review the PR here: https://github.com/cladjules/react-native-keychain

@brunobar79
Copy link
Contributor

brunobar79 commented Nov 22, 2018

@cladjules I'd start by fixing the conflicts that you have against master of this repo (See master...cladjules:master)

then maybe @LinusU, @earonesty and @oblador can take a look ?

BTW, Huge thanks for working on this!

@cladjules
Copy link

@Jyrno42 regarding "This needs to be at least 2 seconds. Otherwise, the decryption will always fail."

Not necessarily, the issue is, when you pass the crypto object to use it to decode, you can only use it once. It means that username and password would need to be encrypted within the same byte[] in order to work.

Setting 2 seconds is working, as you could use ANY cipher within those 2 seconds, but it's a bit a hack...

@brunobar79
Copy link
Contributor

@cladjules I've forked your repo, gave it a try and got it working!

The biggest change I had to make was on my app side, switchingMainActivity to extend from ReactFragmentActivity instead of ReactActivity.
The main problem with it is that ReactFragmentActivity will be deprecated soon

I understand that it's a requirement from the BiometricPrompt on AndroidX, but I'm curious about how did you deal with that? If anyone else knows lmk!

I have some time to help with this on my end this week so if you guys want just let me know and I can open a PR of this soon to start a new code review (since the implementation from @cladjules is different enough than the code in this PR).

@LinusU
Copy link
Author

LinusU commented Dec 17, 2018

@brunobar79 ReactFragmentActivity is getting deprecated because the ReactActivity is going to be a FragmentActivity

It seems like the PR for this might finally be landing 🙌

facebook/react-native#22662

@cladjules
Copy link

@cladjules I've forked your repo, gave it a try and got it working!

The biggest change I had to make was on my app side, switchingMainActivity to extend from ReactFragmentActivity instead of ReactActivity.
The main problem with it is that ReactFragmentActivity will be deprecated soon

I understand that it's a requirement from the BiometricPrompt on AndroidX, but I'm curious about how did you deal with that? If anyone else knows lmk!

I have some time to help with this on my end this week so if you guys want just let me know and I can open a PR of this soon to start a new code review (since the implementation from @cladjules is different enough than the code in this PR).

@brunobar79 I remember doing that change too, I don't really like the fact the fragment is directly added to the main react view. I am wondering if it's possible to launch a new activity and attach to it... I will give it a go.

@brunobar79
Copy link
Contributor

brunobar79 commented Dec 17, 2018 via email

@Timm0
Copy link

Timm0 commented Jan 5, 2019

Is there any further progress here?

@cladjules
Copy link

Is there any further progress here?

@Timm0 I believe my branch is stable, there are few minor issues regarding fragment that I will push a fix for,
also, I need to remove the dependencies using SupportFragment, so I will refactor a bit of code.

@SudoPlz
Copy link

SudoPlz commented Feb 13, 2019

Hmm, I'm curious, what's the status on this? Is it just waiting for a review?

@cladjules
Copy link

I have created a separate PR with all the code from that PR

@vchernobyl
Copy link

Thanks guys for the good work! 🥇 Quick question - is there a particular reason why you chose RSA encryption for the authentication with the fingerprint instead of AES? In a lot of use cases we don't want to store an encrypted username/password but for example an encrypted JSON Web Token instead, that could be a few thousand characters long. This data size is way too large for RSA to encrypt, since max key length is 4096 bits and hence it is able to encrypt up to 512 chars (even less if padding is used). As it stands now the CipherStorageKeystoreRSAECB class throws a IllegalBlockSizeException when trying to encrypt a string longer than 384 chars. Was/is there some kind of Android SDK limitation regarding AES encryption for fingerprint authentication? AES is fast, secure and is able to encrypt strings of arbitrary lengths (it is also used in iOS Keychain).

@aeirola aeirola mentioned this pull request Apr 8, 2019
@cladjules
Copy link

Thanks guys for the good work! 🥇 Quick question - is there a particular reason why you chose RSA encryption for the authentication with the fingerprint instead of AES? In a lot of use cases we don't want to store an encrypted username/password but for example an encrypted JSON Web Token instead, that could be a few thousand characters long. This data size is way too large for RSA to encrypt, since max key length is 4096 bits and hence it is able to encrypt up to 512 chars (even less if padding is used). As it stands now the CipherStorageKeystoreRSAECB class throws a IllegalBlockSizeException when trying to encrypt a string longer than 384 chars. Was/is there some kind of Android SDK limitation regarding AES encryption for fingerprint authentication? AES is fast, secure and is able to encrypt strings of arbitrary lengths (it is also used in iOS Keychain).

@vchernobyl we need an algorithm that handles private/public key pair, as it needs to be encrypted with a public key (without fingerprint) and decrypted with the private key (using fingerprint), if you use an algorithm that doesn't use a pair, you will need to prompt fingerprint for both encrypting and decrypting.

@anagar23
Copy link

@cladjules , I have used your cladjules:master repo to integrate it. It work nicely but I have to change ReactActivity to ReactFragmentActivity as I am on lower version of React Native. Do we have some other way to fix It ?

Also Do we any early plan to merge this pull request in Main repo ?

@cladjules
Copy link

@cladjules , I have used your cladjules:master repo to integrate it. It work nicely but I have to change ReactActivity to ReactFragmentActivity as I am on lower version of React Native. Do we have some other way to fix It ?

Also Do we any early plan to merge this pull request in Main repo ?

@anagar23 Yeah that's the issue, I had to change to ReactFragmentActivity as well and I think React-Native deprecated that.
I will have a look at the weekend, see how to switch that.

@EnricoMazzu
Copy link

Hi guys,

i don't know if this could help you, but i've created an Android library that provides an embedded ui for biometric authentication (without androidX). This library works from api level 23 to 29 (starting to android 28 use the biometricPrompt APIs). it also provides and helps to manage JCA crypto operation.

Github repo (with minimal docs and screenshots)
https://github.com/EnricoMazzu/Biometric-Auth-Compat

@jasonkw9
Copy link

jasonkw9 commented Dec 3, 2019

Any updates on this?

@oblador
Copy link
Owner

oblador commented Mar 4, 2020

Superseded by #260

@oblador oblador closed this Mar 4, 2020
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

Successfully merging this pull request may close these issues.

Android: Fingerprint authentication not implemented yet