martinkasa / capacitor-secure-storage-plugin

Capacitor plugin for storing string values securly on iOS and Android.
MIT License
155 stars 56 forks source link

Issues getting stored key on Android? #8

Closed yulemata closed 4 years ago

yulemata commented 4 years ago

Hi,

I am experiencing an error on Android. Somehow I does not allow me to access the stored key. It works fine on IOS.

Saving the key works fine (i get true on success). However, when I am trying to get the stored key I get the following error:

E/Capacitor/Plugin: error java.lang.NullPointerException: Attempt to get length of null array at java.lang.StringFactory.newStringFromBytes(StringFactory.java:249) at com.whitestein.securestorage.SecureStoragePlugin.get(SecureStoragePlugin.java:42) at java.lang.reflect.Method.invoke(Native Method) at com.getcapacitor.PluginHandle.invoke(PluginHandle.java:99) at com.getcapacitor.Bridge$2.run(Bridge.java:537) at android.os.Handler.handleCallback(Handler.java:883) at android.os.Handler.dispatchMessage(Handler.java:100) at android.os.Looper.loop(Looper.java:214) at android.os.HandlerThread.run(HandlerThread.java:67)

I am trying to save an application access_token (that is valid forever) in the secure storage. My set javascript method is as follows:

`async setAccessToken(access_token:string){

const key = 'access_token';
const value = access_token;

try {

  let success = await SecureStoragePlugin.set({ key, value })
  console.log('success saving token '+JSON.stringify(success))

} catch (error) {
  return error
}

}`

my get method:

` async getAccessToken(){

  try {

    const key = 'access_token';

    let object = await SecureStoragePlugin.get({ key });
    console.log('Getting access token '+object)

    return object.value

  } catch (error) {

    return error

  }    

} `

I am doing the test in the Android emulator in Android Studio and the device has a lockScreen PIN set. Any ideas what the problem might be?

lonwi commented 4 years ago

I have the same issue, it works fine if I close and open the app again.

Mageek627 commented 4 years ago

I have the same issue and it seems to depend on the length of the value string. Error when length is >=247

martinkasa commented 4 years ago

Do you guys use latest version of the plugin? Support for long values on Android was added in v0.3.0

Mageek627 commented 4 years ago

@martinkasa I am using v0.3.0

Proper-Job commented 4 years ago

@yulemata make sure your stored value is a string. Using anything but a string results in data loss on iOS and creates an exception on Android.

harshithmohan commented 4 years ago

I have the same problem too. I'm storing a string.

fmflame commented 4 years ago

Same problem with Android here (using v0.3.0 as well). set is working correctly, but get results in:

Error: error
    at Object.fromNative (capacitor-runtime.js:226)
    at <anonymous>:1:18

Any ideas on how to fix this? - it happens when I haven't set the data yet and also when I have set a simple object as data.

martinkasa commented 4 years ago

@harshithmohan @fmflame did you run

npx cap sync

after installing v0.3.0? I have just tried it in my app and everything seems to work correctly here.

Just to summarize steps you need to do:

  1. install plugin by npm
  2. run "npx cap sync"
  3. in an android project you have to register the plugin in MainActivity.java
    import com.whitestein.securestorage.SecureStoragePlugin;
    .
    .
    this.init(savedInstanceState, new ArrayList<Class<? extends Plugin>>() {{
      // Additional plugins you've installed go here
      // Ex: add(TotallyAwesomePlugin.class);
      add(SecureStoragePlugin.class);
    }});

If you do not register the plugin, then Web implementation is used instead of native.

fmflame commented 4 years ago

@martinkasa yes I did all of that and it's not working. Tested on android emulator API 28 (android 9), and on my android phone Xiaomi Mi A2 Android one which is API 29 (android 10).

