Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support devices runing KitKat (API 19) #6 #10

Closed
wants to merge 1 commit into from

Conversation

davidmigloz
Copy link
Contributor

@davidmigloz davidmigloz commented Jul 9, 2018

@davidmigloz davidmigloz changed the title Support devices runing KitKat (API 19) #6 Support devices runing KitKat (API 19) Jul 9, 2018
@davidmigloz davidmigloz force-pushed the kit-kat branch 2 times, most recently from 70fca34 to 2e094dd Compare July 9, 2018 19:06
@davidmigloz davidmigloz changed the title Support devices runing KitKat (API 19) Support devices runing KitKat (API 19) #6 Jul 9, 2018
@patrickfav
Copy link
Owner

Hi David, thanks for the PR.

I have reservations about the change of using no AAD, because if a device updates to 21+ it will not verify, requiring the lib to persist SDK version and handle migration. This adds quite a bit of complexity and probably opens an downgrade attack vector. Do you have any idea how to tackle that issue?

Also, how serious is the issue of missing GCMParamSpec? Is this just on a handful devices? It seems weired that a class is missing from the Android SDK thats supposed to be there?

@davidmigloz
Copy link
Contributor Author

Hi Patrick,

The traces of the two issues (tested in a Samsung Galaxy SII with Android 4.4.4):

Issue with the GCMParamSpec :

07-10 08:38:49.386 4299-4299/at.favre.lib.securesharedpreferences E/AndroidRuntime: FATAL EXCEPTION: main
    Process: at.favre.lib.securesharedpreferences, PID: 4299
    java.lang.IllegalStateException: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:337)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31)
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248)
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58)
        at android.view.View.performClick(View.java:4445)
        at android.view.View$PerformClick.run(View.java:18446)
        at android.os.Handler.handleCallback(Handler.java:733)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at android.os.Looper.loop(Looper.java:136)
        at android.app.ActivityThread.main(ActivityThread.java:5158)
        at java.lang.reflect.Method.invokeNative(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:515)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566)
        at dalvik.system.NativeStart.main(Native Method)
     Caused by: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:81)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:82)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:70)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335) 
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: java.security.InvalidAlgorithmParameterException: unknown parameter type.
        at com.android.org.bouncycastle.jcajce.provider.symmetric.util.BaseBlockCipher.engineInit(BaseBlockCipher.java:559)
        at javax.crypto.Cipher.init(Cipher.java:616)
        at javax.crypto.Cipher.init(Cipher.java:566)
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:68)
07-10 08:38:49.386 4299-4299/at.favre.lib.securesharedpreferences E/AndroidRuntime: FATAL EXCEPTION: main
    Process: at.favre.lib.securesharedpreferences, PID: 4299
    java.lang.IllegalStateException: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:337)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31)
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248)
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58)
        at android.view.View.performClick(View.java:4445)
        at android.view.View$PerformClick.run(View.java:18446)
        at android.os.Handler.handleCallback(Handler.java:733)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at android.os.Looper.loop(Looper.java:136)
        at android.app.ActivityThread.main(ActivityThread.java:5158)
        at java.lang.reflect.Method.invokeNative(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:515)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566)
        at dalvik.system.NativeStart.main(Native Method)
     Caused by: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:81)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:82)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:70)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335) 
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: java.security.InvalidAlgorithmParameterException: unknown parameter type.
        at com.android.org.bouncycastle.jcajce.provider.symmetric.util.BaseBlockCipher.engineInit(BaseBlockCipher.java:559)
        at javax.crypto.Cipher.init(Cipher.java:616)
        at javax.crypto.Cipher.init(Cipher.java:566)
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:68)

Issue with the AAD:

07-10 08:44:56.974 4764-4764/at.favre.lib.securesharedpreferences E/AndroidRuntime: FATAL EXCEPTION: main
    Process: at.favre.lib.securesharedpreferences, PID: 4764
    java.lang.IllegalStateException: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:337)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31)
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248)
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58)
        at android.view.View.performClick(View.java:4445)
        at android.view.View$PerformClick.run(View.java:18446)
        at android.os.Handler.handleCallback(Handler.java:733)
        at android.os.Handler.dispatchMessage(Handler.java:95)
        at android.os.Looper.loop(Looper.java:136)
        at android.app.ActivityThread.main(ActivityThread.java:5158)
        at java.lang.reflect.Method.invokeNative(Native Method)
        at java.lang.reflect.Method.invoke(Method.java:515)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566)
        at dalvik.system.NativeStart.main(Native Method)
     Caused by: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:81)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335)
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not encrypt
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:88)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.encrypt(DefaultEncryptionProtocol.java:70)
        at at.favre.lib.armadillo.SecureSharedPreferences.encryptToBase64(SecureSharedPreferences.java:335) 
        at at.favre.lib.armadillo.SecureSharedPreferences.access$300(SecureSharedPreferences.java:31) 
        at at.favre.lib.armadillo.SecureSharedPreferences$Editor.putString(SecureSharedPreferences.java:248) 
        at at.favre.lib.securesharedpreferences.MainActivity$2.onClick(MainActivity.java:58) 
        at android.view.View.performClick(View.java:4445) 
        at android.view.View$PerformClick.run(View.java:18446) 
        at android.os.Handler.handleCallback(Handler.java:733) 
        at android.os.Handler.dispatchMessage(Handler.java:95) 
        at android.os.Looper.loop(Looper.java:136) 
        at android.app.ActivityThread.main(ActivityThread.java:5158) 
        at java.lang.reflect.Method.invokeNative(Native Method) 
        at java.lang.reflect.Method.invoke(Method.java:515) 
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566) 
        at dalvik.system.NativeStart.main(Native Method) 
     Caused by: java.lang.UnsupportedOperationException: This cipher does not support Authenticated Encryption with Additional Data
        at javax.crypto.CipherSpi.engineUpdateAAD(CipherSpi.java:393)
        at javax.crypto.Cipher.updateAAD(Cipher.java:1056)
        at at.favre.lib.armadillo.AesGcmEncryption.encrypt(AesGcmEncryption.java:77)

