Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#16998 closed task (fixed)

Make sure <link rel="preconnect"> adheres to URL bar domain isolation

Reported by: gk Owned by: arthuredelstein
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: ff45-esr, tbb-linkability, tbb-6.0a5, TorBrowserTeam201606R
Cc: arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

<link rel="preconnect"> got implemented in Firefox 39 (https://bugzilla.mozilla.org/show_bug.cgi?id=1135160) we should make sure it follows our URL bar domain isolation paradigm. It might be worth to look at the the implementation itself as it claims "allowing to anticipate a future connection without revealing any information"

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by gk

Keywords: tbb-6.0a5 added

comment:2 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added
Owner: changed from tbb-team to arthuredelstein
Severity: Normal
Status: newaccepted

comment:3 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604 added

We want that for the alpha and the ESR 45 stable series.

comment:4 Changed 3 years ago by arthuredelstein

Here's a (temporary) fix that disables link rel=preconnect.

https://github.com/arthuredelstein/tor-browser/commit/8fe8aa27d1dc376b08cf4ad9107d1ebd6467ebee

I'm in the midst of building and testing this fix.

comment:5 Changed 3 years ago by arthuredelstein

My build finished, and I tested it with the following page:
https://arthuredelstein.github.io/tordemos/preconnect.html

I confirmed that the preconnect connection to example.com does not occur.

comment:6 Changed 3 years ago by mcs

For what its worth, Arthur's patch also looks OK to me.

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201605 added; TorBrowserTeam201604 removed

Moving tickets

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606 added; TorBrowserTeam201605 removed

comment:9 Changed 3 years ago by arthuredelstein

Status: acceptedneeds_review

Here's a new branch with three patches:

ac5256ecabbaf0defb421c7598f23a4a1901302c reverts the temporary patch posted in comment:4
a77e88a6309bec6be8eb93ad864b40c90497d962 isolates link rel=preconnect to first party
f42889501e5321d3c3a5bb558a3b2932264b60f5 is a fixup for our first-party regression test that ensures that the preconnect connection has the correct first party

I confirmed that reverting the second patch causes the test in the third patch to fail.

comment:10 Changed 3 years ago by gk

Keywords: TorBrowserTeam201606R added; TorBrowserTeam201606 removed

comment:11 Changed 3 years ago by mcs

Kathy and I reviewed this and ran the tests (as well as a manual test so we could look at the domain isolation logging). Everything seems to be working correctly. Nice work!

It might be helpful to add some comments where you pass an empty string for the isolation key, e.g., in netwerk/base/Predictor.cpp, inside IOServiceProxyCallback::OnProxyAvailable(), and in netwerk/ipc/NeckoParent.cpp. I know Predictor.cpp is OK because that code is pref'd off. Kathy and I are less confident about NeckoParent.cpp because we are not as familiar with the IPC code and all the ways it may be used.

Also, the UUID inside netwerk/base/nsISpeculativeConnect.idl should be changed. Kathy and I are also worried that changing signatures of existing IDL'd methods may break add-ons. Should we add new methods instead?

comment:12 in reply to:  11 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Kathy and I reviewed this and ran the tests (as well as a manual test so we could look at the domain isolation logging). Everything seems to be working correctly. Nice work!

Thank you for the review!

It might be helpful to add some comments where you pass an empty string for the isolation key, e.g., in netwerk/base/Predictor.cpp, inside IOServiceProxyCallback::OnProxyAvailable(), and in netwerk/ipc/NeckoParent.cpp. I know Predictor.cpp is OK because that code is pref'd off. Kathy and I are less confident about NeckoParent.cpp because we are not as familiar with the IPC code and all the ways it may be used.

Good point. I decided to add isolationKeys to the IPC code just to be sure.

Also, the UUID inside netwerk/base/nsISpeculativeConnect.idl should be changed. Kathy and I are also worried that changing signatures of existing IDL'd methods may break add-ons. Should we add new methods instead?

I've added new methods as you suggest. This means that the Predictor code is left to use the old methods, but, as you point out, we have the predictor preffed off.

Here's the new version:

https://github.com/arthuredelstein/tor-browser/commits/16998+10

Same patches as before:

  • b44b2b25bab1ebde9351c842f0ca66d2fdc87579 reverts the temporary patch posted in comment:4
  • be96c4f788ab616d94457bcc4dd1d098b156d62c isolates link rel=preconnect to first party and generally isolates speculative connects
  • 8cfeded9b52a1bbc93622c6dbe6ca59f987b43c0 is a fixup for our first-party regression test that ensures that the preconnect connection has the correct first party
Last edited 3 years ago by arthuredelstein (previous) (diff)

comment:13 Changed 3 years ago by mcs

r=brade, r=mcs
Thanks for making the changes. We did not compile and run this time, but these patches look good.

comment:14 in reply to:  12 ; Changed 3 years ago by gk

Replying to arthuredelstein:

  • be96c4f788ab616d94457bcc4dd1d098b156d62c isolates link rel=preconnect to first party and generally isolates speculative connects

This means this patch is supposed to fix #18762 as well, right?

comment:15 in reply to:  14 Changed 3 years ago by arthuredelstein

Replying to gk:

Replying to arthuredelstein:

  • be96c4f788ab616d94457bcc4dd1d098b156d62c isolates link rel=preconnect to first party and generally isolates speculative connects

This means this patch is supposed to fix #18762 as well, right?

Good point. Yes, it does fix that TODO. I will do a manual test to confirm it is working soon.

comment:16 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

The patches look good to me as well. I applied them to tor-browser-45.2.0esr-6.5-1 as commit b44b2b25bab1ebde9351c842f0ca66d2fdc87579, be96c4f788ab616d94457bcc4dd1d098b156d62c and 8cfeded9b52a1bbc93622c6dbe6ca59f987b43c0. Thanks!

Note: See TracTickets for help on using tickets.