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

Proguard/R8 is stripping out fields from generated file class AesGcmKeyFormat #361

Closed simtse closed 4 years ago

simtse commented 4 years ago

Coming from Tink 1.3.0 stable to Tink 1.4 RC2 (on Android), I noticed that the AesGcmKeyFormat has a possible problem with proguard/R8. We haven't added anything in our proguard-rules.pro file for Tink and am getting a stack trace like this

java.lang.NoClassDefFoundError: com.google.crypto.tink.aead.AeadKeyTemplates
    ... 13 more
Caused by: java.lang.NoClassDefFoundError: com.google.crypto.tink.aead.AeadKeyTemplates
    at slack.commons.security.AeadPrimitiveFactoryImpl.getAeadPrimitive(AeadPrimitiveFactory.kt:4)
    ... 12 more
Caused by: java.lang.ExceptionInInitializerError
    at androidx.security.crypto.EncryptedSharedPreferences$PrefValueEncryptionScheme.
<clinit>(EncryptedSharedPreferences.java:1)
        ... 11 more
Caused by: java.lang.RuntimeException: Field version_ for com.google.crypto.tink.proto.AesGcmKeyFormat not found. Known fields are [public int com.google.crypto.tink.proto.AesGcmKeyFormat.keySize_, public static final com.google.crypto.tink.proto.AesGcmKeyFormat com.google.crypto.tink.proto.AesGcmKeyFormat.DEFAULT_INSTANCE, public static volatile com.google.crypto.tink.shaded.protobuf.Parser com.google.crypto.tink.proto.AesGcmKeyFormat.PARSER]
    at com.google.crypto.tink.shaded.protobuf.MessageSchema.reflectField(MessageSchema.java:7)
    at com.google.crypto.tink.shaded.protobuf.MessageSchema.newSchemaForRawMessageInfo(MessageSchema.java:53)
    at com.google.crypto.tink.shaded.protobuf.MessageSchema.newSchema(MessageSchema.java:2)
    at com.google.crypto.tink.shaded.protobuf.Protobuf.schemaFor(Protobuf.java:30)
    at com.google.crypto.tink.shaded.protobuf.Protobuf.schemaFor(Protobuf.java:48)
    at com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite$Builder.buildPartial(GeneratedMessageLite.java:5)
    at com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite$Builder.build(GeneratedMessageLite.java:1)
    at com.google.crypto.tink.aead.AeadKeyTemplates.createAesGcmKeyTemplate(AeadKeyTemplates.java:5)
    at com.google.crypto.tink.aead.AeadKeyTemplates.<clinit>(AeadKeyTemplates.java:1)

In the Android studio looking at the generated AesGcmKeyFormat, it has the _version

package com.google.crypto.tink.proto;

import ...;

public final class AesGcmKeyFormat extends GeneratedMessageLite<AesGcmKeyFormat, AesGcmKeyFormat.Builder> implements AesGcmKeyFormatOrBuilder {
    public static final int KEY_SIZE_FIELD_NUMBER = 2;
    private int keySize_;
    public static final int VERSION_FIELD_NUMBER = 3;
    private int version_;
    private static final AesGcmKeyFormat DEFAULT_INSTANCE;
    private static volatile Parser<AesGcmKeyFormat> PARSER;

...
}

But when looking at the build APK with proguard/R8 running through it, seems like version_ was removed

Screen Shot 2020-05-19 at 5 33 14 PM

Seems like the AesSivKeyFormat has a similar problem as well.

Screen Shot 2020-05-19 at 5 47 31 PM

I'm going to also check if this problem might have existed in the 1.3 release of the library. But I haven't seen this stack trace before.

We stricly use protobuf 3.12.0

+--- com.google.protobuf:protobuf-java:{strictly 3.12.0} -> 3.12.0 (c)

...

|    |    \--- com.google.protobuf:protobuf-javalite:3.12.0

...

|    |    +--- com.google.android.apps.common.testing.accessibility.framework:accessibility-test-framework:2.1
|    |    |    +--- org.hamcrest:hamcrest-core:1.3 -> org.hamcrest:hamcrest:2.2
|    |    |    \--- com.google.protobuf:protobuf-java:2.6.1 -> 3.12.0
simtse commented 4 years ago

