Opened 4 years ago

Closed 2 months ago

#14952 closed task (fixed)

Audit HTTP/2 and SPDY if needed

Reported by: gk Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-linkability, tbb-usability-website, tbb-performance, ff60-esr, TorBrowserTeam201808R
Cc: mcs, dcf@…, gk, arthuredelstein, mahrud, dmr Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description (last modified by gk)

HTTP/2 is enabled in Firefox 38. We should make sure it adheres to our design specifcation. If not we need to either patch the problematic things or switch them of via preferences. Related: #6101 and #4100.

Child Tickets

Change History (52)

comment:1 Changed 4 years ago by gk

Keep possible timing vectors wrt PING and SETTINGS frames in mind.

comment:2 Changed 4 years ago by gk

Description: modified (diff)

comment:3 Changed 4 years ago by mcs

Cc: mcs added

comment:4 Changed 4 years ago by mikeperry

Looking over Georg and my mails with Mark Nottingham, our concerns then were:

  1. Long-lived connections, especially without proper first party isolation in terms of connection re-use.
  2. We probably want to disable server push, due to cache inference attacks (see: https://gnunet.org/sites/default/files/uzunov2013torspdy.pdf section 6.3.2)
  3. PING and SETTINGS timing side channels.

I had this to say about PING and SETTINGS:

Relatedly, I'm somewhat worried about the bidirectional nature of PING and SETTINGS from the tor-specific perspective. Because these can be sent async by the server and are acked immediately by the client, they might be able to introduce a timing signature by the server beyond what is contained in response to active requests from the client.. Granted, such things are possible with Javascript (or even by simply shoving a bunch of junk inside an html comment in a hidden element), but people are able to disable Javascript, and static content-based mechanisms will also show load progress indication in the UI.

Therefore, In Tor Browser, I'd be tempted to close the connection if I got more than some frequency of PING or SETTINGS updates from the server, especially in the absence of other activity.

Mark said that closing such connections seemed fine, but we'd have to be careful about content elements just reopening them either through JS or dynamic CSS-based element loads. One option for that is to disable browser-based JS/HTML network activity in inactive tabs using Firefox's request filter APIs.

We also discussed playing with HTTP/2's various batching and padding parameters as another possible defense against website traffic fingerprinting, but I'm not sure there is enough there already to make this very useful beyond the types of things that our pipelining defense already does, and we still need more research in this area to know what we'd want to add/change and how. He suggested waiting for HTTP/3 for requesting such things in spec form, since there will be resistance to late spec changes that may change cost profiles for the server side, especially for deployment (since that has already begun).

comment:5 Changed 4 years ago by gk

Note: don't forget the keep-alive audit for SPDY (see: #4100).

comment:6 Changed 3 years ago by gk

Priority: normalmajor

comment:7 Changed 3 years ago by mikeperry

Keywords: tbb-5.0a-highrisk added

Tag the set of things that are risky to debut in the 5.0-stable release without testing in a prior alpha.

comment:8 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201506 added

Ensure all tbb-5.0a items are on the June radar.

comment:9 Changed 3 years ago by mikeperry

Keywords: MikePerry201506 added
Owner: changed from tbb-team to mikeperry
Status: newassigned

comment:10 Changed 3 years ago by mikeperry

I pushed prefs to disable HTTP/2 for 5.0a3. I took a quick look at the source code, and the HTTP/2 implementation in Firefox is heavily dependent on the SPDY implementation.. We're probably going to have to review both HTTP/2 and SPDY v3.1 as a result. :/

comment:11 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201507 added; TorBrowserTeam201506 removed

Move over remaining June items to July

comment:12 Changed 3 years ago by mikeperry

Keywords: MikePerry201507 added; MikePerry201506 removed

Move my tickets over for July

comment:13 Changed 3 years ago by mikeperry

Keywords: tbb-5.0a4 added

Tag some 5.0a4 goals.

comment:14 Changed 3 years ago by dcf

Cc: dcf@… added

comment:15 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201508 added; TorBrowserTeam201507 removed

comment:16 Changed 3 years ago by mikeperry

Keywords: MikePerry201508 added; ff38-esr tbb-5.0a-highrisk MikePerry201507 tbb-5.0a4 removed

I am pretty much done with this audit. I need to write this up and file one or more tickets for support.

This is not a 5.0 item. We won't be able to do this by 5.0.

comment:17 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201509 added; TorBrowserTeam201508 removed

Move remaining August tickets to September.

comment:18 Changed 3 years ago by gk

Keywords: TorBrowserTeam201510 added; TorBrowserTeam201509 removed

Moving Tor Browser tickets to October 2015.

comment:19 Changed 3 years ago by gk

Keywords: TorBrowserTeam201511 added; TorBrowserTeam201510 removed

comment:20 Changed 3 years ago by mikeperry

Keywords: MikePerry201508 TorBrowserTeam201511 removed
Severity: Normal

comment:21 Changed 2 years ago by gk

Summary: Audit HTTP/2Audit HTTP/2 and SPDY if needed

comment:22 Changed 21 months ago by gk

Keywords: tbb-linkability tbb-usability-website added
Owner: changed from mikeperry to tbb-team

Keep #16673 in mind.

comment:23 Changed 21 months ago by gk

Cc: gk added

comment:24 Changed 10 months ago by arthuredelstein

Cc: arthuredelstein added
Keywords: tbb-performance added

I wondered what the performance effects of SPDY/HTTP2 are. As a simple test, I used the network panel in Tor Browser to compare the loading speed of https://en.wikipedia.org/wiki/Main_Page. Here are the page loading times, in seconds:

network.http.spdy.enabled = false
8.36, 7.21, 7.16, 6.47, 9.39, 7.19, 7.10, 7.05,
7.21, 7.03, 8.58, 9.41, 7.03, 6.93, 6.81, 7.40
Average: 7.52 ± 0.90 s

network.http.spdy.enabled = true (also, sub-prefs enabled)
4.67, 5.18, 4.81, 4.56, 5.03, 4.62, 5.21, 4.95,
4.67, 4.69, 5.04, 4.73, 4.86, 4.71, 4.60, 4.82
Average: 4.82 ± 0.21 s

So, at least on that page, Tor Browser would benefit from re-enabled HTTP2/SPDY (unsuprisingly).

Last edited 10 months ago by arthuredelstein (previous) (diff)

comment:25 Changed 10 months ago by arthuredelstein

(Deleting this comment as a separate issue from HTTP/2.)

Last edited 10 months ago by arthuredelstein (previous) (diff)

comment:26 Changed 10 months ago by arthuredelstein

Here's a bug that was resolved for Firefox 55:
https://bugzilla.mozilla.org/show_bug.cgi?id=1334693
So it looks like a lot of work has been done already.

There's also a bug suggesting some unit tests should be written:
https://bugzilla.mozilla.org/show_bug.cgi?id=1337868

comment:27 Changed 10 months ago by arthuredelstein

Keywords: ff-59esr added

comment:28 Changed 10 months ago by cypherpunks

Keywords: ff59-esr added; ff-59esr removed

comment:29 Changed 10 months ago by arthuredelstein

Patrick McManus suggested we check if origin frames are properly isolated. Firefox has a pref to disable them if needed.

https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-04

comment:30 Changed 10 months ago by arthuredelstein

comment:31 Changed 9 months ago by gk

Keywords: ff60-esr added; ff59-esr removed

Firefox 60 is the new ESR.

comment:33 Changed 7 months ago by cypherpunks

comment:34 in reply to:  33 Changed 7 months ago by sysrqb

Replying to cypherpunks:

FWIW according to w3techs HTTP/2 is used by 24.5% of all the websites.

Yes, this is on the roadmap, we should be looking at this real-soon-now. Thanks!

comment:35 Changed 6 months ago by gk

We have first sites that basically break without having HTTP/2 enabled, see #25735.

comment:36 Changed 5 months ago by gk

Keywords: TorBrowserTeam201805 added

comment:37 Changed 5 months ago by gk

Priority: HighVery High

comment:38 Changed 4 months ago by gk

Keywords: TorBrowserTeam201806 added; TorBrowserTeam201805 removed

Moving our tickets to June 2018

comment:39 Changed 4 months ago by mahrud

Cc: mahrud added

comment:40 Changed 3 months ago by gk

Keywords: TorBrowserTeam201807 added; TorBrowserTeam201806 removed

Moving first batch of tickets to July 2018

comment:41 Changed 3 months ago by gk

Parent ID: #25735

comment:42 Changed 3 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201807 removed

Move our tickets to August.

comment:43 Changed 2 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed
Status: assignedneeds_review

To audit the HTTP2 implementation, I read through the code in:

  • nsHttpConnectionManager.h/.cpp
  • ASpdySession.h/.cpp
  • Http2Push.h/.cpp
  • Http2Session.h/.cpp
  • Http2Stream.h/.cpp

I didn't find any obvious instances where cross-first-party state might be leaked to content. I did note a number of places in the code where Origin Attributes are used to isolate by first-party or container ID:

HTTP/2

Each HTTP2 connection is specified by an nsConnectionEntry, which is assigned a single nsHttpConnectionInfo object. nsHttpTransaction is also assigned an nsHttpConnectionInfo object.

Stateful HTTP/2 features need to be isolated by first party as well.

I also confirmed the file cache is isolated:

AltSvc

I also wanted to check that AltSvc will be isolated by first party. I confirmed that Alternate Service hashes use OriginAttributes:

https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/netwerk/protocol/http/AlternateServices.cpp#221

Passive Fingerprinting

Both documents above (linked to in comment:30 and comment:32) raise the possibility of passive fingerprinting via HTTP/2 Settings parameters. Here's what I found:

  • SETTINGS_HEADER_TABLE_SIZE depends on the "network.http.spdy.default-hpack-buffer" pref. In Firefox it is set by default to 65536 on desktop and 4096 on mobile.
  • SETTINGS_ENABLE_PUSH and SETTINGS_MAX_CONCURRENT_STREAMS depend on "network.http.spdy.allow-push" pref, which is "true" by default.
  • SETTINGS_INITIAL_WINDOW_SIZE depends on "network.http.spdy.push-allowance", which is 131072 on desktop and 32768 on mobile by default.
  • SETTINGS_MAX_FRAME_SIZE is always set to 0x4000.

The above prefs don't provide significant entropy, unless the user has modified the one or more of them from their default value. Otherwise they mainly serve to distinguish different browsers or platforms.

PING or SETTINGS updates timing side-channels are something I am not addressing here. I think they could do with further investigation.

Results and patch

The isolation of state to origin attributes looks well done to me in Firefox 60ESR's implementation of HTTP2. That said, the code is highly complex so it would be impossible to say I haven't missed any leaks of state to content or fingerprinting attacks. It would be good to have https://bugzilla.mozilla.org/show_bug.cgi?id=1337868 implemented by the Tor uplift team if possible.

Over the long term, I would suggest that we should look into first-party isolation by process of both content and chrome code (the latter includes caching, networking code, and other stateful things.)

But given the convincing effort by Mozilla to use Origin Attributes to provide state isolation everywhere in the HTTP2 code, I think we should enable HTTP2. Here's my proposed patch for review:

https://github.com/arthuredelstein/tor-browser/commit/14952

Last edited 2 months ago by arthuredelstein (previous) (diff)

comment:44 Changed 2 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewneeds_revision

Nice, thanks for the investigation. Some first thoughts while reading through your notes:

1) Is the disk avoidance requirement respected in case there is some caching going on?
2) Does New Identity give us a clean slate with HTTP/2 enabled?
3) I don't see why we want to have server push enabled. Let's try with that disabled first.
4) I am fine leaving possible PING/SETTINGS-related timing side-channels for a different bug for now. If so, please open a new one.
5) I am not overly happy about the different values of some of the prefs you mentioned above depending on being on a desktop/mobile platform we should investigate the impact of shipping the same configuration for both of them. After all, tbb-fingerprinting-os bugs are still bugs. I guess this can be done in a new bug as well.

