Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Fix automatic data reset on for users affected by encryption issues (EXPOSUREAPP-3313, EXPOSUREAPP-3335) #1433

Merged
merged 3 commits into from
Oct 21, 2020

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Oct 19, 2020

The automatic data reset for users affected by #642 does unfortunately not match ☹️.
We overlooked that KeyException is a subclass of GeneralSecurityException.
This causes us to abort early and continue with KeyException which then fails the error message matching test.
As the underlying Tink code throws the GeneralSecurityException first and should have no further causes, I've changed the code to not iterate, but directly pick the last exception in the list of causes.

Test

This can't be easily tested on device.
Understand the original exception flow in the stracktrace provided by a user and confirm that this is now fixed and the unit test correctly covers this case.

time: 1603116265174
msg: java.security.GeneralSecurityException: decryption failed
stacktrace: java.lang.RuntimeException: Unable to create application de.rki.coronawarnapp.CoronaWarnApplication: de.rki.coronawarnapp.exception.CwaSecurityException: something went wrong during a critical part of the application ensuring security, please referto the details for more information
	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6465)
	at android.app.ActivityThread.access$1300(ActivityThread.java:219)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1859)
	at android.os.Handler.dispatchMessage(Handler.java:107)
	at android.os.Looper.loop(Looper.java:214)
	at android.app.ActivityThread.main(ActivityThread.java:7356)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:491)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:925)
Caused by: de.rki.coronawarnapp.exception.CwaSecurityException: something went wrong during a critical part of the application ensuring security, please referto the details for more information
	at de.rki.coronawarnapp.util.security.SecurityHelper$encryptedPreferencesProvider$1.invoke(SecurityHelper.kt:9)
	at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:3)
	at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:6)
	at de.rki.coronawarnapp.util.security.SecurityHelper.getGlobalEncryptedSharedPreferencesInstance(Unknown Source:2)
	at de.rki.coronawarnapp.storage.LocalData.getSharedPreferenceInstance(LocalData.kt:1)
	at de.rki.coronawarnapp.CoronaWarnApplication.onCreate(CoronaWarnApplication.kt:42)
	at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1190)
	at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6460)
	... 8 more
Caused by: java.security.KeyException: Permantly failed to instantiate encrypted preferences
	at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory.create(EncryptedPreferencesFactory.kt:2)
	at de.rki.coronawarnapp.util.security.SecurityHelper$encryptedPreferencesProvider$1$1.invoke(SecurityHelper.kt:1)
	at de.rki.coronawarnapp.util.security.SecurityHelper$encryptedPreferencesProvider$1.invoke(SecurityHelper.kt:7)
	... 15 more
Caused by: java.lang.SecurityException: Could not decrypt key. decryption failed
	at androidx.security.crypto.EncryptedSharedPreferences.getAll(EncryptedSharedPreferences.java:13)
	at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory$create$1.invoke(EncryptedPreferencesFactory.kt:29)
	at de.rki.coronawarnapp.util.RetryMechanism.retryWithBackOff$default(RetryMechanism.kt:7)
	at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory.create(EncryptedPreferencesFactory.kt:1)
	... 17 more
Caused by: java.security.GeneralSecurityException: decryption failed
	at com.google.crypto.tink.daead.DeterministicAeadWrapper$WrappedDeterministicAead.decryptDeterministically(DeterministicAeadWrapper.java:16)
	at androidx.security.crypto.EncryptedSharedPreferences.getAll(EncryptedSharedPreferences.java:8)
	... 20 more

@d4rken d4rken added bug Something isn't working maintainers Tag pull requests created by maintainers do not merge labels Oct 19, 2020
@d4rken d4rken added this to the 1.6.0 milestone Oct 19, 2020
@d4rken d4rken requested a review from a team October 19, 2020 16:51
@d4rken
Copy link
Member Author

d4rken commented Oct 19, 2020

Marked as DO NOT MERGE as I want to leave the option open to rebase this against release/1.5.x if we decide that there will be a hotfix.

…eption`.

This causes us to continue with `KeyException` which then fails the message matching test.
As the underlying Tink code throws the GeneralSecurityException first, we check the type of the last exception.
They may not have read the dialog message completely or want to re-read it.
@d4rken d4rken changed the base branch from release/1.6.x to release/1.5.x October 20, 2020 14:32
@d4rken d4rken force-pushed the fix/3313-fix-encryption-reset-matching branch from 76d89dc to e4e2280 Compare October 20, 2020 14:32
@d4rken
Copy link
Member Author

d4rken commented Oct 20, 2020

I rebased this against 1.5.x.

I added an additional test case case and changed the explanation dialog behavior slightly.
It will no longer dismiss the dialog when the user clicks details and visits the FAQ.
It's possible that the user clicks "details" before reading everything or wants to read it again.
The dialog will now only dismiss when pressing "OK".

Before doing on device tests, please review the code and test cases again.
To test on device you can start by adding an exception like this to SecurityHelper:

        withSecurityCatch {
            try {
                factory.create(ENCRYPTED_SHARED_PREFERENCES_FILE)
// ################
                throw KeyException(
                    "Permantly failed to instantiate encrypted preferences",
                    SecurityException(
                        "Could not decrypt key. decryption failed",
                        GeneralSecurityException("decryption failed")
                    )
                )
// ################
            } catch (e: Exception) {
                if (encryptionErrorResetTool.tryResetIfNecessary(e)) {
                    Timber.w("We could recovery from this error via reset. Now retrying.")
                    factory.create(ENCRYPTED_SHARED_PREFERENCES_FILE)
                } else {
                    throw e
                }
            }
        }

@d4rken d4rken removed this from the 1.6.0 milestone Oct 20, 2020
@sonarcloud
Copy link

sonarcloud bot commented Oct 20, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

29.4% 29.4% Coverage
0.0% 0.0% Duplication

@d4rken d4rken changed the title Fix automatic data reset on for users affected by encryption issues (EXPOSUREAPP-3313) Fix automatic data reset on for users affected by encryption issues (EXPOSUREAPP-3313, EXPOSUREAPP-3335) Oct 20, 2020
@harambasicluka harambasicluka added this to the 1.5.1 milestone Oct 20, 2020
@harambasicluka
Copy link
Contributor

@BMItter will also review this PR, as it is not testable for our testers.

@harambasicluka harambasicluka requested a review from BMItr October 20, 2020 16:12
@BMItr
Copy link
Contributor

BMItr commented Oct 20, 2020

LGTM, also toast handling i.o, exception handling i.o

@ralfgehrer ralfgehrer merged commit 4a6e439 into release/1.5.x Oct 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cause 9002: file is not a database: , while compiling: select count(*) from sqlite_master;
4 participants