Opened 4 years ago

Closed 4 years ago

#16222 closed task (fixed)

Review networking code for Firefox 38

Reported by: mikeperry Owned by: mikeperry
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff38-esr, tbb-5.0a3-essential, TorBrowserTeam201506R, MikePerry201506, GeorgKoppen201506
Cc: brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I need to do the usual networking system call review for FF38.

Along the way, #16221 is worth investigating.

Child Tickets

Change History (12)

comment:1 Changed 4 years ago by mikeperry

Keywords: tbb-5.0a3-essential added

Tag the set of things we should aim to understand/fix for the fist FF38-based TBB (5.0a3, on June 30th).

comment:2 Changed 4 years ago by mikeperry

Owner: changed from tbb-team to mikeperry
Status: newassigned

comment:3 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201506 MikePerry201506 added

comment:4 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201506R added; TorBrowserTeam201506 removed
Status: assignedneeds_review

Ok, I have finished auditing all of the Firefox socket/networking API usage. My notes are here:
https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF38_NETWORK_AUDIT

Everything that was suspicious/concerning is flagged with an XXX. Things that I'd like another set of eyes on are marked with a - prefix. The set of things I'm confident about are marked with a + prefix.

The following are things we want to keep an eye on in future releases (marked with "+ XXX"):

  • UDP push services (dom/push/PushService.jsm) are currently disabled, but will use UDP and bypass proxy settings if enabled.
  • The UDPSocket DOM API seems to be FxOS only for now, but can be enabled on the desktop via the pref dom.udpsocket.enabled
  • Similarly, the mozTCPSocket DOM API is FxOS only, but will bypass proxy settings and is also behind the pref dom.mozTCPSocket.enabled.
  • Roku Screen Sharing is FxOS/Android only, but will bypass proxy settings if enabled.

The following things could use a second set of eyes and a decision on what to do (marked with "- XXX"):

  • The WebIDE debugger (and possibly also parts of the webconsole debugger?) seem to have remote debugging capabilities. Additionally, if you use WebIDE in stock FF38, it downloads and installs an ADB addon and some other addon called Valence. The pref devtools.webide.enabled will disable WebIDE, but there are also prefs for devtools.debugger.enabled and devtools.debugger.remote-enabled. I think we want to turn all of these off, but it would be useful if someone else could verify that this is sufficient and not overkill (I found some conflicting information about remote debugging being available in FF33 vs FF39+, and it sure seems like my FF38 at least has WebIDE UI to connect remotely).
  • SimpleServiceDiscovery (related to Roku screen sharing) can also bypass proxy settings, and I'm not 100% sure it's not compiled in on the desktop. The moz.build files are a bit hard to follow here.
  • The "WebappRT" (runtime for webapps? See ./mobile/android/chrome/content/WebappRT.js) can set a whole bunch of prefs, including prefs that enable the DOM UDP/TCPSocket APIs. Can webapps be installed on the desktop? Does doing so suddenly enable all of these APIs?
  • The "Dashboard service" (./netwerk/base/Dashboard.cpp) can bypass proxy settings. What the hell is this thing? I don't see it being used anywhere else in the code...
  • What's the deal with the server sockets in wrt gfx/layers/LayerScope.cpp? Some websocket thing?

comment:5 in reply to:  4 Changed 4 years ago by gk

Keywords: GeorgKoppen201506 added

Replying to mikeperry:

  • The "Dashboard service" (./netwerk/base/Dashboard.cpp) can bypass proxy settings. What the hell is this thing? I don't see it being used anywhere else in the code...

It is used for about:networking. See: https://bugzilla.mozilla.org/show_bug.cgi?id=783205 and https://wiki.mozilla.org/Necko/Dashboard. https://mxr.mozilla.org/mozilla-esr38/source/toolkit/content/aboutNetworking.js depends on it. So, this is basically an internal API which I think is not harmful (although it might not be that useful for us) and we can keep.

Last edited 4 years ago by gk (previous) (diff)

comment:6 Changed 4 years ago by mcs

Cc: brade mcs added

comment:7 in reply to:  4 ; Changed 4 years ago by mcs

Replying to mikeperry:

  • Roku Screen Sharing is FxOS/Android only, but will bypass proxy settings if enabled.

Note that RokuApp.jsm is present in desktop Firefox builds. Flipping browser.casting.enabled to true enables it. I do not have a Roku device, but I assume it works.

  • SimpleServiceDiscovery (related to Roku screen sharing) can also bypass proxy settings, and I'm not 100% sure it's not compiled in on the desktop. The moz.build files are a bit hard to follow here.

Unfortunately, it is included in desktop builds. But the "search the local network for services" part that uses the network will not be executed unless browser.casting.enabled == true.

It makes me a little nervous to include code like this even though it is pref'd off. I guess it is no worse than a lot of other things. But it looks like we could remove the RokuApp.jsm and SimpleServiceDiscovery.jsm files from our builds without breaking anything in the default situation (things will break if someone sets browser.casting.enabled = true and restarts their browser).

Opinions?

comment:8 in reply to:  4 Changed 4 years ago by mcs

Replying to mikeperry:

  • What's the deal with the server sockets in wrt gfx/layers/LayerScope.cpp? Some websocket thing?

When enabled via gfx.layerscope.enabled=true, that code listens on TCP port 23456 for connections from a viewer application. It is disabled by default but I think TCP is used to allow for remote viewers (e.g., for Firefox OS or Android debugging).
See: https://wiki.mozilla.org/Platform/GFX/LayerScope

Maybe game developers would use something like this? At least it is disabled by default.

Last edited 4 years ago by mcs (previous) (diff)

comment:9 in reply to:  7 ; Changed 4 years ago by gk

Replying to mcs:

It makes me a little nervous to include code like this even though it is pref'd off. I guess it is no worse than a lot of other things. But it looks like we could remove the RokuApp.jsm and SimpleServiceDiscovery.jsm files from our builds without breaking anything in the default situation (things will break if someone sets browser.casting.enabled = true and restarts their browser).

Opinions?

I am in favor of ripping them out for defense-in-depth.

comment:10 Changed 4 years ago by mcs

For WebIDE, I think we should set prefs. like this:

devtools.webide.autoinstallADBHelper = false
devtools.webide.autoinstallFxdtAdapters = false

(prevent download and install of the extensions Mike mentioned)

and:

devtools.debugger.remote-enabled = false

(which is the default setting except in FF Developer Edition)

I could not find where devtools.debugger.enabled is currently used; back in FF 24 it was used to disable the JS debugger, but apparently no longer.

And as Mike mentioned we probably want to set:

devtools.webide.enabled = false

because as soon as you open WebIDE, networking code starts to run that looks for Firefox OS devices, etc. on the local network (via the code in toolkit/devtools/discovery/discovery.js)

Also, because we have disabled IndexedDB, when you open WebIDE it shows an error anyway: "Unable to load project list" (it uses IndexedDB to store info about the projects users are debugging).

comment:11 in reply to:  9 Changed 4 years ago by mcs

Replying to gk:

I am in favor of ripping them out for defense-in-depth.

OK. I filed #16439 for this. Patch coming soon.

comment:12 Changed 4 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, I set the webide prefs and pushed. I think we're good on the rest of this.

Note: See TracTickets for help on using tickets.