comment:45 Changed 2 months ago by gk

Oh, and 6): What about dom.push.http2.*? Are we getting the code behind those as well and, if so, are we good with it?

Last edited 2 months ago by gk (previous) (diff)

comment:46 Changed 2 months ago by dmr

Cc: dmr added

comment:47 Changed 2 months ago by arthuredelstein

Replying to gk:

Nice, thanks for the investigation. Some first thoughts while reading through your notes:

Thanks for the review and these good questions:

1) Is the disk avoidance requirement respected in case there is some caching going on?

I'm still working on figuring this out and will post here when I have an answer.

2) Does New Identity give us a clean slate with HTTP/2 enabled?

I looked into network connections, which are supposed to be killed as a result of torbutton sending a net:prune-all-connections notification. I followed the code to nsHttpConnectionMgr::ClosePersistentConnections(nsConnectionEntry *ent), which removes idle nsHttpConnections, causes active nsHttpConnections (which represents both HTTP1 and HTTP/2 connections) to immediately expire, regardless of whether these nsHttpConnections are HTTP/2 (mSpdySession) or not. So I think New Identity is correctly stopping HTTP/2 connections.

3) I don't see why we want to have server push enabled. Let's try with that disabled first.

OK, I've opened #27127 to remind us to audit and potentially enable push in the future. In the meantime I will set network.http.spdy.allow-push to false.

