Reintroduce a fix for strongbox decryption data corruption (#383) #385

cshfang · 2020-08-27T23:00:41Z

A PR (#288) a while back was merged to fix an issue where decrypting the keychain using a CipherInputStream causes certain Google Pixel devices to throw an exception and hang.

The change seems to have been inadvertently reverted in the large refactoring that took place in #260.

This PR reintroduces that fix.

aeirola

In general, the code looks good! Would still like to clean it up a bit by removing the mostly unused byte array streams. Otherwise I'm worried they might cause some confusion in the future.

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageBase.java

* Clean up comments referencing input stream * Update handler to expect bytes array instead of input stream * Remove unused resources in try

cshfang · 2020-08-28T17:11:19Z

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageBase.java

@@ -505,7 +504,7 @@ public static String getDefaultAliasIfEmpty(@Nullable final String service, @Non
    public static IvParameterSpec readIv(@NonNull final byte[] bytes) throws IOException {
      final byte[] iv = new byte[IV_LENGTH];

-      if (IV_LENGTH <= bytes.length)


When switching over to this overloaded method handling byte[], this condition was triggered. Reading through this method, I believe the intent may have been to validate that the input bytes array is actually greater than IV_LENGTH.

Whoa, strange bug. Wonder if this code was ever used before.

aeirola · 2020-08-31T06:56:04Z

The code looks good, and normal encoding and decoding seems to work OK on Android. But when trying to get stored values which are protected with fingerprint authentication, I get the a javax.crypto.IllegalBlockSizeException error caused by android.security.KeyStoreException: Invalid input length. Apparently the AES and RSA cipher storages use the same decryptBytes method, and the RSA cipher used for fingerprints doesn't allow decyrpting data using doFinal with arbitrary length input.

Might be that we need to move the modified decryptBytes method from CipherStorageBase to the CipherStorageKeystoreAesCbc class, so that we don't interfere with the workings of the other cipher storage implementations.

This issue wasn't present in the previous fix for the same issue, as the Android fingerprints weren't supported yet at the time.

cshfang · 2020-08-31T17:11:07Z

The code looks good, and normal encoding and decoding seems to work OK on Android. But when trying to get stored values which are protected with fingerprint authentication, I get the a javax.crypto.IllegalBlockSizeException error caused by android.security.KeyStoreException: Invalid input length. Apparently the AES and RSA cipher storages use the same decryptBytes method, and the RSA cipher used for fingerprints doesn't allow decyrpting data using doFinal with arbitrary length input.

Might be that we need to move the modified decryptBytes method from CipherStorageBase to the CipherStorageKeystoreAesCbc class, so that we don't interfere with the workings of the other cipher storage implementations.

This issue wasn't present in the previous fix for the same issue, as the Android fingerprints weren't supported yet at the time.

Ah thanks for the good catch. Based on what you're saying, it makes sense to me to try to make the modified decryptBytes method the exception for AES rather than the norm. I'll work on this today.

* Refactored strongbox decryption changes to apply only to CipherStorageKeystoreAesCbc

cshfang · 2020-09-01T00:31:18Z

@aeirola I pushed a refactor to override decryptBytes within the CipherStorageKeystoreAesCbc class. It seems like the changes are still holding up in my testing but I only tested with a fairly contrived RSA set/get. I'm not sure how to replicate the android.security.KeyStoreException: Invalid input length you found above so may have to lean on you for further testing.

aeirola

Code looks good, and works well in my testing. Might personally prefer to keep the changes even more local to the CipherStorageKeystoreAesCbc class by not using generics for the DecryptBytesHandler interface, but this is more of a matter of taste. Feel free to change it or ignore my comments :)

Before merging I'll try to contact people who've faced the issue and see wether they would be willing to help out testing this PR with actual Titan M devices.

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageBase.java

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAesCbc.java

cshfang · 2020-09-01T08:23:35Z

Before merging I'll try to contact people who've faced the issue and see wether they would be willing to help out testing this PR with actual Titan M devices.

FWIW, I have been testing this against my physical Pixel 4 device as well, which I believe utilizes the Titan M.

aeirola · 2020-09-01T08:40:38Z

@Metroidaron @nicolashemonic Hi, I remember that you've previously helped this project by testing issues on Pixel devices in #208 . Now we're in a situation where we have faced a regression in the support for Pixel devices with Titan M security chips. In order to verify that our fix is valid, I'd like to ask wether you would be able to help out once again by checking if the latest code in this PR still works on your devices and use cases. The code from this PR can be installed with npm install github:cshfang/react-native-keychain#fix-strongbox-decryption. Thanks again!

* Move away from generic typing in DecryptBytesHandler interface * Call readIv directly in AES cipher storage decryptBytes rather than delegating to handler

aeirola · 2020-09-07T05:38:07Z

Unfortunately there wasn't much interest in testing the fix on additional devices. Would have been nice with some extra validations with different devices, but won't let that block the deployment of the fix as the code looks good and you already tested on one Pixel 4 device. I'll merge this and see if we can get it released soon.

aeirola · 2020-09-07T05:41:18Z

@oblador Do we have a schedule for the next release? Releaseing this fix would be useful for a certain group of Android users.

oblador · 2020-09-08T07:30:46Z

@aeirola It's released in 6.2.0!

nicolashemonic · 2020-10-01T16:09:08Z

Hi @aeirola @oblador

Sorry for the silence because I was in long vacation 🏖 and busy when back to the office.

I upgraded my app to [email protected] and I confirm that the fix works well on Pixel 3a Android 11. The authentication (very long token stored in keychain) is not lost when the app is closed and opened.

Thanks !

Reintroduce a fix for strongbox decryption data corruption (oblador#383)

0c8c146

cshfang mentioned this pull request Aug 27, 2020

getGenericCredentials never resolves or rejects due to ShortBufferException in Google Pixel 4 #383

Closed

aeirola self-assigned this Aug 28, 2020

aeirola requested changes Aug 28, 2020

View reviewed changes

aeirola added the bug label Aug 28, 2020

aeirola linked an issue Aug 28, 2020 that may be closed by this pull request

getGenericCredentials never resolves or rejects due to ShortBufferException in Google Pixel 4 #383

Closed

Chris Fang added 2 commits August 28, 2020 09:54

Fix logic in byte[] version of readIv method

138f345

Clean up around removal of CipherInputStream

6c01fcd

* Clean up comments referencing input stream * Update handler to expect bytes array instead of input stream * Remove unused resources in try

cshfang commented Aug 28, 2020

View reviewed changes

Restore decryptBytes implementation in CipherStorageBase

f6b2bd0

* Refactored strongbox decryption changes to apply only to CipherStorageKeystoreAesCbc

aeirola approved these changes Sep 1, 2020

View reviewed changes

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageBase.java Outdated Show resolved Hide resolved

android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAesCbc.java Outdated Show resolved Hide resolved

aeirola mentioned this pull request Sep 1, 2020

Pixel 4 support? #371

Closed

Small refactor in AES cipher storage to forego decrypt handler

e71efbf

* Move away from generic typing in DecryptBytesHandler interface * Call readIv directly in AES cipher storage decryptBytes rather than delegating to handler

aeirola merged commit c3ecdda into oblador:master Sep 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintroduce a fix for strongbox decryption data corruption (#383) #385

Reintroduce a fix for strongbox decryption data corruption (#383) #385

cshfang commented Aug 27, 2020

aeirola left a comment

cshfang Aug 28, 2020

aeirola Aug 31, 2020

aeirola commented Aug 31, 2020

cshfang commented Aug 31, 2020

cshfang commented Sep 1, 2020

aeirola left a comment

cshfang commented Sep 1, 2020

aeirola commented Sep 1, 2020

aeirola commented Sep 7, 2020

aeirola commented Sep 7, 2020

oblador commented Sep 8, 2020

nicolashemonic commented Oct 1, 2020

Reintroduce a fix for strongbox decryption data corruption (#383) #385

Reintroduce a fix for strongbox decryption data corruption (#383) #385

Conversation

cshfang commented Aug 27, 2020

aeirola left a comment

Choose a reason for hiding this comment

cshfang Aug 28, 2020

Choose a reason for hiding this comment

aeirola Aug 31, 2020

Choose a reason for hiding this comment

aeirola commented Aug 31, 2020

cshfang commented Aug 31, 2020

cshfang commented Sep 1, 2020

aeirola left a comment

Choose a reason for hiding this comment

cshfang commented Sep 1, 2020

aeirola commented Sep 1, 2020

aeirola commented Sep 7, 2020

aeirola commented Sep 7, 2020

oblador commented Sep 8, 2020

nicolashemonic commented Oct 1, 2020