Opened 3 years ago

Closed 2 years ago

#21625 closed task (fixed)

Review networking code for Firefox 52

Reported by: gk Owned by: mikeperry
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: ff52-esr, tbb-7.0-must-alpha, TorBrowserTeam201705
Cc: brade, mcs, tom Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (19)

comment:1 Changed 3 years ago by gk

Keywords: ff52-esr tbb-7.0-must added
Owner: changed from tbb-team to mikeperry
Status: newassigned

Mike said he would help with that. Setting him as owner. Tentative deadline April 1.

comment:2 Changed 3 years ago by mikeperry

Ok, I'm done with the review. I conducted it a little different this time. I just did a 'git diff -U10 esr45 esr52' and focused primarily on the new lines, looking for various types of socket usage. (Things like the PR_ calls, UDPSockets, SOCK_, etc). My review notes will be posted in the repo.

I'm going to break the writeup down into categories on this ticket. First I'll list stuff that really needs to be patched. Then I'll list some stuff that we already patched that changed a bit (which may cause our patches to fail) that we should look at closely after patch application. Then I'll list stuff that it would be nice to have a second set of eyes confirm.

We may also want someone to go over the android stuff again in more detail, as well.

Will be posting the above mentioned sections one per comment here.

comment:3 Changed 3 years ago by mikeperry