4) I am fine leaving possible PING/SETTINGS-related timing side-channels for a different bug for now. If so, please open a new one.

I opened #27123.

5) I am not overly happy about the different values of some of the prefs you mentioned above depending on being on a desktop/mobile platform we should investigate the impact of shipping the same configuration for both of them. After all, tbb-fingerprinting-os bugs are still bugs. I guess this can be done in a new bug as well.

OK, I have opened #27128.

Replying to gk:

Oh, and 6): What about dom.push.http2.*? Are we getting the code behind those as well and, if so, are we good with it?

These prefs are used by the Push API implementation. We are not currently affected by them, because we have dom.push.enabled set to false and dom.servicesWorkers.enabled set to false (both following Firefox 60ESR). These will be enabled in the next ESR, however (#15563).

Here's the revised patch for review:

https://github.com/arthuredelstein/tor-browser/commit/14952+1

comment:48 Changed 2 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed
Status: needs_revisionneeds_review

Requesting review for the revised patch, but in the meantime I am also still looking into the disk avoidance question.

comment:49 in reply to:  47 Changed 2 months ago by gk

Keywords: TorBrowserTeam201808 added; TorBrowserTeam201808R removed
Status: needs_reviewnew

Replying to arthuredelstein:

[snip]

Here's the revised patch for review:

https://github.com/arthuredelstein/tor-browser/commit/14952+1

Looks good. commit 36724cc11e94d0dc3094c94f046d76fb5ce44a2b on tor-browser-60.1.0esr-8.0-1 has the changes.

comment:50 Changed 2 months ago by gk

Parent ID: #25735

#25735 should be fixed with the changes committed in comment:49, unparenting.

comment:51 in reply to:  44 Changed 2 months ago by arthuredelstein

Keywords: TorBrowserTeam201808R added; TorBrowserTeam201808 removed
Status: newneeds_review

Replying to gk:

1) Is the disk avoidance requirement respected in case there is some caching going on?

