Huawei Android 7.0 java.lang.NullPointerException

[phillbaker]

Thanks for the hard work on this package!

I'm not sure this is an issue with this library, but I wanted to document it in case others are also experiencing it (it looks similar to iamMehedi/Secured-Preference-Store#25).

When uploading our app to the Google Play store, google's pre-launch report indicates that the app is crashing with a null pointer exception (see stack trace at bottom). On launch our app (abbreviated) calls:

  createStoreSecret = () => Keychain
    .setGenericPassword('user', 'password')
    .then(() => {
      // success
    })

  Keychain
    .getGenericPassword()
    .then((credentials) => {
      if (credentials) {
        // success
      } else {
        createStoreSecret();
      }
    }).catch(() => {
      createStoreSecret();
    });

Should we not do this?

Using version 2.0.0-rc of this package.

Environment:
  OS: macOS Sierra 10.12.6
  Node: 6.9.5
  Yarn: Not Found
  npm: 3.10.10
  Watchman: 4.7.0
  Xcode: Xcode 9.1 Build version 9B55
  Android Studio: Not Found

Packages: (wanted => installed)
  react: 16.0.0
  react-native: 0.51.1

Stacktrace:

java.lang.NullPointerException: Attempt to invoke interface method 'int android.security.IKeystoreService.del(java.lang.String, int)' on a null object reference
	at android.security.KeyStore.delete(KeyStore.java:186)
	at android.security.Credentials.deletePrivateKeyTypeForAlias(Credentials.java:292)
	at android.security.Credentials.deleteAllTypesForAlias(Credentials.java:251)
	at android.security.keystore.AndroidKeyStoreKeyGeneratorSpi.engineGenerateKey(AndroidKeyStoreKeyGeneratorSpi.java:329)
	at javax.crypto.KeyGenerator.generateKey(KeyGenerator.java:580)
	at com.oblador.keychain.cipherStorage.CipherStorageKeystoreAESCBC.encrypt(CipherStorageKeystoreAESCBC.java:78)
	at com.oblador.keychain.KeychainModule.setGenericPasswordForOptions(KeychainModule.java:65)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.facebook.react.bridge.JavaMethodWrapper.invoke(JavaMethodWrapper.java:374)
	at com.facebook.react.bridge.JavaModuleWrapper.invoke(JavaModuleWrapper.java:162)
	at com.facebook.react.bridge.queue.NativeRunnable.run(Native Method)
	at android.os.Handler.handleCallback(Handler.java:755)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at com.facebook.react.bridge.queue.MessageQueueThreadHandler.dispatchMessage(MessageQueueThreadHandler.java:31)
	at android.os.Looper.loop(Looper.java:156)
	at com.facebook.react.bridge.queue.MessageQueueThreadImpl$3.run(MessageQueueThreadImpl.java:194)
	at java.lang.Thread.run(Thread.java:776)

[maxkomarychev]

@phillbaker did you have a chance to check this on other android versions? is it only specific to 7.0 on Huawei?

[phillbaker]

It is specific to 7.0 on Huawei and noticeable mostly through the Google Play store's automated testing.

[emartynov]

I see the same crash in Google Play pre-launch report. However, I got this device from eBay and was able to run the app without issues. So I also contacted Google for more explanations and got an answer they are pretty confident in their tests. So I'm a bit confused now.

[maxkomarychev]

@emartynov hey, is the device that you've got is the same model and android version?
In my case it was Mate 9 android 7.0

[maxkomarychev]

what is the purpose of generator.generateKey()

what is happening to this secret key? is it getting written somewhere implicitly?

cc @oblador

[emartynov]

Yes, Mate 9 and Android 7

[vonovak]

@maxkomarychev upon quick look at the code you linked, that indeed seems suspicious; I'd guess the retuned key should be assigned to the Key variable which is defined later. If this is a bug, it would be beneficial to extract obtaining of the key into a separate method.

[maxkomarychev]

After looking around in the code I found out exceptions are handled in two different ways:

1. either individual exceptions are handled like in enchrypt or decrypt
2. generic Exception is handled in case of encryptString
   and decryptBytes

It looks like generic catch (Exception e) { ... } should be added here (and in all other places as well)

react-native-keychain/android/src/main/java/com/oblador/keychain/cipherStorage/CipherStorageKeystoreAESCBC.java

Line 91 in c3990b0

}

so the app at least won't crash and the exception can be properly handled on the side of js.

Thoughts?

[vonovak]

@maxkomarychev it seems like you spent some time with the code and got some insights; I suggest you open a PR and we can iterate on the proposed improvements there.

[maxkomarychev]

@vonovak sure, I'll try to prepare something tomorrow.

[maxkomarychev]

@vonovak I posted a fix here #134
With this code my try {} catch (error) {} on js side is able to prevent application crash and build is passing pre launch report.

[maxkomarychev]

@emartynov @phillbaker will appreciate if you guys can try this on your side if this issue is still relevant to you. Thanks.

You can try this fix with this line in your package.json:

"react-native-keychain": "git+https://github.com/maxkomarychev/react-native-keychain#catch-uncaught",

[emartynov]

Unfortunately, I got this crash with own crypto functionality, so I can not confirm. As well, the app will not crash but at the same time, you lost secure storage. Are you sure want to operate with this conditions? Sure, you can report to the user that app will not work on this device, but I really not sure if it is the best option.

[maxkomarychev]

I would love if the app won't crash in first place, the rest depends on the specific functionality the specific app provides. While my PR doesn't solve the core problem it is at least making it possible to handle the error on js side.

[phillbaker]

@maxkomarychev makes sense to me - if we do get an exception in JS we can display the problem to the user and handle appropriately.

[oblador]

This should be "solved" in 3.0.0. Closing this for now, reopen if issues persist.