tink-crypto / tink

Tink is a multi-language, cross-platform, open source library that provides cryptographic APIs that are secure, easy to use correctly, and hard(er) to misuse.
https://developers.google.com/tink
Apache License 2.0
13.47k stars 1.18k forks source link

ANR reports on different devices when using AndroidX Crypto, pointing to com.google.crypto.tink #638

Closed tryadelion closed 1 year ago

tryadelion commented 1 year ago

Help us help you

We are a software company using the androidx crypto library that implements Tink

Describe the bug:

We have detected a growing number of ANR (Application Not Responding) reports focalized on the Tink layer of the crypto library.

What was the expected behavior?

no ANR happening

How can we reproduce the bug?

we have a background service scheduled every one to five minutes that performs a read on the preferences, compares to a calculated value, and stores it, a few ms of execution time. the preferences are protected by encryption using the crypto library capabilities, so values read are decrypted and values stored are encrypted.

Do you have any debugging information?

ANR with 40 events with 26 users.

org.bouncycastle.crypto.digests.SHA1Digest.processBlockExecuting service OUR_SERVICE_ID

ANR with 22 events with 21 users com.google.crypto.tink.integration.android.AndroidKeystoreAesGcm.decryptInternalExecuting service OUR_SERVICE_ID

ANR with 22 events with 19 users com.google.crypto.tink.integration.android.AndroidKeystoreAesGcm.encryptInternalExecuting service OUR_SERVICE_ID

Provide your version information:

kotlin, Tink version unknown, AndroidX Crypto library version: 1.1.0-alpha03

juergw commented 1 year ago

AndroidKeystoreAesGcm is the class that calls to AndroidKeystore service to encrypt or decrypt the keys that are then used to encrypt or decrypt the shared preferences. Unfortunately, AndroidKeystore may fail in some situations, for example, if there are too many requests at the same time. Because of this, AndroidKeystoreAesGcm retries the call if it fails. See https://github.com/google/tink/commit/a056e3d1a9b3d76d05e7e222b0a6ce7b4b5c0087.

Does your background service create lots of EncryptedSharedPreferences objects at the same time? Are you using StrongBox?

tryadelion commented 1 year ago

Greetings - it's certainly possible a few dozen calls can happen in a relatively short period of time, we update a few fields based on server query results asynchronously which means they can happen at a time, but it shouldn't be more than that. is there an optimal solution? We are not aware of StrongBox.

juergw commented 1 year ago

What you could try is to minimize the creation of new EncryptedSharedPreference objects. For example, create the ones you need at startup, and keep reusing them. And do the same for the MasterKey object: only create one of them at startup. And, if these objects may be created in different threads, make sure you have a global lock around them, so that only one thread at a time is creating these objects.

One thing I will do is to improve the retry in AndroidKeystoreAesGcm: it should only retry temporary errors. If decryption failed because of a bad ciphertext, it should not retry.

juergw commented 1 year ago

I have now fixed the retry in AndroidKeystoreAesGcm, see https://github.com/google/tink/commit/0712922251822c71272cfa2f88ed9f342ddde000.

tryadelion commented 1 year ago

Greetings, thank you for the fix. I know it probably isn't your domain, @juergw , but Is there a way to know when it could reach the AndroidX Crypto library?

In the meanwhile, we've been trying to find a way to reduce to a minimum the creation of new EncryptedSharedPreference without compromising our system, is there any recommended approach you can think of, and maybe a way to ensure the work is done in a non-blocking thread?

juergw commented 1 year ago

We plan to soon release a new version of Tink, and then (I think) also soon after a new AndroidX Crypto release can be made, which uses the new Tink release. But I can't give you exact dates.

I can't really give you any additional tips, sorry.

juergw commented 1 year ago

Sorry, I forgot to update this issue.

Tink v 1.8.0 has been released, which includes the fix I mentioned above: https://github.com/tink-crypto/tink-java/releases/tag/v1.8.0

There has also been a new alpha release of androidx security: https://developer.android.com/jetpack/androidx/releases/security#1.1.0-alpha05

But note that that release still uses Tink v.1.7.0 as dependency, https://mvnrepository.com/artifact/androidx.security/security-crypto/1.1.0-alpha05.

So you need to make sure that you use the newest version of Tink.

Because I think the issues here have been addressed, I'm going to close this issue now.

tryadelion commented 1 year ago

greetings @juergw , thanks for letting us know. We can't seem to find those changes within 1.8.0, was it maybe squashed?

juergw commented 1 year ago

The 1.8 version of Tink is published to maven from the new repository, the source code is now here:

https://github.com/tink-crypto/tink-java/blob/1.8/src/main/java/com/google/crypto/tink/integration/android/AndroidKeystoreAesGcm.java

and it does include the changes. I also checked the source .zip file attached to https://github.com/tink-crypto/tink-java/releases/tag/v1.8.0, and it was also there. So I don't think its missing. Can you point me to where you don't see the change?

juergw commented 1 year ago

Note that there is now a new version 1.1.0-alpha06 of androidx security which includes Tink Java version 1.8.0.

Does this solve the issue?

tryadelion commented 1 year ago

We will update and check for any issues, thank you for letting us know, we'll update as soon as we can.

juergw commented 1 year ago

Any update on this? Is this still an issue?

tryadelion commented 1 year ago

Hi, we haven't experienced this issue since we upgraded, so all indication is that it is solved. If we encounter something similar again we'll open a ticket. Thank you very much for your support.