Review networking code for Firefox 60
We should review the networking code for Firefox 60 to make sure the are no Tor bypasses.
Activity
Trac:
Keywords: ff59-esr deleted, ff60-esr added
Severity: Critical to Normal
Description: We should review the networking code for Firefox 59 to make sure the are no Tor bypasses.to
We should review the networking code for Firefox 60 to make sure the are no Tor bypasses.
Summary: Review networking code for Firefox 59 to Review networking code for Firefox 60
Priority: Very High to MediumI wonder why the severity value doesn't match that of #21625 (moved)
-
Okay, I followed Mike in doing just a 'git diff -U10 esr52 esr60' 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 think we are good with respect to proxy bypass issues, however I have opened #27616 (moved) to have someone looking with fresh eyes on the Rust situation as it is new and complex. I did look at the laundry list from ESR52 (see: comment:3:ticket:21625 and comment:6:ticket:21625) while I was reviewing:
*Make sure disabling WebRTC still disables all of the ./media/mtransport/ stuff and SCTP
if CONFIG['MOZ_WEBRTC'] and CONFIG['COMPILE_ENVIRONMENT']: DIRS += [ '/media/webrtc', '/media/mtransport', ] in toolkit/toolkit.mozbuild
SCTP is only enabled with WebRTC (see MOZ_SCTP in old-configure.in and MOZ_WEBRTC in ./dom/base/moz.build)
**Verify our defense-in-depth patches to NSS/OCSP still apply (ditto for other proxy patches)
Done during rebase review.
**Verify that the TCPSocket and UDPSocket DOM APIs are still disabled by pref (esp if the moz prefix goes away).
TCP socket is chrome-only via ShouldTCPSocketExist() check. There is even a test for that:
test_tcpsocket_not_exposed_to_content.htmland we still havepref("dom.udpsocket.enabled", false);for UDP socket.The Rust review was complicated. There are a bunch of crates included, like
mio, that can do all sorts of networking stuff, but we can't rip that out and modifying the crates is tricky as they are needed as a build time requirement and their integrity checked by hash. So, I did that part of the audit three-fold:- I looked as good as I could for things that could use/provide UDP sockets etc.
- I checked where dangerous libraries got included into the tree and why, checking whether the new APIs got used there. Two examples would be:
https://bugzilla.mozilla.org/show_bug.cgi?id=1391523 which is the cubeb bug which imports mio, mio-uds, net2, ws2_32. Later in https://bugzilla.mozilla.org/show_bug.cgi?id=1428952 cubeb/audioipc got updated to use tokio for async socket processing and imports
among others tokio-*
https://bugzilla.mozilla.org/show_bug.cgi?id=1437571 updated mozrunner: winapi/winapi-0.2.8 -> winreg (update to 0.5.0) -> geckodriver/mozrunner
- I started to look backwards from the actual things that got embedded into
gkrustwhich can be found intoolkit/library/rust/gkrust-features.mozbuildchecking the features for potential proxy bypasses and I took Manish's great comment:11:ticket:21862 into account.
Not sure if those are good approaches. Happy to improve that process and if we think we should indeed hack the Rust crates to back out dangerous calls I am open for that heavy-handed approach...
-
Okay, I am finally done here after doing the review a second time (this time in my spare time, though) as I accidentally nuked the notes from the first round while waiting for #27616 (moved). The result is commit 2ac466c637335af083b7b747b490979fb33f6a1c in
tor-browser-spec.I included the Android proxy safety review sysrqb did in #21863 (moved), #22170 (moved), and #25851 (moved) and child bugs.
Trac:
Status: new to closed
Resolution: N/A to fixed mentioned in issue #27616 (moved)