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

Wrap EncryptedSharedPreferences initialization in a retry mechanism (EXPOSUREAPP-1851) #1235

Merged
merged 9 commits into from
Sep 28, 2020

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Sep 25, 2020

New PR to merge into 1.4.x instead of 1.5.x

Description

Wrapped the EncryptedSharedPreferences initialization in an exponential backoff mechanism with randomized delay, to mitigate issues caused by the system's keystore being busy. The idea is that after a few retries the congestion has cleared and we can get access to the system's keystore.

We'll retry for a maximum of 15s, delay at least 25ms and at most 3s between tries.

Should mitigate #642, possibly also #1214, #1121.

Bug description:

How to test

  • Difficult to reproduce ☹️
  • Understand the bug as described above (see links), review the code.
  • Add a test exception in the preference creation code, observe behavior.
Example log output
2020-09-23 00:34:12.839 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=1, totalDelay=0, lastDelay=0, exception=null)
2020-09-23 00:34:12.839 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.138 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=2, totalDelay=25, lastDelay=25, exception=java.security.KeyException)
2020-09-23 00:34:13.139 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.255 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=3, totalDelay=50, lastDelay=25, exception=java.security.KeyException)
2020-09-23 00:34:13.255 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.368 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=4, totalDelay=75, lastDelay=25, exception=java.security.KeyException)
2020-09-23 00:34:13.368 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.476 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=5, totalDelay=100, lastDelay=25, exception=java.security.KeyException)
2020-09-23 00:34:13.477 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.604 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=6, totalDelay=144, lastDelay=44, exception=java.security.KeyException)
2020-09-23 00:34:13.604 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.769 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=7, totalDelay=208, lastDelay=64, exception=java.security.KeyException)
2020-09-23 00:34:13.769 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:13.976 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=8, totalDelay=331, lastDelay=123, exception=java.security.KeyException)
2020-09-23 00:34:13.977 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:14.231 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=9, totalDelay=490, lastDelay=159, exception=java.security.KeyException)
2020-09-23 00:34:14.231 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:14.669 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=10, totalDelay=849, lastDelay=359, exception=java.security.KeyException)
2020-09-23 00:34:14.670 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:15.554 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=11, totalDelay=1655, lastDelay=806, exception=java.security.KeyException)
2020-09-23 00:34:15.554 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:18.114 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=12, totalDelay=4136, lastDelay=2481, exception=java.security.KeyException)
2020-09-23 00:34:18.115 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:20.963 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=13, totalDelay=6775, lastDelay=2639, exception=java.security.KeyException)
2020-09-23 00:34:20.964 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:24.171 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=14, totalDelay=9775, lastDelay=3000, exception=java.security.KeyException)
2020-09-23 00:34:24.172 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:27.382 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=15, totalDelay=12775, lastDelay=3000, exception=java.security.KeyException)
2020-09-23 00:34:27.382 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:30.534 4424-4424/de.rki.coronawarnapp.dev V/RetryMechanism: Executing attempt: Attempt(count=16, totalDelay=15775, lastDelay=3000, exception=java.security.KeyException)
2020-09-23 00:34:30.535 4424-4424/de.rki.coronawarnapp.dev D/EncryptedPreferencesFactory$create: Creating EncryptedSharedPreferences instance.
2020-09-23 00:34:30.778 4424-4424/de.rki.coronawarnapp.dev W/RetryMechanism$createDelayCalculator: Max retry duration exceeded.
2020-09-23 00:34:30.780 4424-4424/de.rki.coronawarnapp.dev W/RetryMechanism: Retrycondition exceeded: Attempt(count=16, totalDelay=15775, lastDelay=3000, exception=java.security.KeyException)
2020-09-23 00:34:30.782 4424-4424/de.rki.coronawarnapp.dev D/AndroidRuntime: Shutting down VM
2020-09-23 00:34:30.785 4424-4424/de.rki.coronawarnapp.dev E/AndroidRuntime: FATAL EXCEPTION: main
    Process: de.rki.coronawarnapp.dev, PID: 4424
    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:6717)
        at android.app.ActivityThread.access$1300(ActivityThread.java:237)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913)
        at android.os.Handler.dispatchMessage(Handler.java:106)
        at android.os.Looper.loop(Looper.java:223)
        at android.app.ActivityThread.main(ActivityThread.java:7656)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947)
     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.withSecurityCatch(SecurityHelper.kt:97)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:41)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:37)
        at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74)
        at de.rki.coronawarnapp.util.security.SecurityHelper.getGlobalEncryptedSharedPreferencesInstance(Unknown Source:2)
        at de.rki.coronawarnapp.storage.LocalData.getSharedPreferenceInstance(LocalData.kt:715)
        at de.rki.coronawarnapp.storage.LocalData.backgroundNotification(LocalData.kt:706)
        at de.rki.coronawarnapp.worker.BackgroundWorkHelper.sendDebugNotification(BackgroundWorkHelper.kt:95)
        at de.rki.coronawarnapp.CoronaWarnApplication.onCreate(CoronaWarnApplication.kt:99)
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1192)
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6712)
        at android.app.ActivityThread.access$1300(ActivityThread.java:237) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:223) 
        at android.app.ActivityThread.main(ActivityThread.java:7656) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947Caused by: java.security.KeyException: Permantly failed to instantiate encrypted preferences
        at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory.create(EncryptedPreferencesFactory.kt:39)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2$1.invoke(SecurityHelper.kt:42)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2$1.invoke(SecurityHelper.kt:37)
        at de.rki.coronawarnapp.util.security.SecurityHelper.withSecurityCatch(SecurityHelper.kt:95)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:41) 
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:37) 
        at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74) 
        at de.rki.coronawarnapp.util.security.SecurityHelper.getGlobalEncryptedSharedPreferencesInstance(Unknown Source:2) 
        at de.rki.coronawarnapp.storage.LocalData.getSharedPreferenceInstance(LocalData.kt:715) 
        at de.rki.coronawarnapp.storage.LocalData.backgroundNotification(LocalData.kt:706) 
        at de.rki.coronawarnapp.worker.BackgroundWorkHelper.sendDebugNotification(BackgroundWorkHelper.kt:95) 
        at de.rki.coronawarnapp.CoronaWarnApplication.onCreate(CoronaWarnApplication.kt:99) 
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1192) 
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6712) 
        at android.app.ActivityThread.access$1300(ActivityThread.java:237) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:223) 
        at android.app.ActivityThread.main(ActivityThread.java:7656) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:947Caused by: java.security.KeyException
        at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory$create$1.invoke(EncryptedPreferencesFactory.kt:34)
        at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory$create$1.invoke(EncryptedPreferencesFactory.kt:14)
        at de.rki.coronawarnapp.util.RetryMechanism.retryWithBackOff(RetryMechanism.kt:19)
        at de.rki.coronawarnapp.util.RetryMechanism.retryWithBackOff$default(RetryMechanism.kt:12)
        at de.rki.coronawarnapp.util.security.EncryptedPreferencesFactory.create(EncryptedPreferencesFactory.kt:31)
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2$1.invoke(SecurityHelper.kt:42) 
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2$1.invoke(SecurityHelper.kt:37) 
        at de.rki.coronawarnapp.util.security.SecurityHelper.withSecurityCatch(SecurityHelper.kt:95) 
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:41) 
        at de.rki.coronawarnapp.util.security.SecurityHelper$globalEncryptedSharedPreferencesInstance$2.invoke(SecurityHelper.kt:37) 
        at kotlin.SynchronizedLazyImpl.getValue(LazyJVM.kt:74) 
        at de.rki.coronawarnapp.util.security.SecurityHelper.getGlobalEncryptedSharedPreferencesInstance(Unknown Source:2) 
        at de.rki.coronawarnapp.storage.LocalData.getSharedPreferenceInstance(LocalData.kt:715) 
        at de.rki.coronawarnapp.storage.LocalData.backgroundNotification(LocalData.kt:706) 
        at de.rki.coronawarnapp.worker.BackgroundWorkHelper.sendDebugNotification(BackgroundWorkHelper.kt:95) 
        at de.rki.coronawarnapp.CoronaWarnApplication.onCreate(CoronaWarnApplication.kt:99) 
        at android.app.Instrumentation.callApplicationOnCreate(Instrumentation.java:1192) 
        at android.app.ActivityThread.handleBindApplication(ActivityThread.java:6712) 
        at android.app.ActivityThread.access$1300(ActivityThread.java:237) 
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1913) 
        at android.os.Handler.dispatchMessage(Handler.java:106) 
        at android.os.Looper.loop(Looper.java:223) 
        at android.app.ActivityThread.main(ActivityThread.java:7656) 
        at java.lang.reflect.Method.invoke(Native Method) 
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:592) 
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:9472020-09-23 00:34:30.823 4424-4424/de.rki.coronawarnapp.dev I/Process: Sending signal. PID: 4424 SIG: 9

