1180145 - Resource timing violate SOP for "no-cors" CSS

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Anne (:annevk)]

An attacker attacker.example can figure out what resources a stylesheet target.example loads by including it on attacker.example and using either the resource timing (shipped) or service worker (about to ship) API.

This violates SOP.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Ouch.  The resource timing bit here is a bug that we should clearly fix (in our code and in the spec).  And yes, we should not expose this via service workers, imo.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Any idea how we ought to fix this? Should we wait for the spec?

Comment 3

[Anne (:annevk)]

For resource timing we should simply not surface these requests. And for service workers these requests should simply bypass the service worker. If you want cross-origin CSS and service workers, use CORS for your CSS.

Comment 4

[Andrew McCreight [:mccr8]]

Should this be split into two separate issues? It sounds like the resource timing part can be addressed. Valentin, is that something you could work on? Thanks.

Comment 5

[(no longer active)]

I think this needs to be split in two.

Comment 6

[(no longer active)]

Making this specific to resource timing.  Filing a new bug for the SW issue.

Comment 7

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

I'm a bit full at the moment, but I wrote this patch last week and didn't get to test it much. I hope it's useful.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

I think we may want some different checks here.  Do you mind waiting for me to implement bug 1201160?  That will lay a lot of groundwork.

Comment 9

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bb71252ef94

Comment 10

[Doug Turner (:dougt)]

ben - eta?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

In the next week or two.  I have to land some tainting infrastructure in bug 1212904 first.

Note, now that I'm not on my cell phone, let me list my concerns with the current patch:

1) It uses nsIHttpChannelInternal::GetCorsMode().  This is a temporary value and is going away.  Its only weakly tied to the security policy.  I wouldn't recommend its use.
2) A better short term solution is to do something like CSSStyleSheet::SubjectSubsumesInnerPrincipal().
3) I don't think this patch accurately deals with @imports.  For example, if you have a cross-origin stylesheet that @imports a same-origin stylesheet, you should not reveal data on the imported stylesheet.  The tainting should hide everything under the tainted sheet.
4) This won't work if the stylesheet was intercepted by a service worker with a CORS response.

I plan to deal with all of this in bug 1201160.  If you don't want to wait for me, you should at least use the SubjectSubsumesInnerPrincipal() approach instead of nsIHttpChannelInternal::GetCorsMode().

Comment 12

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #11)
> 
> I plan to deal with all of this in bug 1201160.  If you don't want to wait
> for me, you should at least use the SubjectSubsumesInnerPrincipal() approach
> instead of nsIHttpChannelInternal::GetCorsMode().

I'll look into doing that, but I'm not sure if nsPerformance has visibility into CSSStyleSheet via the channel.
Do you have a testcase I could use to check if I'm on the right track?

Thanks!

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Valentin Gosu [:valentin] from comment #12)
> I'll look into doing that, but I'm not sure if nsPerformance has visibility
> into CSSStyleSheet via the channel.
> Do you have a testcase I could use to check if I'm on the right track?

This is a good point and not something I noticed when looking at your patch before.

I think the stylesheet code should set a flag to opt-out of performance marking.  That way it can do the necessary security checks internally.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Oh, it also seems this patch would block marks on the cross-origin stylesheet, but really this bug is to block marks on the sub-resources loads started by that stylesheet.  I think its fine to have a performance mark the stylesheet load itself.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> but I'm not sure if nsPerformance has visibility into CSSStyleSheet via the channel.

The channel's nsILoadInfo should include the sheet principal as triggering principal, right?

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> > but I'm not sure if nsPerformance has visibility into CSSStyleSheet via the channel.
> 
> The channel's nsILoadInfo should include the sheet principal as triggering
> principal, right?

I guess, but I don't think that solves these problems:

1) How do you know the channel is being loaded as a sub-resource to a stylesheet?  Looking for "link" initiator types finds you the stylesheet itself, not something like a background-image.  I guess looking for "css" initiator type might work here, buts its unclear to me without looking closer.
2) If the triggering principal is always the stylesheet principal, then it doesn't help with the case of a same-origin @import nested within a cross-origin stylesheet.  It seems the triggering principal on sub-resources of the nested stylesheet would match the same-origin and pass.

I really think this bug would benefit from writing some tests first.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> I really think this bug would benefit from writing some tests first.

100% agreed.

Comment 18

[Boris Zbarsky [:bzbarsky]]

But good point about the same-origin import bit.