Checked with the 1.3.0 stable release that the AesGcmKeyFormat has the version_ and more members. It seems like proguard plays nicer with 1.3.

Screen Shot 2020-05-19 at 6 11 48 PM

Same with the AesSivKeyFormat

Screen Shot 2020-05-19 at 6 11 54 PM
simtse commented 4 years ago

Ahh in 1.3, we already had

-keep class * extends com.google.protobuf.GeneratedMessageLite { *; }

But since this is now in the shaded package, we added this to make it work

-keep class * extends com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite { *; }

We definitely have other libraries that use protobuf in our project, I'm not sure how they conflict or cause problems.

tholenst commented 4 years ago

Apparently this is https://github.com/protocolbuffers/protobuf/issues/6463. I will investigate more.

tholenst commented 4 years ago

(By the way, much thanks for the detailed report: this helps a lot).

thaidn commented 4 years ago

@simtse

Does this smaller rule work for you?

-keepclassmembers class * extends com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite {
  <fields>;
}
thaidn commented 4 years ago

It looks like the right way to do this is to include this rule in Tink. According to https://developer.android.com/studio/build/shrink-code#configuration-files, we just need to put this rule in META-INF/proguard/.

thaidn commented 4 years ago

@simtse HEAD-SNAPSHOT (and 1.4.0) includes a ProGuard file rule with the following rule:

-keepclassmembers class * extends com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite {
  <fields>;
}

I believe your custom rule is no longer needed. Could you please test with HEAD-SNAPSHOT and confirm?

thaidn commented 4 years ago

I confirm that the fix works perfectly.

simtse commented 4 years ago

I'm running into some issues on my part so I'm doing some testing right now. I don't have anything conclusive yet, but this is an error that I get when taking HEAD-SNAPSHOT and removing my proguard rules.

java.lang.IncompatibleClassChangeError: com.google.crypto.tink.proto.AesCtrHmacAeadKey
at dalvik.system.DexFile.defineClassNative(Native Method)
at dalvik.system.DexFile.defineClass(DexFile.java:226)
at dalvik.system.DexFile.loadClassBinaryName(DexFile.java:219)
at dalvik.system.DexPathList.findClass(DexPathList.java:338)
at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:54)
at java.lang.ClassLoader.loadClass(ClassLoader.java:511)
at java.lang.ClassLoader.loadClass(ClassLoader.java:469)
at com.google.crypto.tink.aead.AesCtrHmacAeadKeyManager.<init>(AesCtrHmacAeadKeyManager.java:43)
at com.google.crypto.tink.aead.AeadConfig.<clinit>(AeadConfig.java:37)

And AndroidX EncryptedSharedPreferences has similar problem

java.lang.IncompatibleClassChangeError: com.google.crypto.tink.proto.AesSivKeyFormat
at dalvik.system.DexFile.defineClassNative(Native Method)
at dalvik.system.DexFile.defineClass(DexFile.java:226)
at dalvik.system.DexFile.loadClassBinaryName(DexFile.java:219)
at dalvik.system.DexPathList.findClass(DexPathList.java:338)
at dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:54)
at java.lang.ClassLoader.loadClass(ClassLoader.java:511)
at java.lang.ClassLoader.loadClass(ClassLoader.java:469)
at com.google.crypto.tink.daead.DeterministicAeadKeyTemplates.createAesSivKeyTemplate(DeterministicAeadKeyTemplates.java:45)
at com.google.crypto.tink.daead.DeterministicAeadKeyTemplates.<clinit>(DeterministicAeadKeyTemplates.java:38)
at androidx.security.crypto.EncryptedSharedPreferences$PrefKeyEncryptionScheme.<clinit>(EncryptedSharedPreferences.java:146)

I'm still trying to figure out what might be the problem. Might be the emulator environment as well.

simtse commented 4 years ago

@thaidn, So this is going to sound odd as these are some combinations I've tried and their results.

