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

RESOLVED FIXED in Firefox 52

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: arthuredelstein, Assigned: timhuang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla52
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [tor-testing][necko-backlog][OA-testing])

Attachments

(3 attachments, 2 obsolete attachments)

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
Whiteboard: [tor] → [tor][necko-backlog]

Updated

2 years ago
Assignee: nobody → jhao
Priority: -- → P1
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.
Assignee: jhao → nobody
Flags: needinfo?(tihuang)
(Assignee)

Comment 2

2 years ago
Sure, I will take this bug.
Assignee: nobody → tihuang
Flags: needinfo?(tihuang)

Updated

2 years ago
Whiteboard: [tor][necko-backlog] → [tor-testing][necko-backlog][OA-testing]
(Assignee)

Comment 3

2 years ago
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
Flags: needinfo?(tanvi)
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Created attachment 8803841 [details] [diff] [review]
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.
Attachment #8803841 - Flags: feedback?(honzab.moz)
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
Attachment #8803841 - Flags: superreview?(mcmanus)
Attachment #8803841 - Flags: feedback?(honzab.moz)
Attachment #8803841 - Flags: feedback+
Attachment #8803841 - Flags: superreview?(mcmanus) → superreview+
(Assignee)

Comment 6

2 years ago
Created attachment 8804532 [details] [diff] [review]
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
Attachment #8804532 - Flags: review?(honzab.moz)
(Assignee)

Updated

2 years ago
Attachment #8803841 - Attachment is obsolete: true
(Assignee)

Comment 7

2 years ago
Created attachment 8804544 [details] [diff] [review]
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.
Attachment #8804544 - Flags: review?(honzab.moz)
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1312954
(Assignee)

Comment 9

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

Comment 10

2 years ago
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.
Flags: needinfo?(hsivonen)
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!
Attachment #8804532 - Flags: review?(honzab.moz) → review+
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
Attachment #8804544 - Flags: review?(honzab.moz) → review?(hurley)
Attachment #8804569 - Flags: review+
Flags: needinfo?(hsivonen)
(Assignee)

Updated

2 years ago
Blocks: 1264577
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)
Attachment #8804544 - Flags: review?(hurley) → review-
(Assignee)

Comment 14

2 years ago
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?
Flags: needinfo?(hurley)
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.
Attachment #8804544 - Flags: review- → review+
Flags: needinfo?(hurley)

Updated

2 years ago
Flags: needinfo?(tanvi)
(Assignee)

Comment 16

2 years ago
Created attachment 8807468 [details] [diff] [review]
Part 2: Update speculativeConnect() to speculativeConnect2() for Necko.
Attachment #8807468 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8804544 - Attachment is obsolete: true
(Assignee)

Comment 18

2 years ago
Try looks good.
Keywords: checkin-needed

Comment 19

2 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 20

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b03f99accc58
https://hg.mozilla.org/mozilla-central/rev/89c941e929e3
https://hg.mozilla.org/mozilla-central/rev/a8a2dc984ce6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1316683
You need to log in before you can comment on or make changes to this bug.