I guess the right thing is something like tainting cross-origin sheets, tainting anything they import, and then exempting loads from tainted stuff from the resource timing API...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Another interesting question here is what other browsers (Chrome, IE) do.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #19)
> Another interesting question here is what other browsers (Chrome, IE) do.

This was found during spec discussions.  I believe chrome at least are implementing.

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> But good point about the same-origin import bit.
> 
> I guess the right thing is something like tainting cross-origin sheets,
> tainting anything they import, and then exempting loads from tainted stuff
> from the resource timing API...

Yea, which I suggesting wait for bug 1201160 since I am working on all this stylesheet tainting gunk there.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> This was found during spec discussions.

Right, I understand that.  My question is what the UAs that already implement resource timing, if any, do right now.  If anything.

I just looked at the spec at https://w3c.github.io/resource-timing/#processing-model and it says:

  For each resource fetched by the current browsing context, excluding resources fetched by
  cross-origin stylesheets fetched with no-cors policy, perform the following steps: 

which doesn't match the desired behavior in cases when a cross-origin sheet fetches a same-origin sheet which fetches something, right?

Solving this together with bug 1201160 certainly makes sense.

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Boris Zbarsky [:bz] from comment #22)
> I just looked at the spec at
> https://w3c.github.io/resource-timing/#processing-model and it says:
> 
>   For each resource fetched by the current browsing context, excluding
> resources fetched by
>   cross-origin stylesheets fetched with no-cors policy, perform the
> following steps: 
> 
> which doesn't match the desired behavior in cases when a cross-origin sheet
> fetches a same-origin sheet which fetches something, right?

Right, but obviously I think thats inadequate for the intended goal here.  The right way to do this would be for CSS spec to define the concept of a tainted sheet and let other specs like performance/sw then reference that concept.

For what its worth, the chrome guys did agree with my interpretation in this hidden issue:

  https://code.google.com/p/chromium/issues/detail?id=532374

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #23)
> For what its worth, the chrome guys did agree with my interpretation in this
> hidden issue:
> 
>   https://code.google.com/p/chromium/issues/detail?id=532374

Err... scratch that.  I asked them a different question before.  I will ask them now about the nested imports.

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

I asked on the SW spec issue here:

  https://github.com/slightlyoff/ServiceWorker/issues/719#issuecomment-149908726

Comment 26

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

Ben, I tried using SubjectSubsumesInnerPrincipal, but this patch makes the wpt tests for resource timing fail. It seems it fails for every css load. Am I initializing the jsapi with the right object? Thanks

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8677209 [details] [diff] [review]
bug1180145-resourcetiming-nocors.patch

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