1) HEAD-SNAPSHOT + removed local keep members proguard rule - ❌ 2) HEAD-SNAPSHOT + with local keep class proguard rule - ❌ 3) HEAD-SNAPSHOT + with local keep members proguard rule - ❌ 4) 1.4.0-rc2 + removed local keep members/class proguard rules - ❌ (this is expected as classes are stripped) 5) 1.4.0-rc2 + using local keep class proguard rules - ✅ 6) 1.4.0-rc2 + using local keep memebers proguard rules - ✅

thaidn commented 4 years ago

Can you show me your build.gradle?

I suspect that this is because of caching. Can you remove $HOME/.gradle/.caches and try again?

simtse commented 4 years ago

Our build.gradle is kotlin script, so not the same for pasting Not sure what you're looking for specifically, so I'll just translate to what we have. The build is happening in a CI environment with a Google Cloud NexusLowRes device from Firebase test lab. I had seen this problem with IncompatibleClassChangeError before with Tink 1.3.0, but this stopped happening when I brought in Tink 1.4.0 RC2.

dependences {
  ...
  implementation 'com.google.crypto.tink:tink-android:HEAD-SNAPSHOT'
  ...
}

And we've declared the https://oss.sonatype.org/content/repositories/snapshots as a repository for the build.

image

Note: I updated my comment above https://github.com/google/tink/issues/361#issuecomment-633727033 to say that ✅ worked when we declare the keep rules ourselves.

thaidn commented 4 years ago

Thanks @simtse. Have you tried removing $HOME/.gradle/.caches?

simtse commented 4 years ago

Will try locally with removing cache. Because this is running on Firebase test lab devices/emulators, I would have thought the caching doesn't really matter. But I'll try

simtse commented 4 years ago

Whoops sorry I hit the wrong button.

tytydraco commented 4 years ago

Thanks @simtse. Have you tried removing $HOME/.gradle/.caches?

I am having the issue stated above, with EncryptedSharedPrefs. I just invalidated my Android Studio and Gradle caches, cleaned my project, and restarted Android Studio. The issue persists when building a release APK with minify enabled. Here is my log if this helps:

Caused by: java.lang.RuntimeException: Field version_ for com.google.crypto.tink.proto.AesGcmKeyFormat not found. Known fields are [public int com.google.crypto.tink.proto.AesGcmKeyFormat.keySize_, public static final com.google.crypto.tink.proto.AesGcmKeyFormat com.google.crypto.tink.proto.AesGcmKeyFormat.DEFAULT_INSTANCE, public static volatile com.google.crypto.tink.shaded.protobuf.Parser com.google.crypto.tink.proto.AesGcmKeyFormat.PARSER]

thaidn commented 4 years ago

@tytydraco hi there, EncryptedSharedPrefs would require a different solution. It comes from AndroidX Security which depends on 1.4.0-rc2. You have two workarounds:

1/ Use this ProGuard rule

-keepclassmembers class * extends com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite {
  <fields>;
}

2/ Configure Gradle to remove the 1.4.0-rc2 version and add a dependency on HEAD-SNAPSHOT.

Could you please try either way and let us know which one works for you?

tytydraco commented 4 years ago

Hey @thaidn, thanks for the help. Solution #1 solved the problem for me. I assume that in the future, I'll be able to remove the proguard rule. I appreciate your response.

thaidn commented 4 years ago

Hey @thaidn, thanks for the help. Solution #1 solved the problem for me. I assume that in the future, I'll be able to remove the proguard rule. I appreciate your response.

Thanks for confirming. Yeah the goal is to have the rule within Tink.

I added it to Tink in https://github.com/google/tink/commit/8bdaed4f41edc01821be9b089123410be0b57a6c. It works on my machine, but it seems that @simtse is having some trouble.

simtse commented 4 years ago

I'm working on this today again so I'll report back. I will try to build locally instead of in CI to see if it might be a CI caching problem.

simtse commented 4 years ago

Another weird thing happened when running locally with HEAD-SNAPSHOT locally with proguard. This doesn't happen if I depend on Tink 1.4.0-rc2. When using the newer API in building the AndroidKeysetManager

val keysetBuilder = AndroidKeysetManager.Builder()
          .withKeyTemplate(AesGcmKeyManager.aes256GcmTemplate())
          .withSharedPref(context, "keyset_name", "shared_prefs_file_name")
          .withMasterKeyUri("tink-key-alias")

Then got