I took two approaches to try to confirm that HTTP/2 is respecting disk avoidance.

1. In the first approach, I read through the caching code and installed breakpoints using gdb:

To write to the cache, nsHttpChannel.cpp calls mCacheEntry->OpenOutputStream
https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/netwerk/protocol/http/nsHttpChannel.cpp#5395

I repeatedly hard-reloaded https://commons.wikimedia.org/wiki/Vincent_van_Gogh, and I confirmed this code location was hit repeatedly with "network.http.spdy.enabled" and "network.http.spdy.enabled.http2" set to false, and also with both set to true. I used the Network Monitor tab to confirm that files were loading via HTTP/1.1 GET requests in the first case, and HTTP/2.0 in the second.

This indicates to me what I gathered from reading the code: that both HTTP1 and HTTP2 are using the same caching mechanism. So that gives us at least some hope that the private browsing policies are the same.

Then I looked at AltSvc. The AlternativeServices.cpp uses DataStorage.cpp to store its AltSvcMapping data (the mapping between primary scheme/host/port and alternate service scheme/host/port). DataStorage has three DataStorageType values:

  • DataStorage_Persistent
  • DataStorage_Temporary
  • DataStorage_Private

Only the DataStorage_Persistent type is sync'd to disk. So I wanted to confirm that DataStorage_Private is being used. So I put a breakpoint where AlternateServices.cpp stores a mapping:
https://dxr.mozilla.org/mozilla-esr60/rev/dd52b41d2b775e5c7261ce52795268b7670635fc/netwerk/protocol/http/AlternateServices.cpp#281
and then repeatedly visited an AltSvc header demo served at https://arthuredelstein.net/altsvc
I confirmed there that mPrivate was always true, and thus DataStorage_Private is used in Tor Browser.

