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

Crash while calling getInt with "null" saved. #37

Open inktomi opened 8 years ago

inktomi commented 8 years ago
  Caused by: java.lang.ClassCastException: Invalid int: "null" 
     at com.securepreferences.SecurePreferences.getInt(SecurePreferences.java:389)

This is a valid (but bad practice) call:

   Integer test = null;
   editor.putInt("key", test);

It results in "null" being set in shared preferences because we .toString the Integer value before saving it off encrypted. Then, when we call editor.getInt("key") we get the crash above.

inktomi commented 8 years ago

I'm torn on even really keeping this open. It's valid to throw ClassCastException if the value of a key is not a number according to SharedPreferences, butI feel like it would be less likely to actually happen as above since the values are not written to Strings before saving and so you won't be actually saving "null".

cermakcz commented 8 years ago

This seems like the same issue as https://github.com/scottyab/secure-preferences/issues/32.

From what Scott said, it seems like it's not about actually passing null to putInt, but more like putInt internally fails during encryption and stores null instead, which is a real problem.

mkaarthick commented 8 years ago

Any workaround?

cermakcz commented 8 years ago

Actually my problem wasn't that null was written, but that the key used for encrypting/decrypting changed, thus it wasn't possible to decrypt the value, null was returned and ClassCastException was raised. The key changed because it was salted with ANDROID_ID, which changed, even though it shouldn't. I don't know why it did, or if it was on some rooted device and someone changed it manually. Anyway, the default implementation also uses ANDROID_ID for salt, so keep it in mind.

scottyab commented 8 years ago

Hey @cermakcz , it should get the device's Serial number if that's empty for some reason it falls back to the Android_ID. The serial should stay statis AFAIK, Saying that the Android_ID isn't the most ideal fallback as it'd has been known to change.

@inktomi I guess workaround is for the getInt method to check the type before casting and rather than throw classcast instead return null. I'm also looking at cermakcz@502eee6 proposed change that might help stop this occuring.

gauravniv commented 6 years ago

Hey @scottyab any timeline on the availability of the fix? Sorry to be a bother , but I have been using the library from a long time and the crashes due to this issue are piling up now. As a work around for now I've stopped storing ints and storing strings instead!

oseparovic commented 6 years ago

I'm still seeing this as well on 0.1.4. It's fairly rare though for whatever reason.

This is the relevant section of the stacktrace:

Caused by: java.lang.ClassCastException: 
  at com.securepreferences.SecurePreferences.getInt (SecurePreferences.java:389)

From the lib source code this happens when return Integer.parseInt(decrypt(encryptedValue)); causes a NumberFormatException.

For performance reasons I've created a singleton out of our secure preferences as running the constructor can be very slow so I want to do that as little as possible:

    public static SharedPreferences getSecurePreferencesRef(Context context) {
        if (sharedPreferences == null) {
            sharedPreferences = new SecurePreferences(context, <key>, <xml>);
        }

        return sharedPreferences;
    }

Any suggestions would be appreciated. I tried updating to v0.1.5 or v0.1.6 from jitpack but installing overtop an existing app with 0.1.4 using either of those new versions reset all of our secure preferences.

gauravniv commented 6 years ago

One possible solution to this is to stop using the getInt method altogether. I have created a wrapper that creates an object for every shared pref int operation, that object holds the default value expected. When reading/writing to prefs just perform a string operation (read and parse to int/ String.valueOf(int) when writing) rather than int, catch the exceptions being thrown yourself, and return default value if exception happens. Worked for me so far!

gauravniv commented 6 years ago

public class IntPreference { private final SharedPreferences preferences; private final String key; private final int integerDefaultValue; private final String defaultValue; //using String because of getInt issues mentioned here: // https://github.com/scottyab/secure-preferences/issues/37

public IntPreference(final SharedPreferences preferences, final String key) { this(preferences, key, -1); }

public IntPreference( final SharedPreferences preferences, final String key, final int defaultValue) { this.preferences = preferences; this.key = key; this.integerDefaultValue = defaultValue; this.defaultValue = String.valueOf(defaultValue); }

public void delete() { preferences.edit().remove(key).apply(); }

public int get() {

int finalData = integerDefaultValue;
String prefData = preferences.getString(key, defaultValue);
try {
  finalData = Integer.parseInt(prefData);
} catch (ClassCastException | NumberFormatException | NullPointerException ex) {
  Timber.e(ex.getMessage());
}
return finalData;

}

public boolean isSet() { return preferences.contains(key); }

public void set(final int value) { preferences.edit().putString(key, String.valueOf(value)).apply(); } }

oneHamidreza commented 5 years ago

i've found problem. if your key has digit then this exception happend. just remove digit from your KEY