#31144: ESR68 Network Code Review -------------------------------------------------+------------------------- Reporter: pili | Owner: tbb- | team Type: task | Status: | needs_review Priority: Very High | Milestone: Component: Applications/Tor Browser | Version: Severity: Normal | Resolution: Keywords: TorBrowserTeam201910R, tbb-9.0 | Actual Points: -alpha-must | Parent ID: | Points: 10 Reviewer: | Sponsor: -------------------------------------------------+-------------------------
Comment (by sysrqb): Replying to [comment:16 mikeperry]: > Replying to [comment:15 sysrqb]: > > (continuing...) > > > > Replying to [comment:11 mikeperry]: > > > > - BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list) > > > > Can you provide a link for this? I'm missing it somehow. > > In ./mobile/android/base/java/org/mozilla/gecko/BrowserApp.java search for BrowserAppDelegate (sorry, no s). That type is used to create a list of things that have Activity related things passed to them. I think it might be harmless though. Okay, I saw that. Yeah, that is harmless. I don't see any of those delegates implementing onNewIntent() (overriding the empty implementation they inherit). > > > > 4. android.content.Intent startActivity() usage (may or may not be unsafe depending on circumstance :/) > > > - ActivityHandlerHelper - Good candidate to patch for external activities, but not everything uses it :/ > > > - BrowserApp.onUrlOpenWithRefferer () - Might be able to launch other apps if OPEN_WITH_INTENT flag is set? > > > > Caught by forcing above prompt. > > Wait, both of these call startActivity() directly with an intent. Forcing the prompy from IntentHelper will NOT catch these. > > If ActivityHandlerHelper was patched to call into IntentHelper (or add its own prompt), then all the things that us it would prompt, but BrowserApp doesn't use either of the Helper classes to handle its Intents. Sorry, I should've been more descriptive. I'm patching them like: {{{ iff --git a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java index 5d6f725dc1c5..ab19b78d54e7 100644 --- a/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java +++ b/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java @@ -175,6 +175,7 @@ import org.mozilla.gecko.util.WindowUtil; import org.mozilla.gecko.widget.ActionModePresenter; import org.mozilla.gecko.widget.AnchoredPopup; import org.mozilla.gecko.widget.AnimatedProgressBar; +import org.mozilla.gecko.widget.ExternalIntentDuringPrivateBrowsingPromptFragment; import org.mozilla.gecko.widget.GeckoActionProvider; import org.mozilla.gecko.widget.SplashScreen; import org.mozilla.geckoview.DynamicToolbarAnimator; @@ -2237,7 +2238,8 @@ public class BrowserApp extends GeckoApp if (AppConstants.RELEASE_OR_BETA) { Intent intent = new Intent(Intent.ACTION_VIEW); intent.setData(Uri.parse("market://details?id=" + getPackageName())); - startActivity(intent); + ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser( + this, getSupportFragmentManager(), intent); break; } @@ -4195,7 +4197,8 @@ public class BrowserApp extends GeckoApp if (flags.contains(OnUrlOpenListener.Flags.OPEN_WITH_INTENT)) { Intent intent = new Intent(Intent.ACTION_VIEW); intent.setData(Uri.parse(url)); - startActivity(intent); + ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser( + this, getSupportFragmentManager(), intent); } else { }}} And I'm forcing `showPromptInPrivateBrowsing` as `true` in `ExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser`. > > > > 6. android.app.PendingIntent > > > - ChromeCastDisplay.java - probably want to make sure this is disabled? > > > > Disabled. > > > > > - CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here > > > > These seem like they could be arbitrary actions. > > Hrmm.. should we patch that somehow, or assume it is handled when the Intent is finally delivered? > I'd rather break this functionality at this point. Someone can change the default browser on their device to Tor Browser under the assumption that CustomTabs are proxy-safe and other apps will happily use it. I'd rather be safe than sorry here. We can come back later and fix it correctly. > > > 7. android.app.DownloadManager > > > - DownloadsIntegration.java uses it, but has a check for useSystemDownloadManager() to avoid using it > > > - BrowserApp.java uses it to download items without any checks > > > > > > > This is controlled by `browser.download.forward_oma_android_download_manager` which is false. (https://bugzilla.mozilla.org/show_bug.cgi?id=1253684 which is restricted?). I'll add this into the override file, just so we aren't surprised by a change later. > > Are we sure that pref governs both usages of the download mamager? I did not see any checks in the BrowserApp itself. Yes, I believe we're safe here (no, it doesn't govern both uses). The `DownloadManager` is used in two places: `BrowserApp.java` and `DownloadsIntegration.java`. In `BrowserApp.java`, DownloadManager is only called when a message is dispatched with the `Download:AndroidDownloadManager` event. This only happens in `mobile/android/components/HelperAppDialog.js`:`_downloadWithAndroidDownloadManager()` which is only called if `this._shouldForwardToAndroidDownloadManager() === true`. That returns false if the pref `browser.download.forward_oma_android_download_manager` is `false`, so we're good here. In `DownloadsIntegration.java`, `DownloadManager` is used three times. Each of them is either within a conditional block dependent on `useSystemDownloadManager()`, or returns early. In `useSystemDownloadManager`, it returns `false` if `!AppConstants.ANDROID_DOWNLOADS_INTEGRATION`. We set `ANDROID_DOWNLOADS_INTEGRATION` as `False` at [https://gitweb.torproject.org/tor- browser.git/tree/mobile/android/torbrowser.configure?h=tor- browser-68.1.0esr-9.0-3#n14 configure time]. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/31144#comment:17> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs