Opened 19 months ago

Last modified 12 days ago

#22176 new task

Review networking code for Firefox 60

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff60-esr, TorBrowserTeam201811, GeorgKoppen201811
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

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

Child Tickets

TicketStatusOwnerSummaryComponent
#27616newtbb-teamDouble-check Rust code for potential proxy bypass in ESR 60Applications/Tor Browser

Change History (19)

comment:1 Changed 14 months ago by cypherpunks

Priority: MediumVery High
Severity: NormalCritical

comment:2 Changed 10 months ago by gk

Description: modified (diff)
Keywords: ff60-esr added; ff59-esr removed
Priority: Very HighMedium
Severity: CriticalNormal
Summary: Review networking code for Firefox 59Review networking code for Firefox 60

comment:3 Changed 6 months ago by gk

Keywords: TorBrowserTeam201805 GeorgKoppen201805 added
Priority: MediumHigh

comment:4 Changed 6 months ago by gk

Priority: HighVery High

comment:5 Changed 6 months ago by cypherpunks

I wonder why the severity value doesn't match that of #21625

comment:6 Changed 6 months ago by gk

Note to self: see: comment:11:ticket:21862 for Rust related things.

comment:7 Changed 6 months ago by gk

Keywords: GeorgKoppen201806 added; GeorgKoppen201805 removed

Moving my tickets to June 2018.

comment:8 Changed 5 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:9 Changed 5 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

Moving first batch of tickets to July 2018

comment:10 Changed 5 months ago by gk

Keywords: GeorgKoppen201807 added; GeorgKoppen201806 removed

Moving my tickets.

comment:11 Changed 4 months ago by gk

Keywords: GeorgKoppen201808 added; GeorgKoppen201807 removed

Moving my tickets.

comment:12 Changed 4 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:13 Changed 2 months ago by gk

Keywords: GeorgKoppen201809 added; GeorgKoppen201808 removed

Moving my tickets to September.

comment:14 Changed 2 months ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:15 Changed 2 months ago by gk

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 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 CONFIGMOZ_WEBRTC? and CONFIGCOMPILE_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.html and we still have pref("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:

1) I looked as good as I could for things that could use/provide UDP sockets etc.
2) 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

3) I started to look backwards from the actual things that got embedded into gkrust which can be found in toolkit/library/rust/gkrust-features.mozbuild checking 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...

comment:16 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:17 Changed 6 weeks ago by gk

Keywords: GeorgKoppen201810 added; GeorgKoppen201809 removed

Moving my tickets to October.

comment:18 Changed 12 days ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:19 Changed 12 days ago by gk

Keywords: GeorgKoppen201811 added; GeorgKoppen201810 removed
Note: See TracTickets for help on using tickets.