@d4rken d4rken added bug Something isn't working maintainers Tag pull requests created by maintainers 1.4.0 labels Sep 25, 2020
@d4rken d4rken requested a review from a team September 25, 2020 14:17
@vaubaehn
Copy link
Contributor

New PR to merge into 1.4.x instead of 1.5.x

very good decision.

@vaubaehn
Copy link
Contributor

vaubaehn commented Sep 26, 2020

Hi @d4rken ,
nice to see that mitigation more soon.
However, from your dev's perspective, do you think it makes sense to provide support to users who run into app crash by linking from a new entry in FAQ to a guide provided here => #1053 (comment) until that mitigation is deployed? If no, why?

And related to #642 (comment) : Chapeau! Looks like a very thorough and nice research and analysis.

[preceeding comment accidently pinged wrong person. Sorry for that.]

@d4rken
Copy link
Member Author

d4rken commented Sep 28, 2020

However, from your dev's perspective, do you think it makes sense to provide support to users who run into app crash by linking from a new entry in FAQ to a guide provided here => #1053 (comment) until that mitigation is deployed? If no, why?

It will probably make sense to have that FAQ entry even after the mitigation is deployed. My fix will only prevent it from affecting new users. There is no recovery for anyone already affected by this as the original encryption key is lost. The only thing that could be done for existing users affected by this issue, is to possibly automate the recovery procedure. Simply resetting the necessary data is not enough though, we don't want to surprise the user with suddenly "reset" app, so there would have to be additional UI components to inform the user about what happened too. 🤔

}
}

