Crashing in API 28 (Android Pie) s== null (DO NOT USE THIS LIBRARY, it is completely broken)

[bitwalrus0]

2019-01-19 13:02:09.533 3402-3402/com.strongapps.frettrainer.android E/AndroidRuntime: FATAL EXCEPTION: main Process: com.strongapps.frettrainer.android, PID: 3402 java.lang.ClassCastException: s == null at com.securepreferences.SecurePreferences.getInt(SecurePreferences.java:389)

Crashes in Android API 28 when trying to get a pref.

[wantroba]

It happened to me

[bitwalrus0]

Every user in my app is now getting this crash in production when upgrading to Pie. The only thing that fixes it is if they reinstall my app. Upgrading causes this crash.

[bitwalrus0]

Dug into the crash, got all the way to AesCbcWithIntegrity returning an exception on line 409 in the "decrypt" method as shown below.

throw new GeneralSecurityException("MAC stored in civ does not match computed MAC.");

[bitwalrus0]

https://android-developers.googleblog.com/2018/03/cryptography-changes-in-android-p.html

[bitwalrus0]

Related: https://github.com/tozny/java-aes-crypto/issues/19

[bitwalrus0]

Problem is most likely caused by getSerialNumber returning "UNKNOWN" in SDK 28.

Need to check for permission and then call Build.getSerial() if API version is >= 28.

    private static String getDeviceSerialNumber(Context context) {
        // We're using the Reflection API because Build.SERIAL is only available
        // since API Level 9 (Gingerbread, Android 2.3).
        try {
            String deviceSerial = (String) Build.class.getField("SERIAL").get(
                    null);
            if (TextUtils.isEmpty(deviceSerial)) {
                return Settings.Secure.getString(
                        context.getContentResolver(),
                        Settings.Secure.ANDROID_ID);
            }else {
                return deviceSerial;
            }
        } catch (Exception ignored) {
            // Fall back  to Android_ID
            return Settings.Secure.getString(context.getContentResolver(),
                    Settings.Secure.ANDROID_ID);
        }
    }

[icoolguy1995]

Experienced the same. Has anyone figured out any workaround?

[bitwalrus0]

I'm going to make my own branch since this guy's not going to do anything to help us.

Gonna create a new constructor that allows a context and salt parameter, and calls the other constructors. Then, on the app side, you should be able to get the serial of the device however you want, and pass it into the salt param so the app doesn't get "UNKNOWN" for the salt and crash the app for upgrading users.

[scottyab]

Hey, apologies for the radio silence I've got a stack of other things competing for my time. Please do branch and if you wish to create Pull Request and I'll review and hopefully merge/release. In fact I'm looking for others to help maintain this library so if people interested the way to start is a PR to improve/fix something. Thanks

[Goran1708]

Happening to me also.

[Goran1708]

Its ok if we drop compileSdkVersion from 28 to 27 and targetSdkVersion from 28 to 27. This is the safest fix until gets handled

[ryanw-mobile]

This happened to all my projects using this library, and it has been a disaster. For my case it looks like users reinstalling the App can't solve the problem, because old users got their App Data restored automatically as part of my system design. Very much appreciated to know if there's a way to get this patched if not downgrading the SdkVersion from 28 to 27.

[willswinson]

Downgrading to the sdk version 27 does not fix the issue for users who have already upgraded. It is not a full solution. Once it kicks in, you either have to screw the users who already upgraded (who will crash after you revert back to 27) or keep screwing the users who are in the process of upgrading.

I submitted a PR that will allow us to use a custom salt in the SecurePreferences constructor. Once the dude actually merges it (if he does, since he neglected this for so long), you should coordinate which salt to use so the app won't crash.

Remember that users who have already upgraded are now using the salt string "UNKNOWN", while users who haven't upgraded are using their Android device ID. You can google how to get the Android device ID, but remember that the method is DIFFERENT on Android Pie (which is why this crash is happening).

Note that I am only guessing this is the issue and have no actually fixed the issue in my app, but I'm pretty sure this is what's causing it.

I believe if you can figure out if they've already upgraded and are using "UNKNOWN" as the new salt (after reinstalling), then pass that into the salt param for the constructor.

