NegativeArraySizeException when getting value with parallel access

MarkVillacampa commented 5 years ago

Any idea what could cause this exception to be raised?

Caused by java.lang.NegativeArraySizeException: -16
       at at.favre.lib.armadillo.AesGcmEncryption.decrypt(SourceFile:99)
       at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(SourceFile:152)
       at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SourceFile:518)
       at at.favre.lib.armadillo.SecureSharedPreferences.getString(SourceFile:149)

patrickfav commented 5 years ago

Hi, Do you have a more complete test case? I probably can't help you with the exception alone. Important would be the config of your armadillo instance and your put/get code.

MarkVillacampa commented 5 years ago

Thanks for your quick answer. Here's some more info:

Config:

Armadillo.create(context, "SECURE_STORAGE")
            .encryptionFingerprint(context)
            .build()

Code that sets the value:

securePreferences.edit().putString("key", value).apply()

Code that get's the value

securePreferences.getString("key", "")

This is the complete crash:

Caused by java.lang.NegativeArraySizeException-16 Raw Text
--
  | at.favre.lib.armadillo.AesGcmEncryption.decrypt (SourceFile:99)
  | at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt (SourceFile:152)
  | at.favre.lib.armadillo.SecureSharedPreferences.decrypt (SourceFile:518)
  | at.favre.lib.armadillo.SecureSharedPreferences.getString (SourceFile:149)
  | myapp.LockService.onActivityStarted (SourceFile:43)
  | android.app.Application.dispatchActivityStarted (Application.java:279)
  | android.app.Activity.onStart (Activity.java:1265)
  | android.support.v4.app.FragmentActivity.onStart (SourceFile:589)
  | android.support.v7.app.AppCompatActivity.onStart (SourceFile:177)
  | myapp.BaseActivity.onStart (SourceFile:36)
  | android.app.Instrumentation.callActivityOnStart (Instrumentation.java:1334)
  | android.app.Activity.performStart (Activity.java:7033)
  | android.app.Activity.performRestart (Activity.java:7107)
  | android.app.Activity.performResume (Activity.java:7112)
  | android.app.ActivityThread.performResumeActivity (ActivityThread.java:3653)
  | android.app.ActivityThread.handleResumeActivity (ActivityThread.java:3718)
  | android.app.ActivityThread$H.handleMessage (ActivityThread.java:1656)
  | android.os.Handler.dispatchMessage (Handler.java:105)
  | android.os.Looper.loop (Looper.java:169)
  | android.app.ActivityThread.main (ActivityThread.java:6595)
  | java.lang.reflect.Method.invoke (Method.java)
  | com.android.internal.os.Zygote$MethodAndArgsCaller.run (Zygote.java:240)
  | com.android.internal.os.ZygoteInit.main (ZygoteInit.java:767)

Pretty standard. It's worth noting this crash happened when coming back from the background/from a locked device.

After the crash, the value could be decrypted correctly after a subsequent launch.

The device was an "Asus Zenfone 3 Zoom ZE553KL" on Android 8.0.

MarkVillacampa commented 5 years ago

After some digging I could reproduce it locally. It seems to happen when trying to get a value from two different threads at the same time.

I assume there must be an internal stateful component that gets reused between decryptions and is not thread-safe, but I'm not familiar enough with the Armadillo internals to know where to look.

Let me know if you would like me to put together a test app that reproduces this issue, or you want me to test something specific.

patrickfav commented 5 years ago

I will look into it, thanks for bringing it up to my attention. The put/get SHOULD be fairly thread-safe, but it is impossible that the whole storage is (there is just too much state(change) to handle).

That being said, it is probably safest to NOT access the store in parallel. As a quick fix I would either guard the access with a ReentrantLock or use AsyncTask's serial executor: https://developer.android.com/reference/android/os/AsyncTask.html#SERIAL_EXECUTOR

I will however look into your specfic problem, but I can't tell you an ETA.

MarkVillacampa commented 5 years ago

Thanks, I will try to add add a workaround to not access the storage in parallel.