Stuff we should patch/disable:

  • FlyWeb (dom/flyweb/FlyWebService.cpp) - This is a mechanism for contacting local devices and interacting with them. It may not be fully implemented, but networking code is definitely here. Disable it.
  • dom/presentation/* and nsNetworkInfoService::ListNetworkAddresses - the Presentation API (for remote displays - https://developer.mozilla.org/en-US/docs/Web/API/Presentation_API). This needs to be disabled even if proxied, because it does ICE-style IP address discovery and advertisement.
  • ./dom/presentation/provider/MulticastDNSDeviceProvider.cpp - used by the Presentation API to announce itself (and maybe other stuff?). Make sure it gets disabled.
  • The Rust URL parser (third_party/rust/url/src/host.rs) has a to_socket_addrs and ToSocketAddrs methods. These should be patched out for safety and to remind us later, I think.
  • netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm - more mDNS stuff that should be disabled.

Android stuff that definitely leaks that we should fix (missing proxy params to HttpUrlConnection - these need to use the buildHttpConnection helper to get a proxy):

  • mobile/android/base/java/org/mozilla/gecko/feeds/FeedFetcher.java
  • mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java
  • mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
  • mobile/android/thirdparty/com/keepsafe/switchboard/SwitchBoard.java

That's it for the stuff that definitely needs patching. I'll post the other sets as soon as I can.

comment:4 Changed 3 years ago by mcs

Cc: brade mcs added

comment:5 Changed 3 years ago by tom

Cc: tom added

comment:6 Changed 3 years ago by mikeperry

Stuff to verify is still patched or disabled (part 2/3)

  • The DNS service was changed a bit for e10s. See ./netwerk/dns/ChildDNSService.cpp. Verify our DNS patch still actually disables non-SOCKS DNS with e10s.
  • Make sure RTSP is still disabled for desktop and Android (netwerk/protocol/rtsp/*)
  • Make sure disabling WebRTC still disables all of the ./media/mtransport/* stuff.
  • Verify our defense-in-depth patches to NSS/OCSP still apply (ditto for other proxy patches)
  • Verify that the TCPSocket and UDPSocket DOM APIs are still disabled by pref (esp if the moz prefix goes away).
Last edited 3 years ago by mikeperry (previous) (diff)

comment:7 Changed 3 years ago by mikeperry

Components that could probably use another review (Part 3/3):

  • The NetworkInfoService (./netwerk/base/NetworkInfoServiceCocoa.cpp and ./netwerk/base/NetworkInfoServiceLinux.cpp) both collect a list of local IP addresses for use in nsNetworkInfoService::ListNetworkAddresses(). This is used by mDNS and the Presentation API. Did I miss any uses? Maybe we want to patch this out just in case?
  • media/mtransport/nr_socket_prsock.cpp is an alternate non-SOCKS socket API. It should be disabled by WebRTC, but if Mozilla removed the compile-time WebRTC option, this definitely needs a double-check that it is not used.
  • netwerk/base/ThrottleQueue.cpp uses what appear to be local sockets for timer notification. Could use a double-check.
  • On Android, the uses of ch.boye.httpclientandroidlib.impl.client.* should be verified again.
  • The full git diff from esr45 to esr52 of ./android/ should probably be reviewed by someone with more Android development experience than me, for additional networking calls.
Last edited 3 years ago by mikeperry (previous) (diff)

comment:9 in reply to:  3 Changed 3 years ago by gk

Replying to mikeperry:

Stuff we should patch/disable:

  • FlyWeb (dom/flyweb/FlyWebService.cpp) - This is a mechanism for contacting local devices and interacting with them. It may not be fully implemented, but networking code is definitely here. Disable it.

We have #21746 for that.

#18862

  • ./dom/presentation/provider/MulticastDNSDeviceProvider.cpp - used by the Presentation API to announce itself (and maybe other stuff?). Make sure it gets disabled.
  • The Rust URL parser (third_party/rust/url/src/host.rs) has a to_socket_addrs and ToSocketAddrs methods. These should be patched out for safety and to remind us later, I think.
  • netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm - more mDNS stuff that should be disabled.

We have #21861 for the mdns stuff and #21862 for the Rust part.

Android stuff that definitely leaks that we should fix (missing proxy params to HttpUrlConnection - these need to use the buildHttpConnection helper to get a proxy):

  • mobile/android/base/java/org/mozilla/gecko/feeds/FeedFetcher.java
  • mobile/android/base/java/org/mozilla/gecko/media/GeckoMediaDrmBridgeV21.java
  • mobile/android/base/java/org/mozilla/gecko/search/SearchEngineManager.java
  • mobile/android/thirdparty/com/keepsafe/switchboard/SwitchBoard.java

That's #21683.

Version 0, edited 3 years ago by gk (next)

comment:10 Changed 3 years ago by gk

Keywords: TorBrowserTeam201704 added; TorBrowserTeam201703 removed

Moving tickets over to April

comment:11 Changed 3 years ago by gk

Keywords: tbb-7.0-must-alpha added; tbb-7.0-must removed

Getting this on our radar for alpha release in less than two weeks.

comment:12 Changed 2 years ago by gk

Keywords: TorBrowserTeam201705 added; TorBrowserTeam201704 removed

Moving our tickets to May 2017.

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

mcs/brade: I'd like to hear your opinion about the TCPSocket stuff (see below) as you had concerns about that the last time which resulted into filing #18866. (All the other pieces replied to in this comment are even less problematic I think.)

Replying to mikeperry:

Stuff to verify is still patched or disabled (part 2/3)

  • The DNS service was changed a bit for e10s. See ./netwerk/dns/ChildDNSService.cpp. Verify our DNS patch still actually disables non-SOCKS DNS with e10s.

ChildDNSService.cpp has no own resolver capabilities. Sync resolve is not supported at all; AsyncResolveExtended creates a DNSChildRequest and starts that request. It gets sent to the parent process (SendPDNSReqeustContstructor()). The corresponding RecvPDNSRequestConstructor method calls DoAsyncResolve provided by DNSRequestParent which calls AsyncResolveExtended which we have patched in nsDNSService2.cpp.

  • Make sure RTSP is still disabled for desktop and Android (netwerk/protocol/rtsp/*)

RTSP is gone with

https://bugzilla.mozilla.org/show_bug.cgi?id=1295885
https://bugzilla.mozilla.org/show_bug.cgi?id=1291629

. The hint in the moz.build file is just a leftover.

  • Make sure disabling WebRTC still disables all of the ./media/mtransport/* stuff.

We have

if CONFIG['MOZ_WEBRTC']:
    DIRS += [
        '/media/webrtc',
        '/media/mtransport',
    ]

in toolkit.mozbuild and we don't set MOZ_WEBRTC as we don't compile it in with the configure switch.

  • Verify our defense-in-depth patches to NSS/OCSP still apply (ditto for other proxy patches)

They do and other patches still applied as well (see #20680 for what we did and for review comments).

  • Verify that the TCPSocket and UDPSocket DOM APIs are still disabled by pref (esp if the moz prefix goes away).

There is no pref anymore for TCPSocket, rather it is bound to ShouldTCPSocketExist:

-  [NewObject, Pref="dom.mozTCPSocket.enabled", CheckAnyPermissions="tcp-socket"]
+  [NewObject, Func="mozilla::dom::TCPSocket::ShouldTCPSocketExist"]

which does

return nsContentUtils::IsSystemPrincipal(nsContentUtils::ObjectPrincipal(global));

. Thus only chrome code can use it. I think we are not worse off than we were with the pref in ESR45.

There are no changes regarding the UDPSocket DOM API, so we are still good.

comment:14 Changed 2 years ago by cypherpunks

2c: now Firefox uses 3 loopback connections (the child process too).
What about DNS-RPC calls?
(Also what protection might be added against extraction of MAC address as in recent exploit? Maybe, some interception of such system calls?)

comment:15 in reply to:  7 Changed 2 years ago by gk

Replying to mikeperry:

Components that could probably use another review (Part 3/3):

  • The NetworkInfoService (./netwerk/base/NetworkInfoServiceCocoa.cpp and ./netwerk/base/NetworkInfoServiceLinux.cpp) both collect a list of local IP addresses for use in nsNetworkInfoService::ListNetworkAddresses(). This is used by mDNS and the Presentation API. Did I miss any uses? Maybe we want to patch this out just in case?

Sounds like a good idea to me. This is: #22165.

  • media/mtransport/nr_socket_prsock.cpp is an alternate non-SOCKS socket API. It should be disabled by WebRTC, but if Mozilla removed the compile-time WebRTC option, this definitely needs a double-check that it is not used.

Marked for ff59-esr (#22166).

  • netwerk/base/ThrottleQueue.cpp uses what appear to be local sockets for timer notification. Could use a double-check.

This looks actually okay to me. It got implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1244227, for dev tools as an option to simulate crappy networks.

  • On Android, the uses of ch.boye.httpclientandroidlib.impl.client.* should be verified again.

#22170

  • The full git diff from esr45 to esr52 of ./android/ should probably be reviewed by someone with more Android development experience than me, for additional networking calls.

#22171

comment:16 in reply to:  13 ; Changed 2 years ago by mcs

Replying to gk:

mcs/brade: I'd like to hear your opinion about the TCPSocket stuff (see below) as you had concerns about that the last time which resulted into filing #18866. (All the other pieces replied to in this comment are even less problematic I think.)

Kathy and I agree with your analysis.
We do wonder why Mozilla is keeping APIs such as these (making them chrome-only) instead of simply removing them.

comment:17 in reply to:  16 ; Changed 2 years ago by cypherpunks

Replying to mcs:

We do wonder why Mozilla is keeping APIs such as these (making them chrome-only) instead of simply removing them.

Compatibility: https://bugzilla.mozilla.org/show_bug.cgi?id=1247628

comment:18 in reply to:  17 Changed 2 years ago by mcs

Replying to cypherpunks:

Replying to mcs:

We do wonder why Mozilla is keeping APIs such as these (making them chrome-only) instead of simply removing them.

Compatibility: https://bugzilla.mozilla.org/show_bug.cgi?id=1247628

Thanks. I would characterize the situation more as "Mozilla may reuse the TCPSocket/UDPSocket code to provide similar capabilities to WebExtensions add-ons" (which, if you compare what can be done in an XPCOM extension with WebExtensions could be viewed as a compatibility issue).

comment:19 Changed 2 years ago by gk

Resolution: fixed
Status: assignedclosed

Thanks, we are done here then, yay!

Note: See TracTickets for help on using tickets.