private const val DEFAULT_TOTAL_MAX_RETRY = 15 * 1000L // 15 seconds total delay
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but shouldn't constants be on top of the class

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. For classes they should be in the companion object which are supposed to be at the end of the class according to KotlinStyleGuide. Here this is an object already. I placed it where it is so it is close to where it is used. I would go with your suggestion if it were used in other locations too. Here it seems beneficial to view it all without scrolling, what do you think?

Copy link
Contributor

@AlexanderAlferov AlexanderAlferov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All testes passed, no formatting issues, smoke test passed.

@vaubaehn
Copy link
Contributor

vaubaehn commented Sep 28, 2020

However, from your dev's perspective, do you think it makes sense to provide support to users who run into app crash by linking from a new entry in FAQ to a guide provided here => #1053 (comment) until that mitigation is deployed? If no, why?

It will probably make sense to have that FAQ entry even after the mitigation is deployed. My fix will only prevent it from affecting new users. There is no recovery for anyone already affected by this as the original encryption key is lost. The only thing that could be done for existing users affected by this issue, is to possibly automate the recovery procedure. Simply resetting the necessary data is not enough though, we don't want to surprise the user with suddenly "reset" app, so there would have to be additional UI components to inform the user about what happened too. thinking

Hi @d4rken , thanks very much for your response.
FAQ-entry is online now.
Automatic recovery would be nice for sure, but I'm copying your reservations according necessary UI additions.
Both thumbs pressed for your fix being effective in almost all cases!
Have a good week.

@sonarqubecloud
Copy link

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

59.2% 59.2% Coverage
0.0% 0.0% Duplication

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.

5 participants