::: layout/style/Loader.cpp
@@ +1701,5 @@
>        }
> +
> +      AutoJSAPI jsapi;
> +      jsapi.Init(aLoadData->mSheet->GetDocument()->GetScopeObject());
> +      if (NS_FAILED(aLoadData->mSheet->SubjectSubsumesInnerPrincipal())) {

You probably need to call this in the channel OnStartRequest().  I doubt the sheet principal is set here before the channel has been opened.

Also, I'm still not sure this is what we want.  I think this is checking if the sheet itself is tainted, but we really want to know if the parent sheet loading the resource is tainted.  That means setting this, for example, on the image channel down in imgLoader.

Comment 28

[Valentin Gosu [:valentin] (he/him)]

Ben, I'm not sure we can use SubjectSubsumesInnerPrincipal in this case.
The problem is that the method will return a failure code if mComplete==false, but the resource timing entry will be added to the resourceTiming list just before the channel calling OnStopRequest, because the timing entry is sometimes checked in the resource's onload handler.
So SubjectSubsumesInnerPrincipal doesn't work before OnStopRequest, and we can't add the entry after calling OnStopRequest.
I guess there could be a hacky way of removing the resource from the list after OnStopRequest if needed, but that would probably defeat the purpose.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

I still recommend waiting until I can sort out the tainting corner cases.  I don't think this is super critical.  For example, see Jonas's comments here:

  https://github.com/slightlyoff/ServiceWorker/issues/719#issuecomment-150665556

I personally still think we should fix it, but its not very exploitable and stylesheets leak tons of information already.

So if we could just wait a few weeks, that would be best IMO.

Comment 30

[Ben Kelly [:bkelly, not reviewing]]

Note, its a bit unclear how some of the corner cases should be implemented.  See this IRC conversation:

  http://logs.glob.uno/?c=freenode%23whatwg#c974181

We should wait to implement this until we can get clarity on the details.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Ben, Valentin: I am going through 44+ tracked bugs and noticed that this one hasn't had much activity for a while. Given that it's a sec-high issue, I hope we can get a fix uplifted to Aurora44 soon.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

This will not be implemented for 44 or 45.  We should mark it as wontfix for those releases.  We need spec input before proceeding.

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ben Kelly [:bkelly] from comment #32)
> This will not be implemented for 44 or 45.  We should mark it as wontfix for
> those releases.  We need spec input before proceeding.

Ok. Thanks! Updating status-firefox44 and 45 to reflect that. Please let me know if there are any concerns.

Comment 34

[Andrew McCreight [:mccr8]]

Ben, is there something we can do here? I don't see any further activity in the linked spec bug.

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

The spec discussion is kind of stalled for service workers.  I personally don't think the resource timing spec language is adequate for all the corner cases.  Jonas has argued over in bug 1201160 that we should not worry about these issues.

I'm not sure what to do here.  I guess we could implement the resource timing spec language even if it is missing some corner cases.

Comment 36

[Valentin Gosu [:valentin] (he/him)]

Does this mean we can go forward with attachment 8656817 [details] [diff] [review] without worrying about tainting?

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

I have no objections.  Sorry for derailing things here.

Comment 38

[Anne (:annevk)]

Comment on attachment 8677209 [details] [diff] [review]
bug1180145-resourcetiming-nocors.patch

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

::: dom/tests/mochitest/general/resource_timing_main_test.html
@@ +203,5 @@
>        "The initiator type for " + resourceName + " is invalid");
>    }
>  
> +  is(window.performance.getEntriesByName("http://test1.example.com/tests/SimpleTest/test.css").length, 0,
> +     "CORS CSS resource should not be added to the entry list");

Did you mean "no-cors" here? Because if CSS is fetched through "cors", it's fine to share the URLs of its subresources.

Comment 39

[Valentin Gosu [:valentin] (he/him)]

(In reply to Anne (:annevk) from comment #38)
> Comment on attachment 8677209 [details] [diff] [review]
> bug1180145-resourcetiming-nocors.patch
> 
> Review of attachment 8677209 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/tests/mochitest/general/resource_timing_main_test.html
> @@ +203,5 @@
> >        "The initiator type for " + resourceName + " is invalid");
> >    }
> >  
> > +  is(window.performance.getEntriesByName("http://test1.example.com/tests/SimpleTest/test.css").length, 0,
> > +     "CORS CSS resource should not be added to the entry list");
> 
> Did you mean "no-cors" here? Because if CSS is fetched through "cors", it's
> fine to share the URLs of its subresources.

Yes, that's a typo.

Comment 40

