Opened 2 years ago

Closed 7 days ago

#22176 closed task (fixed)

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, GeorgKoppen201907, TorBrowserTeam201907
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
#27616closedtbb-teamDouble-check Rust code for potential proxy bypass in ESR 60Applications/Tor Browser

Change History (36)

comment:1 Changed 22 months ago by cypherpunks

Priority: MediumVery High
Severity: NormalCritical

comment:2 Changed 18 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 15 months ago by gk

Keywords: TorBrowserTeam201805 GeorgKoppen201805 added
Priority: MediumHigh

comment:4 Changed 15 months ago by gk

Priority: HighVery High

comment:5 Changed 15 months ago by cypherpunks

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

comment:6 Changed 14 months ago by gk

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

comment:7 Changed 14 months ago by gk

Keywords: GeorgKoppen201806 added; GeorgKoppen201805 removed

Moving my tickets to June 2018.

comment:8 Changed 14 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:9 Changed 13 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

Moving first batch of tickets to July 2018

comment:10 Changed 13 months ago by gk

Keywords: GeorgKoppen201807 added; GeorgKoppen201806 removed

Moving my tickets.

comment:11 Changed 12 months ago by gk

Keywords: GeorgKoppen201808 added; GeorgKoppen201807 removed

Moving my tickets.

comment:12 Changed 12 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:13 Changed 11 months ago by gk

Keywords: GeorgKoppen201809 added; GeorgKoppen201808 removed

Moving my tickets to September.

comment:14 Changed 11 months ago by gk

Keywords: TorBrowserTeam201809 added; TorBrowserTeam201808 removed

Moving our tickets to September 2018

comment:15 Changed 10 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 10 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:17 Changed 9 months ago by gk

Keywords: GeorgKoppen201810 added; GeorgKoppen201809 removed

Moving my tickets to October.

comment:18 Changed 8 months ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:19 Changed 8 months ago by gk

Keywords: GeorgKoppen201811 added; GeorgKoppen201810 removed

comment:20 Changed 8 months ago by gk

Keywords: GeorgKoppen201812 added; GeorgKoppen201811 removed

Moving my tickets to December.

comment:21 Changed 7 months ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201811 removed

Moving our tickets to December.

comment:22 Changed 6 months ago by gk

Keywords: GeorgKoppen201901 added; GeorgKoppen201812 removed

Moving my tickets.

comment:23 Changed 6 months ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201812 removed

Moving tickets to Jan 2019.

comment:24 Changed 5 months ago by gk

Keywords: GeorgKoppen201902 added; GeorgKoppen201901 removed

Moving my tickets to Feb

comment:25 Changed 5 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:26 Changed 4 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving my tickets to March.

comment:27 Changed 4 months ago by gk

Keywords: GeorgKoppen201903 added; GeorgKoppen201902 removed

Now for my keyword.

comment:28 Changed 3 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:29 Changed 3 months ago by gk

Keywords: GeorgKoppen201904 added; GeorgKoppen201903 removed

Moving my tickets for April

comment:30 Changed 2 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:31 Changed 2 months ago by gk

Keywords: GeorgKoppen201905 added; GeorgKoppen201904 removed

Move my tickets.

comment:32 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:33 Changed 5 weeks ago by gk

Keywords: GeorgKoppen201906 added; GeorgKoppen201905 removed

Moving my tickets to June

comment:34 Changed 2 weeks ago by gk

Keywords: GeorgKoppen201907 added; GeorgKoppen201906 removed

Moving my tickets to July.

comment:35 Changed 10 days ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:36 Changed 7 days ago by gk

Resolution: fixed
Status: newclosed

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. The result is commit 2ac466c637335af083b7b747b490979fb33f6a1c in tor-browser-spec.

I included the Android proxy safety review sysrqb did in #21863, #22170, and #25851 and child bugs.

Note: See TracTickets for help on using tickets.