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.

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.

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

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.

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.

I think this needs to be split in two.

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

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.

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.

ben - eta?

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().

(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!

(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.

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.

> 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?

(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.

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

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...

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

(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.

(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.

> 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.

(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

(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.

I asked on the SW spec issue here: https://github.com/slightlyoff/ServiceWorker/issues/719#issuecomment-149908726

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 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.

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.

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.

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.

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.

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

(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.

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

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.

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

I have no objections. Sorry for derailing things here.

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?

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...

(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!

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 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.

(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. :)

> 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.

How are things going here, Valentin?

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

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?

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

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

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

(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!

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...

(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.

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.

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.

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.

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

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?

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?

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.

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.

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

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?

MozReview-Commit-ID: 5A3KLvAaRyY

(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.

Too late for firefox 52, mass-wontfix.

(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?

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

Sounds like someone on the layout team, right?

(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?

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

I'm going to split up the existing patch.

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 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 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 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.

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?

(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.

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 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?

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?

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

Thanks!

Pushed by jwatt@jwatt.org: 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

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