oblador / react-native-keychain

:key: Keychain Access for React Native
MIT License
3.19k stars 520 forks source link

Cannot decrypt password when length is too long on some Android devices. #208

Closed timbjarengren closed 4 years ago

timbjarengren commented 5 years ago

Hi,

We are having an issue with decrypting a lengthy password when using a Pixel 3 and when using the latest version of react-native 0.59.4. It works on any other device we tested using this version, but not on a Pixel 3. And it works on the Pixel 3 when using react-native 0.57.1, which is currently used in your example application. We have not tried any other react-native versions. We have been able to reproduce it on the Pixel 3, but cannot assume it is only present on that device.

We have created a repo with the Keychain Example app from this repo, but have updated it to the latest react-native version. Please see the repo readme for how to use the example: https://github.com/timbjarengren/react-native-keychain-test-059

Our use case is that we save json containing accessToken and refreshToken using setGenericPassword. However, when this data is too long, a Pixel 3 won't be able to decrypt the data. Is this a valid use case?

This issue might be related, since the error message "could not decrypt bytes" is the same message we see as well:

If you need any more information, please let us know. Hopefully this can get resolved soon!

vonovak commented 5 years ago

hi @timbjarengren, thanks for reporting. A PR that fixes the issue or somehow gracefully handles it would be appreciated, speaking for myself, I definitely won't have time to look into this.

timbjarengren commented 5 years ago

I would like to create a PR, but I do not fully understand the issue to be able to do that yet. I did some more digging now and when I turned off StrongBox security key generation and instead use regular security key generation I got it to work.

More precisely, I changed generateKeyAndStoreUnderAlias to:

private void generateKeyAndStoreUnderAlias(@NonNull String service, SecurityLevel requiredLevel) throws NoSuchAlgorithmException, NoSuchProviderException, InvalidAlgorithmParameterException, CryptoFailedException {
    SecretKey secretKey = tryGenerateRegularSecurityKey(service);

    if(!validateKeySecurityLevel(requiredLevel, secretKey)) {
        throw new CryptoFailedException("Cannot generate keys with required security guarantees");
    }
}

This method was added in https://github.com/oblador/react-native-keychain/commit/01755de085b6e7725e565bf5d4dd0be06da1414a

Got the idea to turn off Strongbox after reading this post: https://proandroiddev.com/android-keystore-what-is-the-difference-between-strongbox-and-hardware-backed-keys-4c276ea78fd0 Talking about difference between the old hardware backed keys, and the new StrongBox.

