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

On Android, when GeneralSecurityException "decryption failed" is thrown when calling AeadFactory::decrypt? #191

Closed AllanHasegawa closed 5 years ago

AllanHasegawa commented 5 years ago

Hi,

Edit: we are using Tink 1.2.2.

We have been getting crash reports with the exception GeneralSecurityException: decryption failed with the stack trace pointing to AeadFactory::decrypt as the source of the exception, and we are lost on when that happens.

We are not sharing the keys or the cipher text, so, the same device and algorithm that encrypts it, is the same one that is decrypting it.

We are also using the AndroidKeysetManager.

The error seems to happen exclusively on Android 8 e 9 so far.

Any idea on what may be causing this exception being thrown? Thank you :)

Check our full implementation: https://gist.github.com/AllanHasegawa/85431b6f7c53074511527655712c9872

thaidn commented 5 years ago

We have been getting crash reports with the exception GeneralSecurityException: decryption failed with the stack trace pointing to AeadFactory::decrypt as the source of the exception, and we are lost on when that happens.

It happens when the ciphertext is invalid or the key is incorrect.

https://gist.github.com/AllanHasegawa/85431b6f7c53074511527655712c9872 Interesting.

Are you using the same keysetFileName? Can you dump the Shared Preference to make sure it is there and is not changed on each run?

Also: https://gist.github.com/AllanHasegawa/85431b6f7c53074511527655712c9872#file-tink_keystore-kt-L38 looks incorrect? You simply concatenate the associate value with ciphertext, so values.size should be 2?

AllanHasegawa commented 5 years ago

It happens when the ciphertext is invalid or the key is incorrect.

hmmm. I'm saving the cipher text on SharedPreferences. Maybe Android is corrupting this data somehow? will investigate further.

Are you using the same keysetFileName?

Yes, same keysetFilename.

Can you dump the Shared Preference to make sure it is there and is not changed on each run?

Unfortunately I was not able to replicate this issue to know more about it. The crashes are coming from Crashlytics. I tried this code in many different devices and it always works.

On the devices that the code do work the contents of the SharedPreferences seems to be OK. I tried even manually changing the contents of the shared preference and even so I could not replicate the crash. If I change the key cache, I just lose the content, but no crash.

I'm running a test where it encrypt/decrypts billions of random strings with no issues. The problem should be an issue somewhere else.

values.size should be 2?

The regex matcher will return 3 groups. The first one is the full match, with the others 2 being the associated data with the cipher text. This works fine 99.97% of the time (0.03% being the crashes).

thaidn commented 5 years ago

It looks like we've got a Heisenbug.

The error seems to happen exclusively on Android 8 e 9 so far.

Do you have a list of device models and/or manufacturers on which it happened?

AllanHasegawa commented 5 years ago

Do you have a list of device models and/or manufacturers on which it happened?

Yes.

tholenst commented 5 years ago

If I read the code correctly, this would happen if clock.now().toString() returns a string containing a colon. It sounds unlikely this is the problem (why would it only happen so infrequently?), but still: is this possible?

AllanHasegawa commented 5 years ago

Here Clock::now() == System.currentTimeMillis(). It's in an interface just to make testing easier.

But, if an extra colon were in the String, another type of error would be thrown before.

tholenst commented 5 years ago

I see.

Is it possible for you to do a "safety decryption" right after you encrypted the data? I.e., after val cipherBytes = aead.encrypt( plainText.toBA(), associatedData.toBA()) add something like val unused = aead.decrypt(cipherBytes, associatedData.toBA());

This would make the correct team is working on debugging this issue.

AllanHasegawa commented 5 years ago

@tholenst Will do. Will create an exception to know if we are crashing right after generating the cipher text, or when restoring it from SharedPreferences.

AllanHasegawa commented 5 years ago

We rolled out an updated version of this "safety decryption" as @tholenst suggested and we got no exceptions thrown when deciphering just after encrypting our data. This makes me think that Tink is working as intended here.

We are, however, still getting crashes when deciphering persisted data. But for now, I'll close this issue since I don't think there's much Tink can do to fix in this case.

daewin commented 5 years ago

I had a similar issue on a project that i'm working on. After stepping through the debugger to see what methods and classes get called, I noticed that from the Android integration package (https://github.com/google/tink/tree/master/java/src/main/java/com/google/crypto/tink/integration/android), AndroidKeystoreAesGcm does not get invoked but AesGcmJce does instead.

