scottyab / secure-preferences

Android Shared preference wrapper than encrypts the values of Shared Preferences. It's not bullet proof security but rather a quick win for incrementally making your android app more secure.
1.53k stars 235 forks source link

Updating to 0.1.4 from 0.1.3 return null #32

Closed javiersantos closed 9 years ago

javiersantos commented 9 years ago

I have updated the library to 0.1.4 and now, when I access to any secure preference it returns null and the app closes with a NullException.

Is there something that I need to do after the update? Thanks!

scottyab commented 9 years ago

Hmmm, no? can you supply more details like which constructor are you using? and can you supply examples of the data you are storing? also android device and version.

Certainly we need some better migration tests to pick this up.

javiersantos commented 9 years ago

I have been testing the problem further. My device is a Nexus 4, running Android 5.1.1 (CyanogenMod).

I get a RuntimeException using the secure preferences created from 0.1.3 and updating to 0.1.4 and vice versa. I don't have modified anything about the secure preference.

Here you are the constructor:

public AppSecurePreferences(Context context) {
        this.sharedPreferences = new SecurePreferences(context, MY_PASSWORD, "app_profile_preferences");
        this.editor = sharedPreferences.edit();
} 

The method trying to access at the class secure preferences:

public Integer getUserInt() {
        return sharedPreferences.getInt(KeyUserInt, 0);
}

The log shows:

E/AndroidRuntime: FATAL EXCEPTION: main
E/AndroidRuntime: Process: com.my.app, PID: 4041
E/AndroidRuntime: java.lang.RuntimeException: Unable to start receiver com.my.app.MyReceiver: java.lang.ClassCastException: Invalid int: "null"
E/AndroidRuntime:     at android.app.ActivityThread.handleReceiver(ActivityThread.java:2649)
E/AndroidRuntime:     at android.app.ActivityThread.access$1800(ActivityThread.java:154)
E/AndroidRuntime:     at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1398)
E/AndroidRuntime:     at android.os.Handler.dispatchMessage(Handler.java:102)
E/AndroidRuntime:     at android.os.Looper.loop(Looper.java:135)
E/AndroidRuntime:     at android.app.ActivityThread.main(ActivityThread.java:5294)
E/AndroidRuntime:     at java.lang.reflect.Method.invoke(Native Method)
E/AndroidRuntime:     at java.lang.reflect.Method.invoke(Method.java:372)
E/AndroidRuntime:     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904)
E/AndroidRuntime:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699)
E/AndroidRuntime:  Caused by: java.lang.ClassCastException: Invalid int: "null"
E/AndroidRuntime:     at com.securepreferences.SecurePreferences.getInt(SecurePreferences.java:386)
E/AndroidRuntime:     at com.my.app.utils.AppSecurePreferences.getUserInt(AppSecurePreferences.java:120)
E/AndroidRuntime:     at com.my.app.MyReceiver.onReceive(MyReceiver.java:26)
E/AndroidRuntime:     at android.app.ActivityThread.handleReceiver(ActivityThread.java:2642)
E/AndroidRuntime:     at android.app.ActivityThread.access$1800(ActivityThread.java:154) 
E/AndroidRuntime:     at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1398) 
E/AndroidRuntime:     at android.os.Handler.dispatchMessage(Handler.java:102) 
E/AndroidRuntime:     at android.os.Looper.loop(Looper.java:135) 
E/AndroidRuntime:     at android.app.ActivityThread.main(ActivityThread.java:5294) 
E/AndroidRuntime:     at java.lang.reflect.Method.invoke(Native Method) 
E/AndroidRuntime:     at java.lang.reflect.Method.invoke(Method.java:372) 
E/AndroidRuntime:     at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:904) 
E/AndroidRuntime:     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:699) 
scottyab commented 9 years ago

Thanks for providing stack trace. Seems than some how your KeyUserInt was saved as a string "null' when saving to preferences this could happen if the encrypt failed. The more I look at this, I regretting the decision not to throw exception when encrypt fails.

If you switch back to 0.1.3 does it start working? my guess would be no.

On a side note: do you have the password as a String constant? if so this isn't great security. It should be something each user provides. If you don't have a password from the user then please do not use a hardcoded one instead let the library generate one (more secure than a String constant). Thanks

javiersantos commented 9 years ago

Switching back to 0.1.3 also shows the Exception. Thanks for your time!

scottyab commented 9 years ago

Closing and not an issue with 0.1.4 - please reopen if i've misunderstood.

cermakcz commented 8 years ago

I know this is not an issue with migration from 0.1.3 to 0.1.4, but it's still an issue. It's happening to me as well and I'm not sure what's the best way to handle it. I can't put all calls to try-catch, as that would be ugly. I think that SecurePreferences should just return the default value in such case. And like you said, throw an exception when the encryption fails, because that seems to be the actual issue here.

cermakcz commented 8 years ago

Hi @scottyab, would you consider pulling this change, which makes the encrypt/decrypt methods throw an exception? https://github.com/cermakcz/secure-preferences/commit/502eee65b7d7626374b6a65723168a8a10d9a279

If so, let me know and I'll prepare a pull request.

scottyab commented 8 years ago

Hey @cermakcz, considered and short answer yes! thanks for your efforts. However I'm thinking that we add a throw on enc/dec error flag to constructor that defaults to false. This way it's backwards compatible and devs that upgrade don't unexpectedly get runtime exceptions. How does that sound?

cermakcz commented 8 years ago

I'm a bit afraid of polluting the code with such flags. You'd have to do it with every non-backwards compatible change. It might be better to just bump the major version and mention it in the release notes?