Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Encrypt some preferences #576

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Encrypt some preferences #576

wants to merge 18 commits into from

Conversation

oakkitten
Copy link
Collaborator

@oakkitten oakkitten commented Feb 18, 2024

Closes #574. This is based on top of #575. See individual commit messages. Suggest reviewing commit by commit.

TODO: Add include rules for backups. I suppose we only want to backup preferences, custom fonts and themes.

@oakkitten oakkitten requested review from mhoran and zopieux February 18, 2024 14:05
@oakkitten oakkitten force-pushed the encrypt-passwords branch from ba5f9c1 to f49f919 Compare March 8, 2024 18:36
@oakkitten oakkitten marked this pull request as ready for review March 8, 2024 18:44
From AGP release notes:

  Starting with Android Gradle Plugin 8.4, if an Android library project is
  minified, shrunk program classes will be published for inter-project
  publishing. This means that if an app depends on the shrunk version of
  the Android library subprojects, the APK will include shrunk Android
  library classes. You may need to adjust library keep rules in case there
  are missing classes in the APK.

This leads to various R8 errors when building. The solution would be simply
to not minify the library itself as the app is going to be minified anyway.
This helps to avoid the error:

    Inconsistent JVM-target compatibility detected for tasks
    'compileJava' (21) and 'compileKotlin' (17).

See also 28fbdfc
These have been added ages ago and everything works without them ¯\_(ツ)_/¯
Error from Actions CI:

  This request has been automatically failed because it uses a deprecated
  version of `actions/upload-artifact: v2`. Learn more:
  https://github.blog/changelog/2024-02-13-deprecation-notice-v1-and-v2-of-the-artifact-actions/
Not using fast-forward as some commits in the branch do not build.
The idea is as such: create a custom version of SharedPreferences that
treats preference keys prefixed with `encrypted` differently, saving them
to an instance of EncryptedSharedPreferences.

To actually use this MultiSharedPreferences in a PreferenceFragmentCompat,
we have to employ a little hack of writing its private field, as for some
reason the fragment does not allow customizing it's shared preferences.

# Conflicts:
#	relay/build.gradle.kts
If we have a bad master key, or the encrypted file can't be decrypted using
it, completely reset encrypted shared preferences.

It is quite possible that we will never see this in action.

# Conflicts:
#	app/src/main/java/com/ubergeek42/WeechatAndroid/utils/MultiSharedPreferences.kt
This moves `applicationContext` to the `utils` package.

Additionally, it is mirrored in the tests to allow late initialization.
This will be used in the following commit.

This is highly experimental.
I have no idea what I am doing.
Roboelectric is used to simplify testing with application context and
platform APIs. It provides these things without running on a real device.

We are using JUnit 5. Roboelectric needs JUnit 4; to make this work,
we use org.junit.vintage:junit-vintage-engine.

For in-memory implementation of shared preferences,
we use io.github.ivanshafran:shared-preferences-mock.

Please ignore this:
  testImplementation("androidx.test:runner:1.5.2")
  testImplementation("org.mockito:mockito-core:5.8.0")
I would prefer to only use the `include` rules, but this is quite hard when
it comes to shared preferences. The problem is that the default shared
preferences file name contains the application id. While it is possible to
use application id placeholder in the manifest, it is not currently possible
in the resource files. It is also apparently not possible to use string
resources, it just does not work. We could also use a Gradle plugin that
gives us placeholder replacements, but Android Stem seems to be only parsing
string resources, which is neat but doesn't fit our use. The only feasible
way here would be duplicating the resources for every build variant.

Rules for API ≤ 30 can be tested with the following Bash script,
which is a modified version of the script suitable for > 30 found here:
at https://developer.android.com/guide/topics/data/testingbackup

    #!/bin/bash -eu
    : "${1?"Usage: $0 package name"}"

    adb shell bmgr enable true

    # $ adb shell bmgr list transports
    #     android/com.android.internal.backup.LocalTransport
    #     com.google.android.gms/.backup.BackupTransportService
    adb shell bmgr transport android/com.android.internal.backup.LocalTransport | grep -q "Selected transport" || (echo "Error: error selecting local transport"; exit 1)

    adb shell settings put secure backup_local_transport_parameters 'is_encrypted=true'
    adb shell bmgr backupnow "$1" | grep -F "Package $1 with result: Success" || (echo "Backup failed"; exit 1)

    apk_path_list=$(adb shell pm path "$1")
    OIFS=$IFS
    IFS=$'\n'
    apk_number=0
    for apk_line in $apk_path_list
    do
        (( ++apk_number ))
        apk_path=${apk_line:8:1000}
        # MSYS_NO_PATHCONV=1 allows running this script in Git Bash
        # Copy before pulling to avoid “remote object ... does not exist” errors
        # See this answer josemigallas https://stackoverflow.com/questions/41150038/#43157127
        MSYS_NO_PATHCONV=1 adb shell cp "$apk_path" "/storage/emulated/0/Download/temp_base.apk"
        MSYS_NO_PATHCONV=1 adb pull "/storage/emulated/0/Download/temp_base.apk" "myapk${apk_number}.apk"
    done
    IFS=$OIFS
    adb shell pm uninstall --user 0 "$1"
    apks=$(seq -f 'myapk%.f.apk' 1 $apk_number)

    adb install -t "$apks"

    adb shell bmgr transport com.google.android.gms/.backup.BackupTransportService
    rm $apks

    echo "Done"
@oakkitten
Copy link
Collaborator Author

I still want to do this despite https://www.reddit.ecom/r/androiddev/comments/1cj3krj/jetsec_crypto_is_now_deprecated/, and, more importantly, despite https://issuetracker.google.com/issues/176215143.

As for the deprecation, ESP is a tiny wrapper around SP and tink, and we can just continue updating tink.

As for the latter, it seems that the issue has 3 major sources:

  • Creating ESP from multiple threads — we don't do that.
  • Having data that is undecryptable by the master key. This seems to only arise when we somehow end up with leftover data from a backup of the app. With our backup rules this shouldn't happen, and if it does anyway, in 4e83c22 I added a way to survive this situation. This should not be an issue for us as we want to lose that data in this case.
  • Having a corrupted master key. It is not clear how the corruption might occur; some users were mentioning older devices running custom Android ROMS with higher versions of Android. Either way, we were using AKS with big success for quite some time now, so there's a high chance that we don't have any users with low quality AKS's.

Finally, there just seems to be no alternative. We would still want to go the same basic way, using some sort of a secret in AKS to encrypt data.

@zopieux
Copy link
Collaborator

zopieux commented Nov 30, 2024

That's… a lot. 🙃 Would it possible to have the version bumps and build cleanups as a separate PR (without the weird merge commit) so this code is just about the encryption?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Encrypt, don't back up passwords & keys
2 participants