What I attempted to do was to use the static getOrGenerateNewAeadKey(String keyUri) in AndroidKeystoreKmsClient after generating the KeysetHandler like so (roughly based on your gist):

private fun getAEAD(): Aead { generateKeysetHandler() return getOrGenerateNewAeadKey(masterUri.toString()) }

This now invokes AndroidKeystoreAesGcm, and from my initial testing the error no longer occurs.

I know that in the latest snapshot, there is a new way of doing this based off the Android example (https://github.com/google/tink/blob/master/examples/helloworld/android/app/src/main/java/com/helloworld/TinkApplication.java): aead = getOrGenerateNewKeysetHandle().getPrimitive(Aead.class), but it still invokes AesGcmJce.

This isn't a solution of course because we can only use this Aead on Android M (API level 23) or newer. My question to the team is: How should we properly obtain the Aead interface for Android end-to-end?

daewin commented 5 years ago

My apologies, I stand corrected. I've managed to replicate the scenario and in our case it occurs because when the user logs out, we delete the KeyStore alias entry. As we lazy load the getOrGenerateNewKeysetHandle() function, unless the app is closed fully at that point, the next time a user logs in, it will be using the keyset generated from the now defunct key alias.

Though my point that AndroidKeystoreAesGcm does not get invoked still holds true. Let me know if my usage above is correct.

thaidn commented 5 years ago

Hi @daewin, so what happened was the key got deleted, but getOrGenerateNewAeadKey doesn't generate a new one for you?

daewin commented 5 years ago

Hey @thaidn. I believe that key generation etc. works as intended, but the problem was that we were lazy loading the Aead instance (as described in code below), so even after deleting the KeyStore alias, it was still using the now defunct Aead instance on the next user login - the proper way is to let the AndroidKeysetManager builder do that work for us each time: in particular, this line in the AndroidKeysetManager: masterKey = AndroidKeystoreKmsClient.getOrGenerateNewAeadKey(builder.masterKeyUri)

For some context, it looks a little like this: Aead instance:

    val aead: Aead by lazy {
        AndroidKeysetManager.Builder()
            .withSharedPref(applicationContext, TINK_KEYSET_NAME, PREF_FILE_NAME)
            .withKeyTemplate(AeadKeyTemplates.AES256_GCM)
            .withMasterKeyUri(MASTER_KEY_URI)
            .build()
            .keysetHandle
            .getPrimitive(Aead::class.java)
    }

On logout, this code is called:

        val keyStore = KeyStore.getInstance("AndroidKeyStore")
        keyStore.load(null /* param */)
        if (keyStore.containsAlias(MASTER_KEY_URI_SUFFIX)) {
            keyStore.deleteEntry(MASTER_KEY_URI_SUFFIX)
        }

It was my mistake in the implementation. Though i'm curious as to why AndroidKeystoreAesGcm never gets used even if i'm on Android APIs >=23. We seem to be using AesGcmJce exclusively.

thaidn commented 5 years ago

Though i'm curious as to why AndroidKeystoreAesGcm never gets used even if i'm on Android APIs >=23. We seem to be using AesGcmJce exclusively.

Sorry for the late response. AndroidKeystoreAesGcm is only used to unwrap the keyset. AesGcmJce handles the actual data encryption.

Please reopen if you need more help.

simtse commented 4 years ago

Though i'm curious as to why AndroidKeystoreAesGcm never gets used even if i'm on Android APIs >=23. We seem to be using AesGcmJce exclusively.

Sorry for the late response. AndroidKeystoreAesGcm is only used to unwrap the keyset. AesGcmJce handles the actual data encryption.

Please reopen if you need more help.

So this makes some more sense the role of AndroidKeystoreAesGcm. So my understanding is that tink does the following 1) Tink creates a master key with the alias passed in withMasterKeyUri(). The key is stored in the AndroidKeyStore. 2) Tink gets an Aead from AndroidKeystoreKmsClient#getOrGenerateNewAeadKey() 3) Tink writes the KeySet into the SharedPreference with the master key (stored in the KeyStore). This encryption will use AndroidKeystoreAesGcm to encrypt, as it is passed in as the masterKey in KeysetHandle#encrypt(Keyset keyset, Aead masterKey)

GuilhE commented 3 years ago

Any update on this? Still happens in 1.0.0-rc03 (tested in Mi A2 Lite)