Meanwhile, I was able to easily reproduce the crash with this snippet:

        ArmadilloSharedPreferences shared = Armadillo.create(this, "foo")
                .encryptionFingerprint(this).build();

        shared.edit().putString("foo", "1").apply();
        shared.edit().putString("bar", "1").apply();

        new Thread() {
            @Override
            public void run() {
                while(true) {
                    shared.getString("foo", "");
                }
            }
        }.start();

        new Thread() {
            @Override
            public void run() {
                while(true) {
                    shared.getString("bar", "");
                }
            }
        }.start();

2019-01-29 15:43:36.406 20640-20660/at.favre.lib.securesharedpreferences E/AndroidRuntime: FATAL EXCEPTION: Thread-3
    Process: at.favre.lib.securesharedpreferences, PID: 20640
    at.favre.lib.armadillo.SecureSharedPreferenceCryptoException: could not decrypt 6e9556575c1acd970ea5d7bd983bf3b7bd4d1d65
        at at.favre.lib.armadillo.SimpleRecoveryPolicy.handleBrokenContent(SimpleRecoveryPolicy.java:36)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:520)
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149)
        at at.favre.lib.securesharedpreferences.MyApplication$2.run(MyApplication.java:39)
     Caused by: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not decrypt
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:154)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518)
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$2.run(MyApplication.java:39) 
     Caused by: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not decrypt
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:111)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:152)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518) 
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$2.run(MyApplication.java:39) 
     Caused by: javax.crypto.AEADBadTagException: error:1e000065:Cipher functions:OPENSSL_internal:BAD_DECRYPT
        at java.lang.reflect.Constructor.newInstance0(Native Method)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:430)
        at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.doFinalInternal(OpenSSLCipher.java:1012)
        at com.android.org.conscrypt.OpenSSLCipher.engineDoFinal(OpenSSLCipher.java:350)
        at javax.crypto.Cipher.doFinal(Cipher.java:2056)
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:109)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:152) 
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518) 
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$2.run(MyApplication.java:39) 
2019-01-29 15:43:36.407 20640-20659/at.favre.lib.securesharedpreferences E/AndroidRuntime: FATAL EXCEPTION: Thread-2
    Process: at.favre.lib.securesharedpreferences, PID: 20640
    at.favre.lib.armadillo.SecureSharedPreferenceCryptoException: could not decrypt 453d2fe6773a7f12abdcc56ea1c9d58dd2bced0f
        at at.favre.lib.armadillo.SimpleRecoveryPolicy.handleBrokenContent(SimpleRecoveryPolicy.java:36)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:520)
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149)
        at at.favre.lib.securesharedpreferences.MyApplication$1.run(MyApplication.java:30)
     Caused by: at.favre.lib.armadillo.EncryptionProtocolException: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not decrypt
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:154)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518)
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$1.run(MyApplication.java:30) 
     Caused by: at.favre.lib.armadillo.AuthenticatedEncryptionException: could not decrypt
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:111)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:152)
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518) 
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$1.run(MyApplication.java:30) 
     Caused by: javax.crypto.AEADBadTagException: error:1e000065:Cipher functions:OPENSSL_internal:BAD_DECRYPT
        at java.lang.reflect.Constructor.newInstance0(Native Method)
        at java.lang.reflect.Constructor.newInstance(Constructor.java:430)
        at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.doFinalInternal(OpenSSLCipher.java:1012)
        at com.android.org.conscrypt.OpenSSLCipher.engineDoFinal(OpenSSLCipher.java:350)
        at javax.crypto.Cipher.doFinal(Cipher.java:2056)
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:109)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:152) 
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518) 
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$1.run(MyApplication.java:30)

patrickfav commented 5 years ago

Thanks for the test case!

MarkVillacampa commented 5 years ago

The reason it fails to decrypt seems to be the fingerprint bytes are wrong, and this seems to be caused by ByteArrayRuntimeObfuscator not being thread safe.

