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

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"

added TorBrowserTeam201606R component::applications/tor browser ff45-esr owner::arthuredelstein priority::high resolution::fixed severity::normal status::closed tbb-6.0a5 tbb-linkability type::task labels

Trac:
Keywords: N/A deleted, tbb-6.0a5 added

Trac:
Owner: tbb-team to arthuredelstein
Severity: N/A to Normal
Sponsor: N/A to N/A
Cc: N/A to arthuredelstein
Status: new to accepted
Reviewer: N/A to N/A

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

Trac:
Keywords: N/A deleted, TorBrowserTeam201604 added

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.

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.

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

Moving tickets

Trac:
Keywords: TorBrowserTeam201604 deleted, TorBrowserTeam201605 added

Trac:
Keywords: TorBrowserTeam201605 deleted, TorBrowserTeam201606 added

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.

Trac:
Status: accepted to needs_review

Trac:
Keywords: TorBrowserTeam201606 deleted, TorBrowserTeam201606R added

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?

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

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

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 (moved) as well, right?

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 (moved) as well, right?

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

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!

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

closed

mentioned in issue #18545 (moved)

mentioned in issue #18762 (moved)