1304219 - Ensure link rel=preconnect requests are isolated by origin attributes (Tor 16998)

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Arthur Edelstein [:arthur]]

In https://trac.torproject.org/16998 we isolated <link rel="preconnect" ...> requests in Tor Browser by first party. That means that requests go over the Tor circuit associated with the first party. As part of Tor uplift, we should confirm (or implement) isolation by origin attributes and implement unit tests.

Here's the Tor patch:
https://torpat.ch/16998
And here is how we added this feature to our first-party isolation unit tests:
https://github.com/arthuredelstein/tor-browser/commit/8cfeded9b52a1bbc93622c6dbe6ca59f987b43c0

Comment 1

[Jonathan Hao (inactive) [:jhao]]

This is a follow-up patch based on the patch to uplift in bug 1264577.  Tim is working on that bug, but before he finishes I will be on PTO.  I think Tim should probably take this bug, too.

Comment 2

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Sure, I will take this bug.

Comment 3

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

It looks like that the rel=preconnect does not follow the originAttributes since the API nsISpeculativeConnect.SpeculativeConnect() [1][2], that it uses for pre-connection, is not segregated by originAttributes. And the implementation of SpeculativeConnect() uses the system principal [3] to connect, which uses default containers. 

Tanvi, Are we going to separate the rel=preconnect with originAttributes? If we do, I think we have to modify the interface of nsISpeculativeConnect.SpeculativeConnect() to take account of the principal. However, some addons use this API, this change might affect them. What do you think about this?


[1] http://searchfox.org/mozilla-central/source/dom/base/nsDocument.cpp#9272
[2] http://searchfox.org/mozilla-central/source/netwerk/base/nsISpeculativeConnect.idl#28
[3] http://searchfox.org/mozilla-central/source/netwerk/base/nsIOService.cpp#1799

Comment 4

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Attachment: Part 1: Add a nsISpeculativeConnect.speculativeConnect2() interface which takes a principal as an additional argument.

For not breaking the addons, I'd like to add a new interface, speculativeConnect2() into the nsISpeculativeConnect. This API takes a principal as an additional argument, and will use this principal to establish the channel.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 8803841 [details] [diff] [review]
Part 1: Add a nsISpeculativeConnect.speculativeConnect2() interface which takes a principal as an additional argument.

Review of attachment 8803841 [details] [diff] [review]:
-----------------------------------------------------------------

Patrick, this is a top level api change, should get review from either you or someone responsible for add-on compat.

::: netwerk/base/nsIOService.cpp
@@ +1789,5 @@
>      nsCOMPtr<nsIScriptSecurityManager> secMan(
>          do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv));
>      NS_ENSURE_SUCCESS(rv, rv);
>      nsCOMPtr<nsIPrincipal> systemPrincipal;
>      rv = secMan->GetSystemPrincipal(getter_AddRefs(systemPrincipal));