If they haven't upgraded, prepare them by checking for the Android version. If it's Pie (9), then use the new method for getting the device ID (otherwise you will get UNKNOWN, causing the crash). If they haven't upgraded, continue using the old method (he uses Build.class.getField("SERIAL"), this is the "old" way).

Make sure to test if the new method gives you back the same exact device ID string though or it will crash again! I have not tested this myself yet.

But I'm with you, my app on Android has lost ~1 star in reviews because of this guy's library. I even e-mailed him long ago at his personal e-mail, and he's been completely ignoring it. Time to move on after this.

[Goran1708]

There is fix for this already here https://github.com/scottyab/secure-preferences/pull/67 which is merged in master, but I guess CI build is failing or what ever and because of that code is not deployed. @scottyab can you check why CI build is failing, adding salt manually should fix this problem.

Also FYI it should not make this backwards compatible, users who were getting "unknown" from getDeviceSerialNumber(context) will still be having problems, this either needs to migrate current prefs or delete the file and create new one.

[ryanw-mobile]

I noticed something strange probably dated back to early January, in the way that .getString(...) is returning null on some devices even I have supplied a default value.

I did not have the time to trace back to the source, so ended up I just added additional checking and assigned back the default value in case .getString(...) returns null. This is not perfect, but it is good enough to stop App Crash Reports from coming in with the least effort. Testings at my side shown that if the App writes a new value to the SharedPreference, the exception is not appearing again (now I bet that's because the library is using the "UNKNOWN" for salt when committing?)

Google Play now monitors the App crash rate and sends reminders to the developer if the crash rate is above a certain threshold, so stopping the crash has a higher priority over getting a perfect fix to me.

Then finally the java.lang.ClassCastException: s == null we discuss here is something I did not expect to happen - when we getInt(..) and it is returning a null which obviously a ClassCastException. Right now what I am doing to stop the crash as a quick workaround, is to catch this exception from all .getInt(..) calls, and assign back the default value.

From the comments above, it looks like additional work/migration is needed for a proper fix. Thanks guys for the pointers!

[willswinson]

@Goran1708 Why wouldn't passing "UNKNOWN" as the salt fix their issue? As I said, if they are using UNKNOWN as the salt already from reinstalling, just pass that as the salt and that should fix it for those users.

@scottyab Could you please merge these PRs? We need custom salts.

[Goran1708]

How do you know all users are getting "unknown" and what about those users that do not use API 26+ it will then be broken for them because they are getting serial number. This either needs some sort of migration or upon initialisation to check if we have correct salt and password, if not, recreate sharedPrefs file by our passed in values.

[willswinson]

@Goran1708

How do you know all users are getting "unknown"

Because that's what that method returns once you upgrade to Android Pie. So every user that has upgraded will be using "UNKNOWN" as their salt.

and what about those users that do not use API 26+ it will then be broken for them because they are getting serial number.

They are still using the serial number. So you can check if they are still using serial (meaning they haven't upgraded), and keep using the serial as the salt.

This either needs some sort of migration or upon initialisation to check if we have correct salt and password, if not, recreate sharedPrefs file by our passed in values.

It shouldn't need that. But it doesn't matter, because this guy is never going to merge anything. Doesn't seem to care that his library is completely fucking over peoples' apps.

[gi097]

See my attempt at PR #92. Note that you need to ask your user for the READ_PHONE_STATE permission within the app. This PR will use the "UNKNOWN" salt for decryption as fallback when decryption with the serial number didn't work.

You can test it using the following dependency:

implementation 'com.github.gi097:secure-preferences:d3042ed'

Besides that, I won't blame @scottyab as he already mentioned he has other things to do. I understand the frustration of you all, but it's wrong to be so mad at him.

[willswinson]

@gi097 I've had a PR out for almost 2 weeks that allows you to choose which salt you want with just the basic constructor. You can then handle the problem however you wish outside the framework. The issue is @scottyab won't merge any PR and won't respond to anything.

[ryanw-mobile]

Surely I know @scottyab can't be blamed for because he has been contributing for free, however would it be a way out if somebody else is willing to takeover this library and keep it going? It has been a very handy and useful library and just too bad to have an ending like this.

[scottyab]

Apologies for the lack of comms. I've merged your PR @willswinson0 for to allow custom salt. I'll tag and push this as ~0.1.5~ I mean 0.1.7