2. The second approach was to use parent-process JS to measure the cache's disk consumption. I experimented with Firefox and Tor Browser. First I entered the following code in the Browser Console:

let logDiskConsumption = () => Services.cache2.asyncGetDiskConsumption({
  onNetworkCacheDiskConsumption: x => console.log(x),
  QueryInterface: XPCOMUtils.generateQI([
    Ci.nsISupportsWeakReference
  ])
});
setInterval(logDiskConsumption, 250);
Services.cache2.clear();
gBrowser.loadURI("https://commons.wikimedia.org/wiki/Vincent_van_Gogh");

This code logs the number of bytes found in the disk cache every 250 ms.

In Firefox 60.1.0esr, I confirmed that "network.http.spdy.enabled" and "network.http.spdy.enabled.http2" were set to true by default. Upon running the above code, I could see the disk consumption monotonically increasing until the wikimedia page was fully loaded. Then the disk consumption stopped increasing (and remained constant at 2722816). The Network Monitor indicated HTTP/2 was being used.

In Tor Browser 8.0a9, I manually set "network.http.spdy.enabled" and "network.http.spdy.enabled.http2" to true, because they were false by default. Then I ran the same code in the Browser Console, and confirmed that the total disk consumption remained constant at zero. Again, I used the Network Monitor to confirm HTTP/2.0 GET requests were being used.

(I also checked a Private Browsing Window with Firefox 60.1.0esr and saw no disk consumption. And ran the code in Tor Browser with "private.firstparty.isolate" set to false, again with no disk consumption.)

From the results of these two approaches, I am persuaded that normal HTTP caching, at least for loading web pages and images, is avoiding disk caching in Tor Browser 8.0a9, when we activate HTTP2. Also, in reading the HTTP2 code, I didn't find any special cases of data being written to disk that raised concerns. Nonetheless I can't rule out that anything at all is written to disk that isn't also with HTTP1.x, but that will require more comprehensive research.

Setting this for review in case more investigation is needed for this ticket.

comment:52 Changed 2 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks for the detailed analysis! Closing this ticket then. Let's file follow-up tickets in case we hit something related to HTTP2/AltSvc.

Note: See TracTickets for help on using tickets.