Might also be the reason why it works in older react-native versions, since 0.59 was the first version to support Android 9 (API 28) (https://github.com/pvinis/rn-diff-purge/compare/release/0.58.6..release/0.59.0), and that's the Android version adding StrongBox.

Any insight from @oblador and @mandrigin about this?

I will investigate some more, to see if I can better understand the issue. (For context, I have mostly done iOS development in recent years, so my Android knowledge is lagging behind. Hoping to catch up.)

Jrodseth commented 5 years ago

Also seeing aberrant behavior on a Pixel 3 with a similar use case - storing id and access tokens via setInternetCredentials. Retrieving the credentials results in corrupted strings.

Based on the investigation done by @timbjarengren it's seems likely this problem will become more widespread with hardware adoption. While a fix isn't implemented, projects that use this package to store long credentials will have to deal with the fact that running on a growing Android hardware subset will simply not work as expected.

mvayngrib commented 5 years ago

does anyone know how many characters can safely be stored?

vikeri commented 5 years ago

I implemented @timbjarengren's suggestion and it seems to work for me now. If anyone needs a quick fix they can use "Pilloxa/react-native-keychain#70d7914" in their package.json until this gets a proper fix. The commit is here: https://github.com/Pilloxa/react-native-keychain/commit/70d79143161a00d7f3049eaef2b43cda3673f5e7 I can't take any responsibility for people that use that fork though.

TomMahle commented 5 years ago

@vikeri Do you have any intention of submitting that as a pull request? I'm not sure what shape a proper fix would take, and it seems likely to me that having no Strongbox usage is better than having the possibility of crashes for Strongbox users for the majority of consumers of this package (it certainly would be the case for me :D).

mandrigin commented 5 years ago

@vikeri @TomMahle what if you try to increase the buffer size here? I don't have a pixel 3 to test now, but maybe it will help. https://github.com/oblador/react-native-keychain/blob/master/android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAESCBC.java#L234? will that help?

TomMahle commented 5 years ago

@mandrigin I gave that a try to no avail. I also tried hard coding a smaller value for reading into the buffer and also removing the 0 and buffer length optional parameters. All of those resulted in the same error with large data sizes on a Pixel 3. Removing Strongbox usage does solve the issue, however.

vikeri commented 5 years ago

@TomMahle Definitely, I'd need a maintainer on this lib to agree that that fix is a reasonable one though before I'll submit the PR.

zyofeng commented 5 years ago

Vikeri's fix seems to be working for Pixel 3A. can someone get this merged in?

mashi-mashi commented 5 years ago

Pixel3a does not work correctly if it exceeds 854 bytes. I also confirmed that Pixel3a works with Vikeri's fix. Are there any side effects of quitting Strongbox?

nicolashemonic commented 5 years ago

Trying to save Tokens > 1000 bytes I experience same issue on Pixel 3A API 28 RN 0.59.8

@mandrigin Increased buffer size do not solve issue.

@timbjarengren @vikeri Disabled tryGenerateStrongBoxSecurityKey solve issue ✅

Accepting @TomMahle's PR should be helpful for everyone, thanks !

henninghall commented 5 years ago

@oblador Shouldn't we keep this issue open since the "fix" is just disabling the strongbox feature, not actually solving the issue?

oblador commented 5 years ago

@henninghall You're right, I looked at that PR a bit too quickly, I'll revert. I'm however not sure how to address it. :-/

mindflow commented 5 years ago

Maybe the StrongBox feature was never intended for longer values such as RSA-512 and above, but only for passwords, pins etc. I couldn't find any specifications on this though.

olag1981 commented 5 years ago

We are having the same issues storing tokens on the Pixel 3. Any progress on this ?

mindflow commented 5 years ago

@olag1981 we ended up encrypting the value with AES128 and then storing the value in async store and just storing the decryption key in the keychain.

rodrigofeijao commented 4 years ago

For everyone having problems on Pixel3, Pixel3A and Pixel4, if you downgrade to 3.0.0 it "fixes" the problem as there's no reference to StrongBox there. This is not a solution, but will short term fix your problem.

Metroidaron commented 4 years ago

Pixel 3XL user here, and still seeing this issue... (I think my proj is up to date, can check)

Hello, Looks like last talk on this issue was a while ago now...

The merged code was reverted and after comparing the current master branch it looks like that fix or any other fixes are still not been implemented... Do we have an ETA or workaround to be merged any time soon?

If there is no solution in sight at the moment I need to look into alternative storage options and/or downgrading until there is a fix.

vonovak commented 4 years ago

@Metroidaron I'm not aware of anyone working on a proper fix; but the suggestion given in https://github.com/oblador/react-native-keychain/pull/232#issuecomment-547388572 seems good, so I'm open to merging such PR

aeirola commented 4 years ago

I was experiencing the same issue, and got myself hands on a Pixel 3a. Unfortunately I wasn't able to determine the exact root cause of the decryption errors, but I was able to fix the issue nonetheless. PR at #288.

As I am only able to test on a Pixel 3a, it would be really great if the community could help out with ensuring that the fix works on the other affected devices as well. If anyone of you have a Pixel 3, 3 XL or 4 device, we would greatly appreciate it if you could try the fix by testing your affected app with

"react-native-keychain": "git+https://github.com/aeirola/react-native-keychain.git#fix-strongbox-decryption",

in your package.json dependencies section. 🙇

Metroidaron commented 4 years ago

Just wanted to share that your branch worked for me @aeirola ; I have a pixel 3XL and I have a co-worker with a pixel 4 device, I'll be asking him to test with his phone and I'll comment back here with my findings. :)

nicolashemonic commented 4 years ago

Hello @aeirola Your fix works on React Native 0.61.5 and my Pixel 3a. Thanks!