[Valentin Gosu [:valentin] (he/him)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=002650c41883

Comment 41

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

Comment 42

[Valentin Gosu [:valentin] (he/him)]

resource_timing_nocors.html has a few tests for this, and a nice load graph to mark which resources should be visible.
Please let me know if I missed any case, or if any of those included are wrong.

Also, I don't know if this needs to be sec-high. This is a publicly known issue, and as Ben and Jonas mentioned isn't that much of a problem. I'd suggest sec-low.

Anne, I took the phrase "cross-origin stylesheets fetched with no-cors policy" to mean if the domains don't match and we have CORS_MODE_NO_CORS on the channel we should hide the request. Is that correct, or should we take the Access-Control-Allow-Origin header into account?

Comment 43

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

Ooops, forgot to cleanup commented code.

Comment 44

[Anne (:annevk)]

1. It's not the stylesheet that needs to be hidden, but its subresources (if the stylesheet is cross-origin and fetched with mode "no-cors").
2. It needs to be an origin comparison, not a domain comparison.
3. I don't know enough about Gecko networking to know if CORS_MODE_NO_CORS maps to Fetch's mode "no-cors", but it sure sounds like it...

Comment 45

[Valentin Gosu [:valentin] (he/him)]

(In reply to Anne (:annevk) from comment #44)
> 1. It's not the stylesheet that needs to be hidden, but its subresources (if
> the stylesheet is cross-origin and fetched with mode "no-cors").
> 2. It needs to be an origin comparison, not a domain comparison.
> 3. I don't know enough about Gecko networking to know if CORS_MODE_NO_CORS
> maps to Fetch's mode "no-cors", but it sure sounds like it...

I didn't do a good job of explaining, but it seems it is correct. Thanks!

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8727020 [details] [diff] [review]
bug1180145-resourcetiming-nocors.patch

This needs a commit message explaining what the patch is actually trying to do (including spec links, if needed).  Incidentally, that would actually make it possible to review without telepathy...

Comment 47

[Valentin Gosu [:valentin] (he/him)]

Attachment: [Resource timing] Don't include resources loaded by a cross-origin sylesheet fetched with a no-cors policy

Added patch description and a couple more comments to the patch.
In Loader.cpp it marks if the stylesheet is cross-origin and no-cors. If so, every load it triggers will not be reported by resource timing.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8728670 [details] [diff] [review]
[Resource timing] Don't include resources loaded by a cross-origin sylesheet fetched with a no-cors policy

Sorry for the lag; needed time to sort through this.

>+++ b/dom/tests/mochitest/general/mochitest.ini

The newly added support files need much better names.  Like maybe ones that mean something.  e.g. cssA.css could be emptyCssFile1.css.  Once that's done it will become clear that cssB.css is quite different from cssA.css and cssC.css.

Also, the distinction between css*.css and link*.css would hopefully become clearer too.

>+++ b/dom/tests/mochitest/general/resource_timing_nocors.html

The convention is to call this file_resource_timing_nocors.html

>+  is(entries.length, Object.keys(allResources).length, "Found wrong number of resources");

This error message would be more useful if every time we had a match while doing our loop we deleted the corresponding property from allResources and here checked that Object.keys(allResources).length is 0 and if it's not included JSON.stringify(allResources) in the error message.

Either way, though.

Do we not also need a test that we _do_ include subresources if the cross-origin sheet was fetched via CORS?

>+++ b/layout/style/Loader.cpp
>+        // If this load was triggered by a cross-origin stylesheets

s/stylesheets/stylesheet/

In general the code here seems to be wrong, though good enough for this patch (because the page can't manipulate the CSSOM of no-cors cross-origin sheets).  This code is assuming that aLoadData->mParentData being null means this is not an @import load, but that assumption is flat-out false.  Please file a followup bug: the failing case is adding an @import rule via CSSOM to an already-parsed sheet; we will set the initiator type of the load to "link" as far as I can see, whereas it should be "css".

>+      rv = secMan->GetChannelResultPrincipal(channel, getter_AddRefs(channelPrincipal));

No, this won't do what you want, because you haven't done the load yet.

For example, say you do a no-cors load from your own site, but send a 3xx response redirecting off-site.  We should treat that sheet as cross-origin for purposes of the subresource loads its makes, but getting the channel principal here will return a same-origin principal.

You should probably move all this stuff to the place where we get back the data and know something about where the sheet actually came from.  Incidentally, we already GetChannelResultPrincipal there, for other reasons.



>+      // If the is a cross-origin stylesheets fetched with no-cors policy, set a

s/the is/this is/?  And also "stylesheet", not "stylesheets".

>+      if (internal && NS_SUCCEEDED(internal->GetCorsMode(&mode)) &&

What's wrong with aLoadData->mSheet->GetCORSMode()?

>+          !channelPrincipal->Subsumes(aLoadData->mLoaderPrincipal)) {

Er... why is this a Subsumes() check?  That doesn't seem right at all, in general: it would report timing for a privileged sheet being loaded into an unprivileged document (if our privileged stuff supported timing, that is).

>+++ b/netwerk/base/nsITimedChannel.idl
>+  // Marks if this resource should not be reported in resource timing

How about: // If this attribute is false, this resource MUST NOT be reported in resource timing.

This doesn't seem to address non-stylesheet subresources loaded from stylesheets, right?  Was that an oversight, or is there a reason we're not handling those?

Anyway, to handle those and @import after initial parse correctly, the "don't report timing for loads from this sheet" state needs to be stored in the sheet itself, not in SheetLoadData.  And we'll need to check for a parent sheet in LoadSheet or something.

There is also a nasty edge case here: Say we have a page that loads "same-origin.css" and "cross-origin.css", with the latter being a cross-origin load.  Then "cross-origin.css" imports "same-origin-2.css" (which is same-origin with the original page) and then both "same-origin.css" and "cross-origin.css" import "shared.css".

In this situation, will we report timing for "shared.css"?  It depends on the order in which all those loads race, because as far as I can tell we would share a single channel for the loads (or a single preloaded sheet if there is one).

Which means that we should probably be including this "report timing for this load" state in the hashtable key we use for looking up sheets in Loader::CreateSheet().

Again, sorry it took me so long to get to this.  Future reviews should be faster now that I don't have to disentangle so many bits, hopefully.

Comment 49

[Valentin Gosu [:valentin] (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #48)
> You should probably move all this stuff to the place where we get back the
> data and know something about where the sheet actually came from. 
> Incidentally, we already GetChannelResultPrincipal there, for other reasons.
> 

The problem is that Loader::OnStreamComplete is called after nsPerformance::AddEntry. So can't call SetReportResourceTiming(false) there, because it wouldn't affect anything.
Could we do that in OnDetermineCharset? It's the closest think to OnStartRequest that I can make out.

> This doesn't seem to address non-stylesheet subresources loaded from
> stylesheets, right?  Was that an oversight, or is there a reason we're not
> handling those?

An oversight, given I didn't know that was allowed. Is this also done with an @import? Could you point me to the code that handles this?

> Anyway, to handle those and @import after initial parse correctly, the
> "don't report timing for loads from this sheet" state needs to be stored in
> the sheet itself, not in SheetLoadData.  And we'll need to check for a
> parent sheet in LoadSheet or something.

I missed that we reused already parsed stylesheets.

> There is also a nasty edge case here: Say we have a page that loads
> "same-origin.css" and "cross-origin.css", with the latter being a
> cross-origin load.  Then "cross-origin.css" imports "same-origin-2.css"
> (which is same-origin with the original page) and then both
> "same-origin.css" and "cross-origin.css" import "shared.css".
> 
> In this situation, will we report timing for "shared.css"?  It depends on
> the order in which all those loads race, because as far as I can tell we
> would share a single channel for the loads (or a single preloaded sheet if
> there is one).
> Which means that we should probably be including this "report timing for
> this load" state in the hashtable key we use for looking up sheets in
> Loader::CreateSheet().

Could you maybe outline how I could do that? I'm not very familiar with this code.
Also, if you think someone else is more appropriate to write this patch, let me know. I have a few big blind spots regarding how the stylesheet loader works. :)

Comment 50

[Boris Zbarsky [:bzbarsky]]

> The problem is that Loader::OnStreamComplete is called after nsPerformance::AddEntry.

So?  In OnStreamComplete is where you would mark the cross-origin no-CORS sheet as "anything loaded from this sheet needs to not add performance timing entries".  You _do_ want to add entries for that sheet itself, though.  So the fact that they're added before this point is not a problem, because you _do_ want to add them.

> Is this also done with an @import?

Not at all, sorry.

> Could you point me to the code that handles this?

1)  The css::ImageValue::ImageValue constructor handles background images 
    and generated content images and -moz-image-rect and so forth.
2)  XBL loading, though I expect we don't care about that here.
3)  Various SVG things like markers and masks; these probably go through
    nsIDocument::RequestExternalResource if they lead to a load at all.  
4)  Clip paths.  I don't know offhand where those are handled; maybe those are
    in the "SVG things" bucket.  Needs checking.