Caused by: java.lang.NoSuchMethodError: No static method aes256GcmTemplate()Lcom/google/crypto/tink/KeyTemplate; in class Lcom/google/crypto/tink/aead/AesGcmKeyManager; or its super classes (declaration of 'com.google.crypto.tink.aead.AesGcmKeyManager' appears in base.apk)

However, if I were to use the now deprecated approach

.withKeyTemplate(AeadKeyTemplates.AES256_GCM)

I would be able to reference the field. The odd thing was that I also added the rule as well

-keep class * extends com.google.crypto.tink.shaded.protobuf.GeneratedMessageLite { *; }

but it still failed with the same error. Locally, I haven't run into the incompatible class error. So it might be some problem in our caching and CI then.

Screen Shot 2020-05-26 at 2 38 29 PM

It is odd that it consistently fails even after I cleared my caches from {HOME}/.gradle/.caches and also gradle commands to clean and build.

tholenst commented 4 years ago

Caused by: java.lang.NoSuchMethodError: No static method aes256GcmTemplate()Lcom/google/crypto/tink/KeyTemplate; in class Lcom/google/crypto/tink/aead/AesGcmKeyManager; or its super classes (declaration of 'com.google.crypto.tink.aead.AesGcmKeyManager' appears in base.apk)

I'm not sure that this is related to proguard though, it sounds like somehow an old version of AesGcmKeyManager made it in your compilation. How did you build it in this case?

simtse commented 4 years ago

@tholenst and @thaidn , yes so I figured out why I had those problems. I had the AndroidX security library also, and it uses Tink 1.3.0 (or 1.4.0-rc4 for newer versions) and when pointing my exlicity dependency to HEAD-SNAPSHOT, it took the 1.3.0 version instead (or the one with the number) instead of HEAD-SNAPSHOT. This caused the builds to take an older API and appear to strip out code that wasn't in 1.3.0.

I updated the local builds and everything looks good now ✅ 1) No incompatible class problems 2) No proguard stripping issues 3) Taking HEAD-SNAPSHOT over transitive 1.3.0 or 1.4.0-rc2 also consumes the new snapshot proguard rule https://github.com/google/tink/commit/8bdaed4f41edc01821be9b089123410be0b57a6c.

HEAD-SNAPSHOT looks good to fix the proguard issue with generated files from protobuf. 👍

thaidn commented 4 years ago

@simtse awesome, thank you!

Would you mind sharing your Gradle config, especially how you replace 1.3.0 in AndroidX Security with 1.4.0-rc2? I believe many would found it useful.

simtse commented 4 years ago

So my problem wasn't exactly getting AndroidX security to 1.4.0-rc2, I believe it was more that the HEAD-SNAPSHOT takes a lower precedence in dependency resolution if ordered by alphabetically?

I had something like this in Gradle config (kotlin gradle script). But the groovy implementation is very similar.

dependences {
  ...
  implementation("com.google.crypto.tink:tink-android:HEAD-SNAPSHOT")
  implementation("androidx.security:security-crypto:1.0.0-rc01") // uses Tink 1.3.0
  ...
}

And for whatever reason, it would pick up 1.3.0 Tink when building to the APK. So I had to override resolution strategy to get HEAD-SNAPSHOT

configurations.all {
  resolutionStrategy.eachDependency {
    if (requested.group == "com.google.crypto.tink" && requested.name == "tink-android" && (requested.version != "HEAD-SNAPSHOT")) {
      useVersion("HEAD-SNAPSHOT")
      because("Testing out snapshots")
    }
  }
}
thaidn commented 4 years ago

This is very helpful, thanks @simtse.

I'll leave this open until 1.4.0 is released.

thaidn commented 4 years ago

https://github.com/google/tink/releases/tag/v1.4.0 was released a few minutes ago.

michael-behnam-carriage commented 1 year ago

Hi @thaidn i tried the above solution but it didn't work for me also i tried different versions for tink library and i have the same problem

implementation 'androidx.security:security-crypto:1.1.0-alpha03'
implementation 'com.google.crypto.tink:tink-android:1.5.0'

i can find tink-android:1.5.0 and tink:1.3.0-rc2 in the gradle cache

@tholenst i wonder if you can help aslo