Android: Protect app from crashing due to unsafe input from 3rd parties

[cmonfortep]

Describe the bug
Our app can be launched from 3rd party apps, and we access an Intent which was created by the other app.

If the other app has a badly created Intent, or the calling app's process dies before we can process the Intent, it can cause our app to crash when trying to process it.

Other browsers have had to add a safety net around processing 3rd party intents previously

• Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1147992
• Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=469219

Potential fixes

• Firefox fixes by wrapping all incoming Intents into a SafeIntent, which has try/catches embedded in it to prevent crashes.
• Chrome runs all Intents through a sanitize method [preferred]
  
  
  
  • attempts to pull out an extra from the Intent.
  
  
  • the extra isn't there, but trying to get it will cause the exception to happen if the - Intent is malformed or invalid.
  
  
  • Returns an Intent with a nulled out extras bundle if exception is caught
  
  

Play Store:
Link to crashes
Number of users affected: low
Total occurrences: low

If applicable, please include information about any settings that may be relevant to this issue (e.g. selected theme, auto clear option, etc).

Expected behavior

• Protect against our app crashing due to problems with 3rd party apps

Smartphone (please complete the following information):

• DDG App Version: all versions.

[abdulbasit75]

I would like to work on this issue, please assign me.

[cmonfortep]

Sure @abdulbasit75, feel free to contribute on this issue. 👍

[code-bluebird]

Hi,
I would like to work on this issue, could somebody help me with these details.

• Which activity processes the received intent from 3rd parties ?
• and please provide any more details of error log if possible.

[cmonfortep]

This is the stacktrace of the error described above:

android.os.BadParcelableException: 
  at android.os.Parcel.readParcelableCreator (Parcel.java:3059)
  at android.os.Parcel.readParcelable (Parcel.java:2981)
  at android.os.Parcel.readValue (Parcel.java:2883)
  at android.os.Parcel.readArrayMapInternal (Parcel.java:3261)
  at android.os.BaseBundle.initializeFromParcelLocked (BaseBundle.java:292)
  at android.os.BaseBundle.unparcel (BaseBundle.java:236)
  at android.os.BaseBundle.getBoolean (BaseBundle.java:899)
  at android.content.Intent.getBooleanExtra (Intent.java:8415)
  at com.duckduckgo.app.browser.BrowserActivity.launchNewSearchOrQuery (BrowserActivity.kt:191)
  at com.duckduckgo.app.browser.BrowserActivity.access$launchNewSearchOrQuery (BrowserActivity.kt:59)
  at com.duckduckgo.app.browser.BrowserActivity$BrowserStateRenderer.showWebContent (BrowserActivity.kt:403)
  at com.duckduckgo.app.browser.BrowserActivity$BrowserStateRenderer.renderBrowserViewState (BrowserActivity.kt:384)
  at com.duckduckgo.app.browser.BrowserActivity$onCreate$1.onChanged (BrowserActivity.kt:111)
  at com.duckduckgo.app.browser.BrowserActivity$onCreate$1.onChanged (BrowserActivity.kt:59)
  at androidx.lifecycle.LiveData.considerNotify (LiveData.java:131)
  at androidx.lifecycle.LiveData.dispatchingValue (LiveData.java:149)
  at androidx.lifecycle.LiveData.setValue (LiveData.java:307)
  at androidx.lifecycle.MutableLiveData.setValue (MutableLiveData.java:50)
  at com.duckduckgo.app.browser.BrowserViewModel$dataClearingObserver$1.onChanged (BrowserViewModel.kt:103)
  at com.duckduckgo.app.browser.BrowserViewModel$dataClearingObserver$1.onChanged (BrowserViewModel.kt:50)
  at androidx.lifecycle.LiveData.considerNotify (LiveData.java:131)
  at androidx.lifecycle.LiveData.dispatchingValue (LiveData.java:149)
  at androidx.lifecycle.LiveData.setValue (LiveData.java:307)
  at androidx.lifecycle.MutableLiveData.setValue (MutableLiveData.java:50)
  at com.duckduckgo.app.fire.AutomaticDataClearer.clearDataWhenAppInForeground (AutomaticDataClearer.kt:171)
  at com.duckduckgo.app.fire.AutomaticDataClearer$clearDataWhenAppInForeground$1.invokeSuspend (Unknown Source:12)
  at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:33)
  at kotlinx.coroutines.UndispatchedCoroutine.afterResume (Builders.common.kt:207)
  at kotlinx.coroutines.AbstractCoroutine.resumeWith (AbstractCoroutine.kt:113)
  at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:46)
  at kotlinx.coroutines.DispatchedTask.run (DispatchedTask.kt:56)
  at android.os.Handler.handleCallback (Handler.java:883)
  at android.os.Handler.dispatchMessage (Handler.java:100)
  at android.os.Looper.loop (Looper.java:237)
  at android.app.ActivityThread.main (ActivityThread.java:7948)
  at java.lang.reflect.Method.invoke (Native Method)
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:493)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1075)

[tirthyme]

Any potential ways to regenerate this issue? I'd like to work on this.

[cmonfortep]

Sorry for the delay here.
In order to reproduce and test any contribution, I've just created a new android app (using blank activity sample from android studio), and from there trigger DDG browser with an invalid intent.

How to create an invalid intent: Just add inside the extras an object that doesn't exist in the target application, that will produce the crash.

data class FakeContent(val fakeString: String = "fake"): Serializable

val intent = Intent().apply {
    action = Intent.ACTION_MAIN
    addCategory(Intent.CATEGORY_BROWSABLE)
    component = ComponentName("com.duckduckgo.mobile.android.debug", "com.duckduckgo.app.browser.BrowserActivity")
    flags = Intent.FLAG_ACTIVITY_NEW_TASK
}
val bundle = Bundle()
bundle.putSerializable("SOMETHING_RANDOM", FakeContent())
intent.putExtras(bundle)
startActivity(intent)

This is based on Google's tracker issue comment:
"ClassNotFoundException when unmarshalling: com.facebook.graphql.model.GraphQLPrefetchInfo
Should we reach out to Facebook folks to see why they are adding this in their intents?"

[tirthyme]

@cmonfortep Is someone working on this issue? Because I already see an already existing PR with suggested changes. If not, I would like to work on this.

[cmonfortep]

Thanks @tirthyme, @wseemann submitted a PR that solved the problem partially, I assume the user will continue with the work there.

My previous comment was just about keeping this thread up to date.

[cmonfortep]

Closing this one fixed by #1101. Thanks for the contribution. We will merge soon #1163 that completes the loop.