Review networking code for Firefox 52
We should review the networking code for Firefox 52 to make sure the are no Tor bypasses.
Activity
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.
-
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.
-
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).
-
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.
-
The full notes are now pushed to https://gitweb.torproject.org/tor-browser-spec.git/tree/audits/FF52_NETWORK_AUDIT
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 (moved) for that.
- 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.
We have #21861 (moved) for the mdns stuff and #21862 (moved) 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 #21863 (moved).
-
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 (moved). (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;
AsyncResolveExtendedcreates a DNSChildRequest and starts that request. It gets sent to the parent process (SendPDNSReqeustContstructor()). The correspondingRecvPDNSRequestConstructormethod callsDoAsyncResolveprovided byDNSRequestParentwhich callsAsyncResolveExtendedwhich 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.buildfile 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.mozbuildand we don't setMOZ_WEBRTCas 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 (moved) 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 toShouldTCPSocketExist:- [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.
-
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 (moved).
- 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 (moved)).- 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.
- 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.
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 (moved). (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.
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
-
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).
mentioned in issue #21863 (moved)
mentioned in issue #22171 (moved)
mentioned in issue #22176 (moved)
mentioned in issue tpo/applications/tor-browser#21863 (closed)
mentioned in issue tpo/applications/tor-browser#22171 (closed)
