New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing CSS subresource URLs #70

Closed
toddreifsteck opened this Issue Oct 12, 2016 · 52 comments

Comments

Projects
None yet
10 participants
@toddreifsteck
Copy link
Member

toddreifsteck commented Oct 12, 2016

See #27

@plehegar

This comment has been minimized.

Copy link
Member

plehegar commented Oct 12, 2016

The specific test needs to test for resource exclusion:
"For each resource fetched by the current browsing context, excluding resources fetched by cross-origin stylesheets fetched with no-cors policy,"

@plehegar plehegar self-assigned this Oct 12, 2016

@plehegar plehegar added this to the Level 1 milestone Oct 12, 2016

@toddreifsteck toddreifsteck modified the milestones: Level 1, Level 2 Jan 18, 2018

@toddreifsteck

This comment has been minimized.

Copy link
Member

toddreifsteck commented Jan 18, 2018

Adding @yoavweiss as assignee per call on 1/18/2018.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Jan 31, 2018

Submitted a test PR, but all browsers I tested in seem to be failing it in one way or another.(Admittedly, didn't test Edge...)

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Feb 22, 2018

As it seems like browsers expose this information right now, as well as intending to expose it per spec through Service Workers, I'm not sure what the reasoning is to bar that information from Resource Timing.

Note that there are several major use cases for this info (e.g. measuring loading of Google Fonts).

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Apr 24, 2018

This is blocked by w3c/ServiceWorker#719

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented May 16, 2018

I don't see implementer interest in aligning to the spec in Chromium: https://bugs.chromium.org/p/chromium/issues/detail?id=532399

Unless other implementers feel otherwise and want to change their current implementations, I feel we should align the spec to the implementations, and remove the "excluding resources fetched by cross-origin stylesheets fetched with no-cors policy" language.

@toddreifsteck @rniwa @digitarald - thoughts?

@toddreifsteck

This comment has been minimized.

Copy link
Member

toddreifsteck commented May 17, 2018

I generally agree with @yoavweiss 's conclusion

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented May 18, 2018

@past - thoughts from Mozilla?

@past

This comment has been minimized.

Copy link

past commented May 18, 2018

My understanding is that we consider the service worker information exposure a bug in the spec, is this correct @annevk / @wanderview?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 19, 2018

Yeah.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 19, 2018

I'm a bit appalled I wasn't asked for input on this directly given that I raised the security issue to begin with... Seems rather disingenuous.

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented May 19, 2018

Let's keep this collaborative. I don't believe there are any disingenuous intentions here on anyones behalf. This is a long-outstanding issue that (a) is tied to w3c/ServiceWorker#719 that everyone on this thread has been trying to help resolve and nudge along. In absence of clear signals on that thread and in light of feedback in https://bugs.chromium.org/p/chromium/issues/detail?id=532399, I think the raised proposal to remove this wording from RT is worth considering.

If we feel strongly that the SW-exposure is a bug, can we prioritize the work to resolve that in Fetch?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 20, 2018

@igrigorik it's not an issue with Fetch as far as I know. One issue is that CSS doesn't define how it fetches. And one issue is that resource timing is not grounded in web platform architecture and therefore it's unclear how it works.

@wanderview

This comment has been minimized.

Copy link
Member

wanderview commented May 20, 2018

In particular, I still don't think anyone has proposed a solution that completely handles the various @imports cases. I raised this back in w3c/ServiceWorker#719 (comment).

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented May 24, 2018

@annevk the folks I included in my latest message were mostly WebPerfWG folks which were largely present in our latest discussion on this. I missed the fact that you initiated the original security issue, so failed to include you directly.

FWIW, I'm not suggesting here that this issue is not important and should not be solved. I already created the test for it.

At the same time, this SOP hole has been open for the last ~6 years with Resource Timing, and for the last ~3 with Service Workers. This is true for all implementing browsers, so all modern ones. (as well as some not-so-modern ones).
Browser implementations don't seem interested in solving this issue, and on my end, I agree that it makes no sense to cripple one API if the info is happily exposed on another.

So, I suggest to remove the relevant spec language from ResourceTiming, and align the spec with the shipped reality of the web. If at some point the Service Worker issue will be resolved, we can easily bring back that spec language as well as the tests, and more importantly, implement the restriction.

In particular, I still don't think anyone has proposed a solution that completely handles the various @imports cases. I raised this back in w3c/ServiceWorker#719 (comment).

@wanderview - indeed the various import cases will need to be better defined in RT as well if we decide to keep the restriction.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 24, 2018

@yoavweiss https://bugzilla.mozilla.org/show_bug.cgi?id=1180145 suggests it's fixed in Firefox as far as Resource Timing is concerned. The last time Chrome's security team said something in public about this they also wanted to fix this. Has that changed?

We keep running into new security issues with "no-cors", Spectre being a pretty recent and bad case of that. I think we need a much more compelling reason to enshrine SOP violations into the platform than nobody having bothered to fix their issues (which isn't completely true as far as Firefox is concerned).

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented May 24, 2018

Firefox is failing the related WPT test, but looking deeper, it does seem like only the font is exposed, while the image is not...

@annevk

This comment has been minimized.

Copy link
Member

annevk commented May 24, 2018

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented May 24, 2018

OK, great! Is there interest from Firefox to tackle the SW piece as well? Any timelines on that front you can share?

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Aug 22, 2018

@annevk the rational behind the decision is that RT doesn't expose anything that SW isn't already exposing. Once SWs change their behavior, RT will follow. But the current aspirational spec language does not match most RT implementations, and does not make sense in light of the SW situation.

Looking at your past comment:

We are interested, but as we stated in the relevant issue we want to coordinate deployment with other browsers to take the compat risk together.

Have you had any traction with that? Is there reason to believe the situation will change in the near future?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 22, 2018

I don't think rationale of the type "we have a security hole in A so it's acceptable to have the same hole in B" is compelling. Especially since you plan on reverting if the hole is closed in A. You'll just end up making that harder.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Aug 22, 2018

My understanding is that:

  1. There's disagreement that exposing cross-origin CSS subresource URLs is indeed a security hole, and assuming it is, there's disagreement regarding its severity.
  2. As a result, there's lack of implementer appetite to resolve this issue.
  3. A "fix" for this was added to the RT spec, but largely not implemented in the 3 years since. I appreciate that Firefox made some progress on that issue, but it still doesn't pass the related tests.
  4. Even if all implementers would now implement the restriction for RT, users will still have that information exposed in SW in all implementations.
  5. The SW issue have been open for over 3 years, and haven't seen any movement in the last ~7 months.
  6. In the current state, we cannot ship Resource-Timing to L2, as the aspirational CSS subresource limitation doesn't represent the feature implementations that are out there.

Do you disagree on any of the points above? Do you predict the SW situation to change in the near future?

If the answer to the above questions is "no", keeping this restriction in RT basically means holding the feature hostage for the foreseeable future. During the last call, the group agreed that this is not desirable.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Aug 22, 2018

I disagree with the coupling with service workers. I also continue to be surprised that this means you cannot ship, whereas defining the feature without grounding in primitives (i.e., Fetch) is totally fine.

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 23, 2018

I think we need to unexposed non-CORS cross-origin CSS requests from resource timing. If the implementation is the only thing that's holding back, then I'd say we're willing to make the code change in WebKit.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Aug 23, 2018

@rniwa I thought we agreed on the call that as long as this information is exposed in SW, it doesn't make sense to unexpose it in RT. Are you suggesting that you'd be willing to make implementation progress on unexposing it in SW?

@rniwa

This comment has been minimized.

Copy link

rniwa commented Aug 23, 2018

Yes, we should simply fix service workers now that we're aware of the issue. Meanwhile, there is no need to introduce a new security problem into resource timing specification. We should be fixing the implementations instead.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 28, 2018

@youennf - any news on that front following our TPAC breakout session?

@youennf

This comment has been minimized.

Copy link

youennf commented Nov 28, 2018

@yoavweiss, we plan to fix the resource timing issue as soon as possible.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 28, 2018

@yoavweiss, we plan to fix the resource timing issue as soon as possible.

As well as service workers?

@youennf

This comment has been minimized.

Copy link

youennf commented Nov 28, 2018

@yoavweiss, we plan to fix the resource timing issue as soon as possible.

As well as service workers?

We also want to fix this and are discussing the best way to proceed.
Ideally there should be a way for web developers to import CSS stylesheets using CORS.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 29, 2018

Ideally there should be a way for web developers to import CSS stylesheets using CORS.

Maybe the request mode should be transitive for CSS imports?

@domenic

This comment has been minimized.

Copy link

domenic commented Nov 29, 2018

We did something similar for JS modules, whose credentials mode can be controlled at top level via an attribute on the script element, but then propagates to all imports in the graph (in an automatic way that cannot be controlled).

@youennf

This comment has been minimized.

Copy link

youennf commented Nov 29, 2018

Yep, that would also be my preferred option, but I do not know how much breakage that would cause. Any insight?

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 29, 2018

Yep, that would also be my preferred option, but I do not know how much breakage that would cause. Any insight?

That would break in cases where the initial CSS is fetched with a crossorigin attribute, but the import is not. That seems like something the HTTPArchive may be able help with. (One query to gather CORS requested CSS that has @import, and another to see how much of those URLs are CORS enabled.)

/cc @rickviscomi

@youennf

This comment has been minimized.

Copy link

youennf commented Nov 29, 2018

Resource timing leakage is now fixed in WebKit ToT, https://bugs.webkit.org/show_bug.cgi?id=192132.

That would break in cases where the initial CSS is fetched with a crossorigin attribute, but the import is not. That seems like something the HTTPArchive may be able help with. (One query to gather CORS requested CSS that has @import, and another to see how much of those URLs are CORS enabled.

Having a list of cross-origin imported CSS would be definitely useful.
I do not think HTTPArchive will tell us whether these URLs are CORS enabled though (no Origin header in the corresponding request probably).
Once we have the list, we should be able to curl them to check that.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Nov 30, 2018

I do not think HTTPArchive will tell us whether these URLs are CORS enabled though (no Origin header in the corresponding request probably).

The HTTPArchive also includes the request headers, so you could detect CORS enabled requests from the existence of the Origin header.

Once we have the list, we should be able to curl them to check that.

To be clear, I don't think I'll have the bandwidth to tackle this is the near future. I just pointed out that the data set to perform this analysis exists and is publicly available.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Dec 18, 2018

Trying to think how to unblock L2 from this discussion, on which there's no clear agreement and which requires further research and spec work.

I suggest (again) to ship L2 without this restriction, which represents a majority of implementations today, and reintroduce that restriction to L3 if and when implementations actually introduce it and once there's consensus on that front.

@toddreifsteck @igrigorik - thoughts?

@toddreifsteck

This comment has been minimized.

Copy link
Member

toddreifsteck commented Jan 3, 2019

It seems that everyone agrees this is a privacy hole, but the disagreement is in the priority of the privacy hole given that Service Worker has the same hole and appears less apt to fix.

Given that, why don't we update the spec to state that this is an At-Risk piece of the spec and ship L2 with that note?

That leaves the intent of the spec in place, but allows us to remove the normative requirement.

As discussed in a sync of the chairs, if 2 implementations are able to quickly pass the tests, it is a quick patch to make this a requirement.

@igrigorik @yoavweiss How is that?

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Jan 3, 2019

Landed the related tests in web-platform-tests/wpt#9307
Leaving this open until we have 2 passing implementations

@youennf

This comment has been minimized.

Copy link

youennf commented Jan 3, 2019

As discussed in a sync of the chairs, if 2 implementations are able to quickly pass the tests, it is a quick patch to make this a requirement.

STP72 passes the test, Firefox is just missing the font one. The 2 implementations criteria is very close.
IIUC, L2 is a WD and might migrate to CR soon.
Since there is consensus in the behavior we want, I would specify the change before CR as way to encourage implementers to align.

@yoavweiss

This comment has been minimized.

Copy link
Contributor

yoavweiss commented Jan 3, 2019

To be clear, the current spec includes the resource exclusion (step 1 in the processing model). So the question here was whether we should keep that exclusion or not. Assuming Firefox will align and pass the test, I agree it makes sense to keep the exclusion for L2.

If Firefox doesn't align in time, we can indeed mark this "at risk", as @toddreifsteck suggested.

@youennf

This comment has been minimized.

Copy link

youennf commented Jan 3, 2019

Oh, makes sense then!

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Jan 4, 2019

FWIW, given that we agree on the behavior and tests are in place, I don't think we need to keep this open. Rather, let's update #70 with the outstanding requirements we need to hit to move L2 to CR?

@igrigorik

This comment has been minimized.

Copy link
Member

igrigorik commented Jan 4, 2019

@toddreifsteck sorry, missed your earlier comment. I'm OK with adding an at-risk note, which we can remove as soon as we have to green implementations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment