Opened 4 years ago

Closed 3 years ago

#18546 closed task (fixed)

Review networking code for Firefox 45

Reported by: gk Owned by: mikeperry
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff45-esr, MikePerry201604, TorBrowserTeam201605, tbb-6.0-must
Cc: mcs, brade, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should review the networking code for Firefox 45 to make sure the are no Tor bypasses.

Child Tickets

Change History (11)

comment:1 Changed 4 years ago by gk

Keywords: ff45-esr MikePerry201603 added
Owner: changed from tbb-team to mikeperry
Status: newassigned

Mike said he would help us with that one. Assigning the ticket to him.

comment:2 Changed 4 years ago by gk

Keywords: TorBrowserTeam201604 added; TorBrowserTeam201603 removed

comment:3 Changed 4 years ago by gk

Keywords: MikePerry201604 added; MikePerry201603 removed

comment:4 Changed 4 years ago by mikeperry

I pushed my partial progress to: https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF45_NETWORK_AUDIT

Everything that could use a double-check is flagged with XXX.

Here's the quick notes for stuff that really needs another set of eyes:

  • We need to verify the proper application of our OCSP and NSS safety patches in security/nss. Last time we improperly applied the DNS patch while rebasing. That might happen again here, too.
  • We should make sure that ./netwerk/dns/mdns/libmdns/ is Android only and also disabled for OrFox
  • The "Presentation API" stuff seems new, but possibly not enabled yet. It has lots of networking things. We should make sure it is disabled.
  • The nsDNSService patches should be verified for the same reason as the NSS ones
  • There's some resolver stuff in Android that uses SOCK_DGRAM. We should make sure this is not active in OrFox
  • It looks like ./toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm is included now? Can we kill it? And what is this second screen stuff?
  • dom.udpsocket and dom.moztcpsocket are still off, yes?
  • We disabled/patched the debugger and related discovery stuff before, right? Is that still off?


Here's some stuff we should fix:

  • We should get rid of the damn DNS lookup for localhost in toolkit/profile/nsProfileLock.cpp
  • We should patch the "Network Tickler" to be disabled for real, since it looks like it may now apply to the desktop as well. A simple return in nsHttpHandler::TickleWifi() should do the trick, I think.
  • We should disable all of the dom.push.* prefs. Even though it seems that only ServiceWorkers can use Push, it would be good for us to ensure now that if we decide to enable ServiceWorkers, push stays off
  • Shumway (the flash previewer/player) can bypass proxy settings. If it is compiled in, we should rip it out/disable it at build time, so nobody enables it.

comment:5 Changed 4 years ago by mcs

I created tickets to track the things that are on the "should fix" list so far:

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

Replying to mikeperry:

Here's the quick notes for stuff that really needs another set of eyes:

  • We need to verify the proper application of our OCSP and NSS safety patches in security/nss. Last time we improperly applied the DNS patch while rebasing. That might happen again here, too.

They look good to me.

  • We should make sure that ./netwerk/dns/mdns/libmdns/ is Android only and also disabled for OrFox

This is #18821.

  • The "Presentation API" stuff seems new, but possibly not enabled yet. It has lots of networking things. We should make sure it is disabled.
  • The nsDNSService patches should be verified for the same reason as the NSS ones
  • There's some resolver stuff in Android that uses SOCK_DGRAM. We should make sure this is not active in OrFox
  • It looks like ./toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm is included now? Can we kill it? And what is this second screen stuff?
  • dom.udpsocket and dom.moztcpsocket are still off, yes?
  • We disabled/patched the debugger and related discovery stuff before, right? Is that still off?
Last edited 4 years ago by gk (previous) (diff)

comment:7 in reply to:  6 Changed 3 years ago by gk

Replying to gk:

Replying to mikeperry:

Here's the quick notes for stuff that really needs another set of eyes:

  • We need to verify the proper application of our OCSP and NSS safety patches in security/nss. Last time we improperly applied the DNS patch while rebasing. That might happen again here, too.

They look good to me.

  • We should make sure that ./netwerk/dns/mdns/libmdns/ is Android only and also disabled for OrFox

This is #18821.

  • The "Presentation API" stuff seems new, but possibly not enabled yet. It has lots of networking things. We should make sure it is disabled.

Yes, it is disabled. However, we already had libmdns things leaking into desktop/android builds which are related to the Presentation API (see: https://wiki.mozilla.org/images/thumb/e/e6/Presentation_API_Architecture_overview.png/650px-Presentation_API_Architecture_overview.png). Thus, we should take a closer look at the whole picture when we move to ESR52: #18862

  • The nsDNSService patches should be verified for the same reason as the NSS ones

Looks good to me.

  • There's some resolver stuff in Android that uses SOCK_DGRAM. We should make sure this is not active in OrFox

Might be best to ask the Orfox people as this code is available for ages and IIRC Orfox is already supposed to be proxy bypass free: #18864.

  • It looks like ./toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm is included now? Can we kill it? And what is this second screen stuff?

We take care of it by the patch mcs and brade wrote back for #16439 (cafffd10e5be3dc27b3a666df1769ee53eb9b009 on tor-browser-45.0.2esr-6.x-1).

  • dom.udpsocket and dom.moztcpsocket are still off, yes?

The former, yes, for the latter there are no relevant changes compared to ESR38 it seems. However, this pref is not really exposed, so we may want to set it explicitly to avoid digging through code each time. This is #18863.

  • We disabled/patched the debugger and related discovery stuff before, right? Is that still off?

Yes and comparing the ESR38 prefs with the ESR45 show we are still good here.

comment:8 Changed 3 years ago by mikeperry

Ok, I pushed the final review to https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF45_NETWORK_AUDIT. Head is now 4e1a7e2cb23a9f6b7f33bf460f48571e771e951b

More android stuff for Orfox to check:

  • ./dom/media/android/AndroidMediaResourceServer.cpp
  • ./build/mobile/sutagent/android/
  • Tthe RtspMediaResource stuff in ./dom/media/ is enabled for all android. We should disable it, as it can do UDP.

I found more debugger stuff in the devtools directories, but this is redundant to the other stuff, and prefed off.

Additional socket stuff is in the gfx layers, but I think this is just e10s RPC. Worth a double check:

  • ./gfx/layers/LayerScope.cpp

I think the only immediate concern is verifying that the gfx stuff is indeed just e10s RPC. The rest of the stuff Amogh and Nathan should verify for OrFox.

Otherwise, I think we're good!

comment:9 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201604 removed

Moving tickets

comment:10 Changed 3 years ago by gk

Keywords: tbb-6.0-must added

comment:11 in reply to:  8 Changed 3 years ago by gk

Resolution: fixed
Status: assignedclosed

Replying to mikeperry:

Ok, I pushed the final review to https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF45_NETWORK_AUDIT. Head is now 4e1a7e2cb23a9f6b7f33bf460f48571e771e951b

More android stuff for Orfox to check:

  • ./dom/media/android/AndroidMediaResourceServer.cpp
  • ./build/mobile/sutagent/android/
  • Tthe RtspMediaResource stuff in ./dom/media/ is enabled for all android. We should disable it, as it can do UDP.

I created #19076, #19077 and #19078 for them.

I found more debugger stuff in the devtools directories, but this is redundant to the other stuff, and prefed off.

Additional socket stuff is in the gfx layers, but I think this is just e10s RPC. Worth a double check:

  • ./gfx/layers/LayerScope.cpp

Actually, that is e10s unrelated. It is a debugging feature for graphics issues that is prefed off: https://wiki.mozilla.org/Platform/GFX/LayerScope. You need a special extension for viewing the LayerScope. And that extension is communicating with the browser over WebSockets. I think we are fine here.

Note: See TracTickets for help on using tickets.