[tor-bugs] #31144 [Applications/Tor Browser]: ESR68 Network Code Review
Tor Bug Tracker & Wiki
blackhole at torproject.org
Thu Oct 17 01:17:21 UTC 2019
#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
More information about the tor-bugs
mailing list