5)  CSS cursors.  Those _might_ go through ImageValue, but I don't know for sure.
6)  Webfonts.  Looks like the entrypoint into that stuff from CSS is
    FontFaceSet::FindOrCreateUserFontEntryFromFontFace and the actual load probably happens
    somewhere in nsFontFaceLoader.

You'd need to thread this state through css::URLValue through to those various places that use it.

> Could you maybe outline how I could do that?

Change URIPrincipalReferrerPolicyAndCORSModeHashKey to include the data you want (and probably rename it a bit) and then make sure you provide that data whenever you construct one.

> Also, if you think someone else is more appropriate to write this patch, let me know. 

Well, me or heycam or dbaron would be obvious options, but I'm pretty swamped with other things right now.  I can't speak for heycam and dbaron.

Comment 51

[Andrew Overholt [:overholt]]

How are things going here, Valentin?

Comment 52

[Valentin Gosu [:valentin] (he/him)]

I have stalled on this bug due to other work issues. Will try to pick it up again tomorrow.

Comment 53

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

Comment 54

[Valentin Gosu [:valentin] (he/him)]

Attachment: bug1180145-resourcetiming-nocors.patch

Comment 55

[Valentin Gosu [:valentin] (he/him)]

I cleaned up the patch a bit, and added tests for some of the cases :bz mentioned - image, -moz-image-rect, font, cursor.
I'm still having trouble fixing these cases. David, Cameron, can one of you take this bug instead?