Installed the npm package v0.3.0 from the start (so no way there's a package problem here) -> run npx cap sync ->

package io.symbiome.symbiome;

import android.os.Bundle;

import com.getcapacitor.BridgeActivity;
import com.getcapacitor.Plugin;

import java.util.ArrayList;

import com.whitestein.securestorage.SecureStoragePlugin;

public class MainActivity extends BridgeActivity {
  @Override
  public void onCreate(Bundle savedInstanceState) {
    super.onCreate(savedInstanceState);

    // Initializes the Bridge
    this.init(savedInstanceState, new ArrayList<Class<? extends Plugin>>() {{
      // Additional plugins you've installed go here
      // Ex: add(TotallyAwesomePlugin.class);
      add(SecureStoragePlugin.class);
    }});
  }
}

Without npx cap sync the changes in the MainActivity didn't work because It couldn't find the import com.whitestein.securestorage.SecureStoragePlugin;. But after the sync it was able to correctly find the import and build the project.

So i don't think it's a setup issue since set is working. Only get doesn't work, which makes it seem like a bug.

P.S One side note, on typescript v3.6.3 and @capacitor/core v1.3.0 your definition file with the PluginRegistry interface, doesn't work for me. When typing Plugins. -> no autocompletion forSecureStoragePlugin. I had to add my own definition to fix the issue which looks like this:

import { SecureStoragePluginPlugin } from 'capacitor-secure-storage-plugin';

declare module '@capacitor/core/dist/esm/core-plugin-definitions' {

    interface PluginRegistry {
        SecureStoragePlugin: SecureStoragePluginPlugin;
    }
}

The only update here compared to your file is the declare module '@capacitor/core/dist/esm/core-plugin-definitions', which is changed from declare module declare module '@capacitor/core'

martinkasa commented 4 years ago

Can you try to debug what is the actual cause? Search for PasswordStorageHelper_SDK18 class in your project and put breakoint to getData method. Then try to call get.

fmflame commented 4 years ago

This is the code where the exception is thrown in SecureStoragePlugin.java:

public void get(PluginCall call) {
        String key = call.getString("key");

        try {
            String value = new String(this.passwordStorageHelper.getData(key), Charset.forName("UTF-8"));
            JSObject ret = new JSObject();
            ret.put("value", value);
            call.success(ret);
        }
        catch ( Exception exception) {
            call.error("error", exception);
        }
    }

The error is: java.lang.NullPointerException: Attempt to get length of null array Stack trace:

java.lang.StringFactory.newStringFromBytes(StringFactory.java:256)
com.whitestein.securestorage.SecureStoragePlugin.get(SecureStoragePlugin.java:42)
java.lang.reflect.Method.invoke(Native Method)
com.getcapacitor.PluginHandle.invoke(PluginHandle.java:99)
...
fmflame commented 4 years ago

Ok so I investigated further and there are 2 issues: 1) I've updated this code in SecureStoragePlugin.java to check if data is null in order to fix the get error:

public void get(PluginCall call) {
        String key = call.getString("key");

        try {
            byte[] data = this.passwordStorageHelper.getData(key);
            String value = null;
            if (data != null && data.length != 0) {
              value = new String(data, Charset.forName("UTF-8"));
            }
            JSObject ret = new JSObject();
            ret.put("value", value);
            call.success(ret);
        }
        catch ( Exception exception) {
            call.error("error", exception);
        }
    }