probably no longer used? (didn't check the code)

@@ +1849,5 @@
> +nsIOService::SpeculativeConnect2(nsIURI *aURI,
> +                                 nsIPrincipal *aPrincipal,
> +                                 nsIInterfaceRequestor *aCallbacks)
> +{
> +  return SpeculativeConnectInternal(aURI, aPrincipal, aCallbacks, false);

indent 4sp

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +2302,5 @@
> +    // principal. Otherwise, we use the originAttributes from the
> +    // loadContext.
> +    if (aPrincipal) {
> +        neckoOriginAttributes.InheritFromDocToNecko(
> +        BasePrincipal::Cast(aPrincipal)->OriginAttributesRef());

nit indent

Comment 6

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Attachment: Part 1: Add a nsISpeculativeConnect.speculativeConnect2() interface which takes a principal as an additional argument. r=mayhemer sr=mcmanus

Addressing the review comments, thanks Honza and Patrick

Comment 7

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Attachment: Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.

Comment 8

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

For the scope of this bug, I will only update speculativeConnect() to speculativeConnect2() for Necko and DOM. The rest of the work for updating the whole Gecko will be finished in a follow-up bug.

Comment 9

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Attachment: Part 3: Update the speculativeConnect() to speculativeConnect2() for DOM. This will make rel=preconnect using nodePrincipal to make connection.

Comment 10

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Comment on attachment 8804569 [details] [diff] [review]
Part 3: Update the speculativeConnect() to speculativeConnect2() for DOM. This will make rel=preconnect using nodePrincipal to make connection.

Henri, could you review this for me? Thanks.

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 8804532 [details] [diff] [review]
Part 1: Add a nsISpeculativeConnect.speculativeConnect2() interface which takes a principal as an additional argument. r=mayhemer sr=mcmanus

Review of attachment 8804532 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 8804544 [details] [diff] [review]
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.

Review of attachment 8804544 [details] [diff] [review]:
-----------------------------------------------------------------

Nick, please check changes in the predictor, can we reach a principal here?  for any kind of isolation I'm afraid preconnection could become useless, conn ent maps via OA too.

Anyway, I can see a lot more places that may need an update: https://dxr.mozilla.org/mozilla-central/search?q=regexp%3ASpeculative(Anonymous)%3FConnect%5C(%5B%5E%5C)%5D+path%3Anetwerk%2F&redirect=false

Comment 13

[u408661]

Comment on attachment 8804544 [details] [diff] [review]
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.

Review of attachment 8804544 [details] [diff] [review]:
-----------------------------------------------------------------

So we should be able to reach a principal during prediction - that's always fired off from docshell, which should have an appropriate principal available.

What worries me more, though, is calls to Learn - those come from various places (font loader, style loader, script loader, image loader - https://dxr.mozilla.org/mozilla-central/search?q=PredictorLearn&redirect=true) and will all have to have principals available, as well, for the predictor to continue to be useful in any way (because, as I'm understanding things, if they don't have the proper OA, they'll end up opening cache entries that aren't actually used in "real life", and so their data will be bogus). I have to imagine those loaders will have a principal available somewhere that has the proper OA for the page load, but my knowledge of OA is very limited (to say the least).

r- because, no matter what, we need to have a principal passed into predict (and we also need to get principals passed into Learn somewhere)

Comment 14

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Nick, I'd like to make the predictor using the principal, but I think it is out of the scope of this bug. So, I think we should file a bug for doing this job instead of doing here. In addition, we want to make this bug being resolved before the next nightly, and I can see that it won`t be an easy job for making predictor using the principal. Thus, I would update the SpeculativeConnect() in the predictor without changing its functionality. 

The patch is doing this job. So, could you take a look at it again? And I will file a follow-up bug. What do you think?

Comment 15

[u408661]

Comment on attachment 8804544 [details] [diff] [review]
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.

Review of attachment 8804544 [details] [diff] [review]:
-----------------------------------------------------------------

So as long as this doesn't make the predictor's work useless (in other words, speculative connections created this way can *still* be used by later connections that may be isolated by origin attributes) I'm fine with this. If not, we need to make sure the predictor properly assigns origin attributes before landing this, IMHO.

r=me assuming the condition holds true.

Comment 16

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Attachment: Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.

Comment 17

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b8d4e27e7117dc35a0cf43286e3b48503611efb

Comment 18

[Tim Huang[:timhuang] (PTO 8/25 ~ 9/2)]

Try looks good.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b03f99accc58
Part 1: Add a nsISpeculativeConnect.speculativeConnect2() interface which takes a principal as an additional argument. r=mayhemer, sr=mcmanus
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c941e929e3
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko. r=hurley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a2dc984ce6
Part 3: Update the speculativeConnect() to speculativeConnect2() for DOM. This will make rel=preconnect using nodePrincipal to make connection. r=hsivonen

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b03f99accc58
https://hg.mozilla.org/mozilla-central/rev/89c941e929e3
https://hg.mozilla.org/mozilla-central/rev/a8a2dc984ce6