Comment 57

[Andrew Overholt [:overholt]]

Let's move this to Layout since that's where the remaining work is.

Comment 58

[Anne (:annevk)]

Just to be clear, if we do not also fix bug 1201160 we're not actually closing the leak.

Comment 59

[Andrew McCreight [:mccr8]]

Jet, could you find somebody to get some forward progress on this 5 month old sec-high issue? Thanks.

Comment 60

[Jet Villegas (inactive)]

(In reply to Valentin Gosu [:valentin] from comment #55)
> I cleaned up the patch a bit, and added tests for some of the cases :bz
> mentioned - image, -moz-image-rect, font, cursor.
> I'm still having trouble fixing these cases. David, Cameron, can one of you
> take this bug instead?

I can help get this sorted. 

IIUC, we're generally OK with the fix in the patch for layout/style/Loader.cpp except that we're still incorrectly reporting Performance values for image, -moz-image-rect, font, and cursor. 

Is that what I need to get fixed, or are there other things to catch? Thx!

Comment 61

[Anne (:annevk)]

dveditz, does it make sense to fix this while bug 1201160 is not fixed? It seems if we don't fix one, we might as well not fix the other and just enshrine yet another same-origin policy exception into the platform...

Comment 62

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #61)
> dveditz, does it make sense to fix this while bug 1201160 is not fixed? It
> seems if we don't fix one, we might as well not fix the other and just
> enshrine yet another same-origin policy exception into the platform...

Well, the performance WG has changed their spec to fix this.  The service worker spec, however, has not made the change.

IMO the performance WG spec change was inadequate to really fix it, but we could at least conform to it.  It just completely ignores things like @import(), etc.

Comment 63

[Anne (:annevk)]

The argument shouldn't revolve around whether there is a specification update. Either we think this is a risk for users or we don't. If we think it's a risk, we need to fix both bugs (and get specifications updated, if needed). If we don't, we might still want to change something because of a specification saying a certain thing, but it's no longer a security issue or high priority.

Comment 64

[Ben Kelly [:bkelly, not reviewing]]

Without buy-in from chrome we will pay the cost of the compatibility hit for this.  Most people test service workers on chrome first and often not on firefox at all.  IMO we should not implement a SW change related to this without google's commitment to do the same.

This will be a compat problem because its taken so long to get consensus.  The current implementation has been out for over a year.  Also its been described in public posts on github during that time, so I doubt the value of keeping this private.

Comment 65

[Jet Villegas (inactive)]

After reading the comments on bug 1201160, it seems like any security we gain by hiding CSS loads from the performance API (this bug) is easily thwarted by allowing those same resource loads from serviceworkers--a more reliable attack than this timing one.

That said, I'm inclined to take the partial fix here, even if it doesn't catch all the cases. I think we should still hide image and font loads before shipping the fix though.

Comment 66

[Stephanie Ouillon [:arroway] (needinfo me)]

Jet, since there is a partial fix identified here, could somebody be assigned to this sec-high bug?

Comment 67

[Jonathan Watt [:jwatt]]

Given that bug 1201160 was un-hidden due to public discussion on the spec bug (see bug 1201160 comment 57), does it make sense to do the same here, Dan?

Comment 68

[Jet Villegas (inactive)]

I can be convinced to land this and file follow-ups for the remaining work. We still need the original patch author to confirm my questions about missing pieces on comment 60. 

Boris: WDYT?

Comment 69

[Valentin Gosu [:valentin] (he/him)]

Hi Jet, you are correct, I didn't manage to fix image, -moz-image-rect, font, cursor.
Boris will know if there are also others cases which need fixing.
Please note that the patch still needs to be reviewed.

Comment 70

[Boris Zbarsky [:bzbarsky]]

It's really unfortunate that we have to whack-a-mole this at multiple callsites somehow...  :(  But yes, modulo that fixing this piecemeal is probably ok.

Comment 71

[Stephanie Ouillon [:arroway] (needinfo me)]

Sec-high triage: is there anything preventing doing the review of the submitted patch? Who should be assigned to this bug?

Comment 72

[Jet Villegas (inactive)]

Comment on attachment 8741736 [details] [diff] [review]
bug1180145-resourcetiming-nocors.patch

Un-obsoleting this patch so it can be rebased, reviewed, and landed. Valentin: do you have cycles to unbitrot this one?

Comment 73

[Valentin Gosu [:valentin] (he/him)]

Attachment: [Resource timing] Don't include resources loaded by a cross-origin sylesheet fetched with a no-cors policy

MozReview-Commit-ID: 5A3KLvAaRyY

Comment 74

[Valentin Gosu [:valentin] (he/him)]

(In reply to Jet Villegas (:jet) from comment #72)
> Valentin: do you have cycles to unbitrot this one?
I rebased the patch, but I don't have the cycles to implement the rest of it and get it landed.

Comment 75

[Julien Cristau [:jcristau]]

Too late for firefox 52, mass-wontfix.

Comment 76

[Frederik Braun [:freddy]]

(In reply to Valentin Gosu [:valentin] from comment #74)
> (In reply to Jet Villegas (:jet) from comment #72)
> > Valentin: do you have cycles to unbitrot this one?
> I rebased the patch, but I don't have the cycles to implement the rest of it
> and get it landed.

Who would be a good person to continue this work?

Comment 77

[Valentin Gosu [:valentin] (he/him)]

Someone with some experience in the image, -moz-image-rect, font, cursor. Not sure who that would be.

Comment 78

[Boris Zbarsky [:bzbarsky]]

Sounds like someone on the layout team, right?

Comment 79

[Jet Villegas (inactive)]

(In reply to Valentin Gosu [:valentin] from comment #77)
> Someone with some experience in the image, -moz-image-rect, font, cursor.

I added bug 1357966 to cover that work. Likely won't start until after Stylo.

jwatt: Can we redirect the review of attachment 8850887 [details] [diff] [review] to you?

Comment 80

[Frederik Braun [:freddy]]

jwatt, this is a security bug and it needs attention. Will you review this?

Comment 81

[Jonathan Watt [:jwatt]]

I'm going to split up the existing patch.

Comment 82

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add a 'reportResourceTiming' member to the nsITimedChannel interface

Comment 83

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource timing reporting

Comment 84

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Block resource timing reporting for channels that are tainted

Comment 85

[Jonathan Watt [:jwatt]]

Attachment: part 4 - tests

These need split up to only land the tests that currently pass. More implementation work to fix various edge cases is needed to land others.

Comment 86

[Cameron McCormack (:heycam)]

Comment on attachment 8884702 [details] [diff] [review]
part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource timing reporting

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

::: layout/style/Loader.cpp
@@ +1728,5 @@
> +          // Set a flag so any other stylesheet triggered by this one will
> +          // not be reported
> +          aLoadData->mReportTiming = false;
> +
> +          // Mark the channel so nsPerformance will not report the resource

Should "nsPerformance" be something else?

@@ +1729,5 @@
> +          // not be reported
> +          aLoadData->mReportTiming = false;
> +
> +          // Mark the channel so nsPerformance will not report the resource
> +          timedChannel->SetReportResourceTiming(false);

This prevents resource timing being reported for child sheets (from @import rules) when the parent sheet was loaded cross origin with CORS disabled.  Where do we prevent timing being reported for the load of the parent sheet itself?

Comment 87

[Honza Bambas (:mayhemer)]

Comment on attachment 8884701 [details] [diff] [review]
part 1 - Add a 'reportResourceTiming' member to the nsITimedChannel interface

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

r+ with the format mistakes fixed

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +3971,5 @@
>    return docPerformance;
>  }
>  
> +NS_IMETHODIMP
> +HttpBaseChannel::SetReportResourceTiming(bool enabled) {

{ on a new line please

::: netwerk/protocol/http/NullHttpChannel.cpp
@@ +847,5 @@
>    return NS_ERROR_NOT_IMPLEMENTED;
>  }
>  
> +NS_IMETHODIMP
> +NullHttpChannel::SetReportResourceTiming(bool enabled) {

an in this file too

Comment 88

[Cameron McCormack (:heycam)]

Comment on attachment 8884702 [details] [diff] [review]
part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource timing reporting

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

Cancelling review while awaiting an answer to comment 86.

Comment 89

[Frederik Braun [:freddy]]

Jonathan, stylo has landed in Nightly. Any chance we can start moving this forward? ;-)
Cameron cancelled the review for your 2nd patch, waiting for his question in comment 86 (quoted below for your convenience) to be answered by you.


(In reply to Cameron McCormack (:heycam) from comment #86)
> Comment on attachment 8884702 [details] [diff] [review]
> part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource
> timing reporting
> 
> Review of attachment 8884702 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/style/Loader.cpp
> @@ +1728,5 @@
> > +          // Set a flag so any other stylesheet triggered by this one will
> > +          // not be reported
> > +          aLoadData->mReportTiming = false;
> > +
> > +          // Mark the channel so nsPerformance will not report the resource
> 
> Should "nsPerformance" be something else?
> 
> @@ +1729,5 @@
> > +          // not be reported
> > +          aLoadData->mReportTiming = false;
> > +
> > +          // Mark the channel so nsPerformance will not report the resource
> > +          timedChannel->SetReportResourceTiming(false);
> 
> This prevents resource timing being reported for child sheets (from @import
> rules) when the parent sheet was loaded cross origin with CORS disabled. 
> Where do we prevent timing being reported for the load of the parent sheet
> itself?

Comment 90

[Jonathan Watt [:jwatt]]

(In reply to Frederik Braun [:freddyb] from comment #89)
> Jonathan, stylo has landed in Nightly. Any chance we can start moving this
> forward? ;-)
> Cameron cancelled the review for your 2nd patch, waiting for his question in
> comment 86 (quoted below for your convenience) to be answered by you.

Yeah, I'll re-request review after 57 has shipped.

Comment 91

[Wennie]

Hi Jet:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 92

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource timing reporting

Comment 93

[Cameron McCormack (:heycam)]

Comment on attachment 8928343 [details] [diff] [review]
part 2 - Have CSSLoader taint no-cors sub-resource loads to block resource timing reporting

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

::: layout/style/Loader.cpp
@@ +747,5 @@
> +
> +    nsCOMPtr<nsITimedChannel> timedChannel(do_QueryInterface(httpChannel));
> +    if (timedChannel) {
> +      // If this load was triggered by a cross-origin stylesheets
> +      // fetched with no-cors policy we must not report this resource

Maybe:

// If this load is for a cross origin style sheet fetched
// with a no-cors policy, we must not report this resource.

otherwise it sounds a bit like the load of this sheet was triggered by a another, cross origin style sheet.

@@ +1577,5 @@
>        if (aLoadData->mParentData) {
>          timedChannel->SetInitiatorType(NS_LITERAL_STRING("css"));
> +
> +        // If this load was triggered by a cross-origin stylesheets
> +        // fetched with no-cors policy we must not report this resource

Mention that this is specifically for handling child sheets, something like:

// This is a child sheet load.  If this child sheet itself is a
// cross origin load with no-cors policy, then we will have
// disabled resource timing reporting above in VerifySheetReadyToParse.
// Here we also disable reporting for any child sheet of one that
// had its reporting disabled.  So same origin sheets that are
// children of cross origin sheets will also have their reporting
// disabled.

@@ +1584,5 @@
> +          // not be reported
> +          aLoadData->mReportTiming = false;
> +
> +          // Mark the channel so PerformanceMainThread::AddEntry will not report
> +          // the resource.

Maybe copy this comment to the call in VerifySheetReadyToParse too?

Comment 94

[Cameron McCormack (:heycam)]

Regarding the inheritance of the mReportTiming = false into all child sheet loads, not just cross origin child sheets, can you either verify that's what the spec requires, or file an issue on the relevant spec to clarify?

Comment 95

[Cameron McCormack (:heycam)]

(needinfo? in case this fell off your radar.)

Comment 96

[Jonathan Watt [:jwatt]]

Thanks!

Comment 97

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72f1748e5d1c
part 1 - Add a 'reportResourceTiming' member to the nsITimedChannel interface. r=mayhemer
https://hg.mozilla.org/integration/mozilla-inbound/rev/38af716749fc
part 2 - Taint style sheet sub-resources of no-cors resources against resource timing reporting. r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0cce8da31164
part 3 - Block resource timing reporting for channels that are tainted. r=jwatt
https://hg.mozilla.org/integration/mozilla-inbound/rev/add10fa49660
part 4 - Tests for blocking of resource timing for sub-resources of no-cors resources. r=jwatt

Comment 98

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/72f1748e5d1c
https://hg.mozilla.org/mozilla-central/rev/38af716749fc
https://hg.mozilla.org/mozilla-central/rev/0cce8da31164
https://hg.mozilla.org/mozilla-central/rev/add10fa49660