Skip to content
This repository has been archived by the owner on Apr 17, 2024. It is now read-only.

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

Closed
simtse opened this issue May 20, 2020 · 28 comments
Closed
Assignees

Comments

@simtse
Copy link
Contributor

simtse commented May 20, 2020

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
Copy link
Contributor Author

simtse commented May 20, 2020

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 simtse changed the title Android Proguard/R8 is stripping out fields from generated file class AesGcmKeyFormat Proguard/R8 is stripping out fields from generated file class AesGcmKeyFormat May 20, 2020
@simtse
Copy link
Contributor Author

simtse commented May 20, 2020

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 tholenst self-assigned this May 20, 2020
@tholenst
Copy link
Contributor

Apparently this is protocolbuffers/protobuf#6463. I will investigate more.

@tholenst
Copy link
Contributor

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

thaidn added a commit to thaidn/androidx-security-issue that referenced this issue May 22, 2020
AndroidX Security delegates crypto operation to Tink which
depends on Protobuf Javalite. Recently Protobuf Javalite
introduced a change that relies on reflection, which doesn't
work with Proguard.

This change adds a rule that keeps the (shaded) Protobuf
classes in Tink as-is.

See also:
 - https://buganizer.corp.google.com/issues/154315507
 - tink-crypto/tink#361
 - protocolbuffers/protobuf#6463
 - https://b.corp.google.com/issues/144631039
@thaidn
Copy link
Contributor

thaidn commented May 22, 2020

@simtse

Does this smaller rule work for you?

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

@thaidn
Copy link
Contributor

thaidn commented May 22, 2020

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 added a commit that referenced this issue May 25, 2020
@thaidn
Copy link
Contributor

thaidn commented May 25, 2020

@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
Copy link
Contributor

thaidn commented May 25, 2020

I confirm that the fix works perfectly.

@simtse
Copy link
Contributor Author

simtse commented May 25, 2020

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
Copy link
Contributor Author

simtse commented May 25, 2020

@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
Copy link
Contributor

thaidn commented May 25, 2020

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
Copy link
Contributor Author

simtse commented May 25, 2020

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 #361 (comment) to say that ✅ worked when we declare the keep rules ourselves.

@thaidn
Copy link
Contributor

thaidn commented May 25, 2020

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

@simtse
Copy link
Contributor Author

simtse commented May 25, 2020

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 simtse closed this as completed May 25, 2020
@simtse simtse reopened this May 25, 2020
@simtse
Copy link
Contributor Author

simtse commented May 26, 2020

Whoops sorry I hit the wrong button.

@tytydraco
Copy link

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
Copy link
Contributor

thaidn commented May 26, 2020

@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
Copy link

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
Copy link
Contributor

thaidn commented May 26, 2020

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 8bdaed4. It works on my machine, but it seems that @simtse is having some trouble.

@simtse
Copy link
Contributor Author

simtse commented May 26, 2020

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
Copy link
Contributor Author

simtse commented May 26, 2020

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
Copy link
Contributor

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
Copy link
Contributor Author

simtse commented May 27, 2020

@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 8bdaed4.

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

@thaidn
Copy link
Contributor

thaidn commented May 27, 2020

@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
Copy link
Contributor Author

simtse commented May 27, 2020

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
Copy link
Contributor

thaidn commented May 28, 2020

This is very helpful, thanks @simtse.

I'll leave this open until 1.4.0 is released.

@thaidn
Copy link
Contributor

thaidn commented Jul 14, 2020

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

@thaidn thaidn closed this as completed Jul 14, 2020
@michael-behnam-carriage
Copy link

michael-behnam-carriage commented May 2, 2023

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants