ESR68 Network Code Review
This ticket is to cover the network code review for ESR68
Activity
Ok, I'm wrapping this up. I have the following questions/observations first:
- ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?
- ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?
- ./toolkit/modules/secondscreen/RokuApp.jsm also makes connections.. ISTR disabling this? Is it off?
- For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).
- Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?
- I found a lot of instances where it looks like Android could use Intents to open external apps. Most of the obvious ones route through IntentHelper.openUriExternal() from ./mobile/android/base/java/org/mozilla/gecko/IntentHelper.java, which has some logic to show prompts in private browsing mode.. Do we set private browsing mode? Can users turn it off? Here's the files that call that function:
- ./mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/ActivityStreamContextMenu.java
- ./mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
- ./mobile/android/base/java/org/mozilla/gecko/ChromeCastDisplay.java
- ./mobile/android/base/java/org/mozilla/gecko/home/HomeFragment.java
- I have not dug through all of the Android code for all Intent usage.. Should I? Has anyone?
-
Trac:
FF68_NETWORK_AUDIT 
Replying to mikeperry:
Ok, I'm wrapping this up. I have the following questions/observations first:
- ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?
It seems we disabled that via pref (see: comment:7:ticket:18546 and comment:10:ticket:16222). We should make sure this still works.
- ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?
We have #18862 (moved) for that. Arthur checked back then that the prefs are disabling everything. They are still set to
falsefor desktop. We had #22165 (moved) as well which got upstreamed meanwhile.I am not exactly sure what we did when Tor Browser on Android entered the picture. sysrqb: did you look at that?
- ./toolkit/modules/secondscreen/RokuApp.jsm also makes connections.. ISTR disabling this? Is it off?
I think it is. We had #16439 (moved) for that and are not including
RokuApp.jsmandSimpleServiceDiscovery.jsmin our code (it's mobile only since https://bugzilla.mozilla.org/show_bug.cgi?id=1393582 landed and there even governed by a pref we set tofalse)- For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).
- Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?
I don't think so. I thought that would be a nice thing to do during the network code review.
I leave the (other) mobile questions to sysrqb.
-
Replying to gk:
Replying to mikeperry:
Ok, I'm wrapping this up. I have the following questions/observations first:
- ./devtools/shared/discovery/discovery.js uses UDP multicast for debugger discovery. This should only be local network, but maybe we should disable it anyway. Do we?
It seems we disabled that via pref (see: comment:7:ticket:18546 and comment:10:ticket:16222). We should make sure this still works.
- ./dom/presentation/PresentationTCPSessionTransport.cpp seems to use TCP for app-to-app communication. Do we disable the DOM presentation stuff?
We have #18862 (moved) for that Arthur checked back then that the prefs are disabling everything. They are still set to
falsefor desktop. We had #22165 (moved) as well which got upstreamed meanwhile.I am not exactly sure what we did when Tor Browser on Android entered the picture. sysrqb: did you look at that?
It seems we flipped the prefs there explicitly, so we should be good, I guess. sysrqb: I guess that's something we can move to the non-mobile specific prefs file as that beast should be disabled for all platforms.
-
- Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?
I don't think so. I thought that would be a nice thing to do during the network code review.
Hrmm I don't have a TBB build environment anymore... Maybe we can just ask Ritter if he's run it recently/to rerun it.
I am going to fall into this Intent rabbithole instead for now.
-
Ok I believe I have completed the android portion now. Here is the full list of found items, removing the ones GeKo said were fixed:
- Rust lib check
- java.net.URL stream usage (which looks like it bypasses the proxy)
- GeckoApplication.downloadImageForSetImage uses URL.openStream()
- GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream()
- GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?)
- GeckoMediaDrmBridgeV21.java - uses android.media.MediaDrm which seems to fetch stuff??
- BitmapUtils.decodeUrl uses openStream for non-jar urls
- GeckoJarReader - tons of stream use.. Can this be used on remote jars?
- AbstractCommunicator.openConnectionAndSetHeaders() - uses url.openConnection() (I think we patched this one in #31934 (moved)?)
- AbstractCommunicator.sendData() - uses url.getOutputStream().. maybe ok?
- IntentHelper openUriExternal usage - maybe we should just patch this to always prompt?
- ActivityStreamContextMenu.java
- BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)
- ChromeCastDisplay.java
- HomeFragment.java
- 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?
- CustomTabsActivity.java - Several methods emit potentially external Intents
- WebAppActivity.onLoadRequest()
- BasicGeckoViewPrompt.onFilePrompt()
- GeckoViewActivity.onExternalResponse()
- Intent bindService() usage:
- SurfaceAllocator - no idea what is happening here :/
- RemoteManager - no idea what is happening here :/
- android.app.PendingIntent
- ChromeCastDisplay.java - probably want to make sure this is disabled?
- CustomTabsActivity.performPendingIntent - again, hard to tell what is happening here
- 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
I committed a rubric of what I did for future audits/tooling here: https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/NETWORK_AUDIT_RUBRIC
I also committed my notes here: https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF68_NETWORK_AUDIT
Trac:
Status: new to needs_review 
Replying to mikeperry:
- For Rust, I found sendmsg and recvmsg only in mio and audioipc. I think this is fine? (I am asking about those two because Ritter's tool whitelisted them and I wanna double check).
That matches my finding from https://bugzilla.mozilla.org/show_bug.cgi?id=1376621#c23
- Otherwise has Ritter's network symbol tool been run on FF68ESR for Rust?
It runs on all builds in both esr68 and central; however in central several critical functions got allowlisted so it's not very effective anymore.
And when I say 'runs on all builds' I mean if Tor added a rust patch that used a networking function, this check would break the (firefox) build (during that step of the tor browser build process).
Replying to mikeperry:
Ok I believe I have completed the android portion now. Here is the full list of found items, removing the ones GeKo said were fixed:
- Rust lib check
- java.net.URL stream usage (which looks like it bypasses the proxy)
- GeckoApplication.downloadImageForSetImage uses URL.openStream()
I don't know why we missed this. We should've found this in #21863 (moved).
- GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream()
Same here.
- GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?)
This seems like it's only local connections (but it has the potential of bypassing the proxy). Specifically, this is used as a protocol handler for
android:URIs. We can reject connections for now, and come back to this.- GeckoMediaDrmBridgeV21.java - uses android.media.MediaDrm which seems to fetch stuff??
We disabled DRM by pref (ticket:16285#comment:34).
- BitmapUtils.decodeUrl uses openStream for non-jar urls
Sigh.
- GeckoJarReader - tons of stream use.. Can this be used on remote jars?
This is safe, only accepts valid file paths.
- AbstractCommunicator.openConnectionAndSetHeaders() - uses url.openConnection() (I think we patched this one in #31934 (moved)?)
- AbstractCommunicator.sendData() - uses url.getOutputStream().. maybe ok?
Only used in mozstumbler, which we exclude at compile time.
(coming back for the rest.)
(continuing...)
Replying to mikeperry:
- IntentHelper openUriExternal usage - maybe we should just patch this to always prompt?
This seems like the easiest solution. I'll add that.
- ActivityStreamContextMenu.java
Caught by forcing above prompt.
- BrowserApp.java (see also onNewIntent() delegation to BrowserAppDelegates list)
Can you provide a link for this? I'm missing it somehow.
- ChromeCastDisplay.java
We unset
MOZ_NATIVE_DEVICESwhich excludes this.- HomeFragment.java
Caught by forcing above prompt.
- 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.
- CustomTabsActivity.java - Several methods emit potentially external Intents
My only concern here is
onLoadRequest()when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.- WebAppActivity.onLoadRequest()
WebActivities can't be installed from PBM (which is the new default tab mode). There will be a weird situation at the next upgrade, because WebApps worked in the current stable due to normal tabs being usable. In the next release, only private tabs will be used (by default). I don't know what will happen if a webapp is loaded in PBM, it seems like they will still work.
- BasicGeckoViewPrompt.onFilePrompt()
- GeckoViewActivity.onExternalResponse()
This is not part of the app (geckoview_example). We should patch these in the future.
- Intent bindService() usage:
- SurfaceAllocator - no idea what is happening here :/
Connecting the app to a background service.
- RemoteManager - no idea what is happening here :/
Needs
MediaManagerwhich is excluded.- 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.
- 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_managerwhich 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.-
Replying to sysrqb:
(continuing...)
Replying to 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.
- 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.
- 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?
- 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_managerwhich 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.
Replying to mikeperry:
Replying to sysrqb:
(continuing...)
Replying to 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).
- 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
showPromptInPrivateBrowsingastrueinExternalIntentDuringPrivateBrowsingPromptFragment.showDialogOrAndroidChooser.- 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.
- 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_managerwhich 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
DownloadManageris used in two places:BrowserApp.javaandDownloadsIntegration.java.In
BrowserApp.java, DownloadManager is only called when a message is dispatched with theDownload:AndroidDownloadManagerevent. This only happens inmobile/android/components/HelperAppDialog.js:_downloadWithAndroidDownloadManager()which is only called ifthis._shouldForwardToAndroidDownloadManager() === true. That returns false if the prefbrowser.download.forward_oma_android_download_managerisfalse, so we're good here.In
DownloadsIntegration.java,DownloadManageris used three times. Each of them is either within a conditional block dependent onuseSystemDownloadManager(), or returns early. InuseSystemDownloadManager, it returnsfalseifAppConstants.ANDROID_DOWNLOADS_INTEGRATION. We setANDROID_DOWNLOADS_INTEGRATIONasFalseat configure time.I pushed
bug31144_04which contains fixups for the missed:GeckAppShell has many wrappers to create inputstreams from URLConnections (but these may need to be opened first?) This seems like it's only local connections (but it has the potential of bypassing the proxy). Specifically, this is used as a protocol handler for android: URIs. We can reject connections for now, and come back to this.
and
GeckoActionProvider.downloadImageForIntent uses java.net.URL.openStream() Same here.
and
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.
and
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.
CustomTabsActivity.java - Several methods emit potentially external Intents My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.
This is included in the patch for #26529 (moved), but in this case we don't prompt. #26529 (moved) seemed like the better commit, but I can move this into its own commit.
diff --git a/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java b/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java index c75962da35a7..438f462755f6 100644 --- a/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java +++ b/mobile/android/base/java/org/mozilla/gecko/customtabs/CustomTabsActivity.java @@ -618,7 +618,8 @@ public class CustomTabsActivity extends AppCompatActivity final Intent intent = new Intent(Intent.ACTION_VIEW); intent.setData(uri); try { - startActivity(intent); + // Bug 31144 - Don't know how to handle this case. + //startActivity(intent); } catch (ActivityNotFoundException e) { Log.w(LOGTAG, "No activity handler found for: " + request.uri); }Replying to sysrqb:
CustomTabsActivity.java - Several methods emit potentially external Intents My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.
This is included in the patch for #26529 (moved), but in this case we don't prompt. #26529 (moved) seemed like the better commit, but I can move this into its own commit.
I pushed
bug31144_05with only this patch on top of68.1.0esr-9.0-3.-
Replying to sysrqb:
Replying to sysrqb:
CustomTabsActivity.java - Several methods emit potentially external Intents My only concern here is onLoadRequest() when the scheme isn't handled by the browser. That's the only place where the user wasn't prompted. I don't know how the customtabs UI should handle this situation. We can break this functionality for now, until we find how this should be done correctly.
This is included in the patch for #26529 (moved), but in this case we don't prompt. #26529 (moved) seemed like the better commit, but I can move this into its own commit.
I pushed
bug31144_05with only this patch on top of68.1.0esr-9.0-3.Thanks. I cherry-picked that one on top of
tor-bŕowser-68.1.0esr-9.0-3(commit d5123da897516717e62db98dccbcabd70162dcb3).The other commits look good to me and I picked them onto
tor-browser-68.1.0esr-9.0-3(commit 947d477eb9289ebcdbbf48ee77609a6d7f395785, 47e35494838222732b9eb9638fb40e7a9707b307, 55b6cd7f3139dd0069ba2c69ec6d08da038594ad, dfdef56ec12ace37d08e2df516430427d0e08d7a, and db19bbcd9c34c933ad7fc84693ed8a5ad3bac3cf).What is missing here is the patch for #26529 (moved) which I'll take care in that bug. We are done here. \o/
mikeperrry, sysrqb: please add your points.
Trac:
Status: needs_review to closed
Resolution: N/A to fixed mentioned in issue #31591 (moved)