This resolves the issue, but I am still not sure if this is enough and here comes the second issue.

  1. I've also debugged if set works and it doesn't. In my app it returns success, but the reality is that it doesn't. It silently fails will exception as of what the guys above have said with the string length. Here is the problem code:

    @SuppressLint("TrulyRandom")
        private static String encrypt(PublicKey encryptionKey, byte[] data) throws NoSuchAlgorithmException,
                NoSuchPaddingException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
                NoSuchProviderException, InvalidKeySpecException {
    
            if (data.length <= KEY_LENGTH) {
                Cipher cipher = Cipher.getInstance(RSA_ECB_PKCS1_PADDING);
                cipher.init(Cipher.ENCRYPT_MODE, encryptionKey);
                byte[] encrypted = cipher.doFinal(data);
                return Base64.encodeToString(encrypted, Base64.DEFAULT);
            } else {
                Cipher cipher = Cipher.getInstance("RSA/ECB/PKCS1Padding");
                cipher.init(Cipher.ENCRYPT_MODE, encryptionKey);
                int limit = KEY_LENGTH / 8 - 11;
                int position = 0;
                ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
                while (position < data.length) {
                    if (data.length - position < limit)
                        limit = data.length - position;
                    byte[] tmpData = cipher.doFinal(data, position, limit);
                    try {
                        byteArrayOutputStream.write(tmpData);
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                    position += limit;
                }
    
                return Base64.encodeToString(byteArrayOutputStream.toByteArray(), Base64.DEFAULT);
            }
        }

    The data I set falls into the first conditional statement (it may also fail in the second one, so it should be checked as well) and then fails at this line: byte[] encrypted = cipher.doFinal(data); with this error:

    Error: javax.crypto.IllegalBlockSizeException: input must be under 256 bytes
    Stack trace:
    com.android.org.conscrypt.OpenSSLCipherRSA.engineDoFinal(OpenSSLCipherRSA.java:299)
    javax.crypto.Cipher.doFinal(Cipher.java:2055)
    com.whitestein.securestorage.PasswordStorageHelper$PasswordStorageHelper_SDK18.encrypt(PasswordStorageHelper.java:319)
    com.whitestein.securestorage.PasswordStorageHelper$PasswordStorageHelper_SDK18.setData(PasswordStorageHelper.java:256)
    com.whitestein.securestorage.PasswordStorageHelper.setData(PasswordStorageHelper.java:72)
    com.whitestein.securestorage.SecureStoragePlugin.set(SecureStoragePlugin.java:27)
    ...

It's incredibly weird that you are not seeing these issues... Let me know if I can assist you somehow to resolve them, would be happy to, but this is out of my expertise to fix on my own any further.

So would you please be able to look into this?

fmflame commented 4 years ago

One more note here, for now I've managed to workaround this by using the official Capacitor Storage plugin for Android which uses SharedPreferences as well and works. My advice here would be to just update your implementation to do what they do? Shouldn't be that hard I guess. Would be nice to just use your plugin for both iOS and Android.

harshithmohan commented 4 years ago

@fmflame SharedPrefs is stored in an un-encrypted state and anyone or any app can read it. It's not safe for data which needs to be secured.

fmflame commented 4 years ago

@harshithmohan This is for the official Capacitor Storage plugin I assume, ok so a need of a fix for this plugin is still needed then @martinkasa :/

fmflame commented 4 years ago

I've made the following fixes to encrypt and decrypt functions, so that the set code would work: 1) if (data.length <= KEY_LENGTH / 8 - 11) { <--- changed from if (data.length <= KEY_LENGTH) {

private static String encrypt(PublicKey encryptionKey, byte[] data) throws NoSuchAlgorithmException,
                NoSuchPaddingException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
                NoSuchProviderException, InvalidKeySpecException {

            if (data.length <= KEY_LENGTH / 8 - 11) {
                Cipher cipher = Cipher.getInstance(RSA_ECB_PKCS1_PADDING);
                cipher.init(Cipher.ENCRYPT_MODE, encryptionKey);
                byte[] encrypted = cipher.doFinal(data);
                return Base64.encodeToString(encrypted, Base64.DEFAULT);
            } else {
                Cipher cipher = Cipher.getInstance("RSA/ECB/PKCS1Padding");
                cipher.init(Cipher.ENCRYPT_MODE, encryptionKey);
                int limit = KEY_LENGTH / 8 - 11;
                int position = 0;
                ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
                while (position < data.length) {
                    if (data.length - position < limit)
                        limit = data.length - position;
                    byte[] tmpData = cipher.doFinal(data, position, limit);
                    try {
                        byteArrayOutputStream.write(tmpData);
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                    position += limit;
                }

                return Base64.encodeToString(byteArrayOutputStream.toByteArray(), Base64.DEFAULT);
            }
        }

2) if (encryptedBuffer.length <= KEY_LENGTH / 8) { <--- changed from if (encryptedBuffer.length <= KEY_LENGTH) {

private static byte[] decrypt(PrivateKey decryptionKey, String encryptedData) throws NoSuchAlgorithmException,
                NoSuchPaddingException, InvalidKeyException, IllegalBlockSizeException, BadPaddingException,
                NoSuchProviderException {
            if (encryptedData == null)
                return null;
            byte[] encryptedBuffer = Base64.decode(encryptedData, Base64.DEFAULT);

            if (encryptedBuffer.length <= KEY_LENGTH / 8) {
                Cipher cipher = Cipher.getInstance(RSA_ECB_PKCS1_PADDING);
                cipher.init(Cipher.DECRYPT_MODE, decryptionKey);
                return cipher.doFinal(encryptedBuffer);
            } else {
                Cipher cipher = Cipher.getInstance("RSA/ECB/PKCS1Padding");
                cipher.init(Cipher.DECRYPT_MODE, decryptionKey);
                int limit = KEY_LENGTH / 8;
                int position = 0;
                ByteArrayOutputStream byteArrayOutputStream = new ByteArrayOutputStream();
                while (position < encryptedBuffer.length) {
                    if (encryptedBuffer.length - position < limit)
                        limit = encryptedBuffer.length - position;
                    byte[] tmpData = cipher.doFinal(encryptedBuffer, position, limit);
                    try {
                        byteArrayOutputStream.write(tmpData);
                    } catch (IOException e) {
                        e.printStackTrace();
                    }
                    position += limit;
                }

                return byteArrayOutputStream.toByteArray();
            }
        }

@martinkasa could you confirm this fix for the set method and also push this simple change if it is correct? The fix for the get method is in a commend above.

martinkasa commented 4 years ago

@fmflame thanks, I will test it and merge if it works, probably today in the evening.

fmflame commented 4 years ago

@martinkasa thanks! please check out the get fix as well. Also for testing - I've encountered this issue while trying to set this data if that helps: {"token":"eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpZCI6IjE0MzRlMjY1LWExNTItNDY2Ni1hMTBlLTBmM2FjZTUwOThmMiIsImVtYWlsIjp0cnVlLCJpYXQiOjE1ODEwODAyNjQsImV4cCI6MTU4MTA4Mzg2NH0.6i4oLQMez1zJNmqDdSiS4B9NoEiqub8fwH6FVWBnw4w","expiresIn":604800,"expiresAt":1582019044016}

martinkasa commented 4 years ago

@fmflame please, try to install version "0.3.1-testing" version from npm and let me know if it works for you.

fmflame commented 4 years ago

@martinkasa Great! Yes, it works for me. I see you've fixed some strings -> constants and the error handling, very nice :)

martinkasa commented 4 years ago

@fmflame ok, released as version 0.3.1