However, after removing the ByteArrayRuntimeObfuscator and making the fingerprint byes static I still get crashes this time from inside the doFinal method. Is it possible Cipher is not thread safe? In that case, could it make sense to build a locking mechanism directly inside Armadillo?

 Caused by: java.lang.ArrayIndexOutOfBoundsException: in
        at com.android.org.conscrypt.NativeCrypto.EVP_AEAD_CTX_open(Native Method)
        at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.doFinalInternal(OpenSSLCipher.java:998)
        at com.android.org.conscrypt.OpenSSLCipher.engineDoFinal(OpenSSLCipher.java:350)
        at javax.crypto.Cipher.doFinal(Cipher.java:2056)
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:109)

     Caused by: java.lang.RuntimeException: error:1e000067:Cipher functions:OPENSSL_internal:BUFFER_TOO_SMALL
        at com.android.org.conscrypt.NativeCrypto.EVP_AEAD_CTX_open(Native Method)
        at com.android.org.conscrypt.OpenSSLCipher$EVP_AEAD.doFinalInternal(OpenSSLCipher.java:998)
        at com.android.org.conscrypt.OpenSSLCipher.engineDoFinal(OpenSSLCipher.java:350)
        at javax.crypto.Cipher.doFinal(Cipher.java:2056)
        at at.favre.lib.armadillo.AesGcmEncryption.decrypt(AesGcmEncryption.java:109)
        at at.favre.lib.armadillo.DefaultEncryptionProtocol.decrypt(DefaultEncryptionProtocol.java:153) 
        at at.favre.lib.armadillo.SecureSharedPreferences.decrypt(SecureSharedPreferences.java:518) 
        at at.favre.lib.armadillo.SecureSharedPreferences.getString(SecureSharedPreferences.java:149) 
        at at.favre.lib.securesharedpreferences.MyApplication$1.run(MyApplication.java:34)

MarkVillacampa commented 5 years ago

Seems like adding synchronized to these two methods fixes the issue. Does that have any other (performance?) implications to take into account?

https://github.com/patrickfav/armadillo/blob/master/armadillo/src/main/java/at/favre/lib/armadillo/SecureSharedPreferences.java#L506-L523

I'm happy to send a PR if you're ok with this solution.

patrickfav commented 5 years ago

Does that have any other (performance?) implications to take into account?

No this should be fine. The obfuscation step is probably under a ms. It should suffice to just guard the createAndEncrypt() method, since it is the only one to mutate the internal byte array (and probably the cause).

Is it possible Cipher is not thread safe?

The cipher instance is definitely not thread safe. The AesGcmEncryption class caches the cipher instance, which is probably the cause of this issue. Im not sure how expenisve it is to generate a cipher class (with using OpenSSL jni in the background and all), so I suggest maybe using it lazily with a ThreadLocal wrapper?

I'm happy to send a PR if you're ok with this solution.

I would very welcome that. 🥇 If you do, try to include a test for parallel access.

Thanks again for all your analyzing, very much appreciated. Seems I choose to "forget" how awfully not thread safe this implementation is :)

patrickfav commented 5 years ago

... should suffice to just guard the createAndEncrypt() method, since it is the only one to mutate the internal byte array

Scrap that, you need to sync getBytes aswell of course.

With quick testing this patch (in addition to fixing the obfuscator) makes your earlier test case work:

AesGcmEncryption.java


    private ThreadLocal<Cipher> cipherWrapper;

...

    private synchronized Cipher getCipher() {
        if (cipherWrapper == null || cipherWrapper.get() == null) {
            try {
                cipherWrapper = new ThreadLocal<>();
                if (provider != null) {
                    cipherWrapper.set(Cipher.getInstance(ALGORITHM, provider));
                } else {
                    cipherWrapper.set(Cipher.getInstance(ALGORITHM));
                }
            } catch (Exception e) {
                throw new IllegalStateException("could not get cipher instance", e);
            }
        }
        return cipherWrapper.get();
    }

We should better also fix AesCbcEncryption for our friends on Kitkat.

MarkVillacampa commented 5 years ago

Fixed with #41

patrickfav commented 5 years ago

@MarkVillacampa fyi 0.8.0 was just released

patrickfav / armadillo

NegativeArraySizeException when getting value with parallel access #40