@davidmigloz
Copy link
Contributor Author

I didn't think about what happens when the user updates to 21. But.. do you think it's worth to hadle it? I don't expect a lot of users updating from 19 to 21 in 2018, if they are using 19 it's probably because the manufacter droped support in that version. What do you think?

About GCMParamSpec, I have no idea how many devices don't have it implemented. The limitation of using IvParameterSpec is that we have to stick to the default tag lenght of 128 bits.

@patrickfav
Copy link
Owner

There are still working SII around 😆? Anyways, its seems that the last official update for that device was 4.1.2 (according to Wikipedia) wo we are dealing with custom rom issues. Not too keen on supporting those :/

So I suggest instead of patching the default encryption logic, why not just add a new implementation of AuthenticatedEncryption without AAD called e.g. KitKatSupportAesGcmEncryption. The Armadillo builder allows a lot of DI so the user could just use this implementation if he/she likes, leaving everybody else without needing to support those exotic devices with the standard impl. One would need to add a paragraph in the Readme and explain (e.g. "Known Issues") when to use this.

What do you think?

@davidmigloz
Copy link
Contributor Author

davidmigloz commented Jul 10, 2018

Yes it has a cyanogenmod rom.. I don't have any device with pure Android 4.4. I also tested it in an emulator and I'm getting the same two exceptions. Do you have any KitKat device where it's working?

So you think is better to make the default AesGcmEncryption compatible just from API 21 and if you want to support from 19 inject KitKatSupportAesGcmEncryption?

@HonzaR
Copy link

HonzaR commented Jul 10, 2018

Hi guys, I have few KitKat devices. And the crash happens on all of them. Today standard for new project says we should still support Kitkat devices, because there is still more than 10% active devices market share. See

https://developer.android.com/about/dashboards/

@patrickfav
Copy link
Owner

I just tried it with the emulator and you are right, the BC version of this build does not seem to support the IV type - my statement was based upon the assumption that this is specific issue to some roms. It is very strange that Google would expose the new APIs but would not support it? We should check if there maybe is another provider in KitKat which supports AAD and GcmIvParamSpec?

But I agree, that we should add a fallback variant in the default impl for kitkat, but maybe make it more clear that it is used (So a dev is clear that this will break data on SDK update in KitKat)

I leave this link as a reference:
https://stackoverflow.com/questions/42928339/gcmparameterspec-throws-invalidalgorithmparameterexception-unknown-parameter-ty

@davidmigloz
Copy link
Contributor Author

Here there is a Google implementation:
https://github.com/google/tink/blob/master/java/src/main/java/com/google/crypto/tink/subtle/AesGcmJce.java

About the AAD, the docs say:

On Android KitKat (API level 19) this method does not support non null or non empty {@code associatedData}. It might not work at all in older versions.

For the params, they first check that GCMParameterSpec class exists and if not they use IvParameterSpec. I tried that and it doesn't work, because the class does exist but it's not implemented.

private static AlgorithmParameterSpec getParams(...) {
    try {
        Class.forName("javax.crypto.spec.GCMParameterSpec");
        return new GCMParameterSpec(8 * TAG_SIZE_IN_BYTES, buf, offset, len);
    } catch (ClassNotFoundException e) {
        if (SubtleUtil.isAndroid()) {
            // GCMParameterSpec should always be present in Java 7 or newer, but it's missing on Android
            // devices with API level < 19. Fortunately, with a modern copy of Conscrypt (either through
            // GMS or bundled with the app) we can initialize the cipher with just an IvParameterSpec.
            // It will use a tag size of 128 bits. We'd double check the tag size in encrypt().
            return new IvParameterSpec(buf, offset, len);
        }
    }
    throw new GeneralSecurityException("cannot use AES-GCM: javax.crypto.spec.GCMParameterSpec not found");
}

@patrickfav
Copy link
Owner

patrickfav commented Oct 28, 2018

@davidmigloz I finally had time to tackle this issue. I implemented a version in #31 supporting migration and implementing AES/CBC + MAC for Kitkat. The work on this PR is highly appreciated, but I think its a better idea to go with #31, so I will close this PR.

If you have time please take a look at #31, since I need a second pair of eyes to check the encrypt-then-mac logic :)

@patrickfav patrickfav closed this Oct 28, 2018
@davidmigloz
Copy link
Contributor Author

Sure, it's a much better approach than this :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants