1201160 - Service workers violate SOP for "no-cors" CSS

Status: REOPENED

(Core :: DOM: Service Workers, defect)

Description

[(no longer active)]

+++ This bug was initially created as a clone of Bug #1180145 +++

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

[Andrew Overholt [:overholt]]

Is the addition of the SW vector enough to block SW on this?

Comment 2

[(no longer active)]

Yes.

Comment 3

[Anne (:annevk)]

To be clear, what needs to be done here is that any resources fetched from a stylesheet, that was itself fetched using "no-cors" and is cross-origin (not sure about cross-origin with redirect to same-origin), need to skip the service worker.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

I assume this means we need opaque interception tainting for CSS stylesheets as well then.  If the stylesheet has a same-origin URL, but was intercepted with a no-cors opaque Response, then we also need to block further interception for that sheet's subresources.  Right?

Comment 5

[Anne (:annevk)]

Yes. 

For the redirect case, currently per Fetch no-cors cross-origin redirect to same-origin will taint and return an opaque response. That seems the most safe, that way a cross-origin URL cannot be used to manipulate a same-origin resource to give up information. (At least not more than it already could.)

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I'll take a look at this one.  The solution here will probably depend on what lands in bug 1173811.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Looks like I either need to call this or do something like it when loading a resource:

  https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#1782

And if it fails to subsume, then set ForceNoIntercept().

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

There is an existing wpt test from blink that tests the case where the stylesheet is loaded from an opaque interception and has a subresource also controlled by the service worker:

  https://dxr.mozilla.org/mozilla-central/source/testing/web-platform/mozilla/tests/service-workers/service-worker/fetch-request-css-base-url.https.html

Its mainly checking to make sure the resulting URLs are correct, but we'll need to change it to enforce that the png file is not seen at all by the SW.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

This is what I plan to test here:

// For frame that loads <link> stylesheet with a png subresource:

// frame a.com, stylesheet a.com, SW sees png
// frame a.com, stylesheet b.com cors, SW sees png
// frame a.com, stylesheet b.com no-cors, SW does not see png
// frame a.com, stylesheet a.com redirected to b.com cors, SW does not see png
// frame a.com, stylesheet a.com redirected to b.com no-cors, SW does not see png
// frame a.com, stylesheet a.com intercepted to synthetic, SW sees png
// frame a.com, stylesheet a.com intercepted to a.com, SW sees png
// frame a.com, stylesheet a.com intercepted to b.com cors, SW sees png
// frame a.com, stylesheet a.com intercepted to b.com no-cors, SW does not see png
// frame a.com, stylesheet a.com intercepted to a.com redirected to b.com cors, SW sees png
// frame a.com, stylesheet a.com intercepted to a.com redirected to b.com no-cors, SW does not see png

Comment 10

[Anne (:annevk)]

How are case 4 and 5 not intercepted (or 1, 2, and 3 for that matter)? I also miss stylesheet b.com redirected/intercepted to a.com.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #10)
> How are case 4 and 5 not intercepted (or 1, 2, and 3 for that matter)?

Sorry, maybe its confusion over my use of "intercepted".  Here I mean that the SW will not call FetchEvent.respondWith() for the stylesheet and just allow normal browser processing.

> I also miss stylesheet b.com redirected/intercepted to a.com.

What should the behavior be in those cases?  And does it differ if b.com is cors vs no-cors?

Comment 12

[Anne (:annevk)]

> What should the behavior be in those cases?

You get tainting for "no-cors" and no tainting for "cors" (the redirect itself and the final response need to have CORS headers, even though the final response is same-origin).

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Anne (:annevk) from comment #12)
> > What should the behavior be in those cases?
> 
> You get tainting for "no-cors" and no tainting for "cors" (the redirect
> itself and the final response need to have CORS headers, even though the
> final response is same-origin).

I commented on this over in bug 1173811 comment 46.  I don't think our current cross-origin tainting plan will handle this case.  So if I write these tests we will have to mark them expected fail.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update service worker CSS wpt tests. r=ehsan

The beginning of the wpt tests.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update service worker CSS wpt tests. r=ehsan

An updated version of the tests with some redirect cases.  Note, these depend on bug 1206124, but currently hit an assert I added in the patch over there.

Also, these tests need to add a unique token to the background-image to avoid image cache confusion.

I will finish this when I get back from PTO.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update service worker CSS wpt tests. r=ehsan

I still need to remove the setTimeout() and cleanup some debugging, but these tests hit all the cases we need.

Currently fails due to this bug and also because redirected interceptions are changing the Response.url.  I think Ehsan has patches in queue or recently landed to fix the second issue.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

This is a bit harder than I thought in comment 7.  I need to find all the places stylesheets load sub-resources to call the ForceNoIntercept().  That is different than the place it does NS_NewChannel() in style/Loader.cpp.

This means doing something different for images vs @import, etc.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

I think it will be a lot easier to implement the intercept bypass for all the different cases if we do bug 1210941 first.

Comment 19

[Ben Kelly [:bkelly, not reviewing]]

As I dig into this, I think its much harder than I originally thought.

Consider:

1) a.com/index.html loads stylesheet at b.com/foo.css as no-cors
2) b.com/foo.css @imports stylesheet at a.com/bar.css
3) a.com/bar.css loads background-image a.com/snafu.jpg

The service worker should get a FetchEvent for b.com/foo.css, but not for any of the other sub-resources.  Since b.com/foo.css loads a same-origin a.com/bar.css, I need some what to propagate the no-cors tainting through the @import.

Maybe @import is same-origin only, though.  That would make this a bit easier.

Comment 20

[(no longer active)]

(In reply to Ben Kelly [:bkelly] from comment #19)
> Maybe @import is same-origin only, though.  That would make this a bit
> easier.

I don't think it is.  See Loader::CheckLoadAllowed().

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

Yea, many people have told me its not.

So I need to create a tainting flag on the stylesheet and propagate it through to child sheets somewhere in Loader::LoadChildSheet() here:

   https://dxr.mozilla.org/mozilla-central/source/layout/style/Loader.cpp#2256

Of course, we could end up with some interesting behavior in this condition:

1) a.com/index.html loads stylesheet at b.com/foo.css as no-cors
2) b.com/foo.css @imports stylesheet at a.com/bar.css
3) a.com/bar.css loads background-image a.com/snafu.jpg
4) a.com/index.html loads stylesheet at a.com/thepain.css
5) a.com/thepain.css @imports stylesheet at a.com/bar.css
6) a.com/bar.css loads background-image a.com/snafu.jpg

Here we have a.com/bar.css being @imported twice; once from cross-origin and once from same-origin.  At steps (2) and (3) we would block the service worker from seeing the fetch events.

What do we do at steps (5) and (6)?  The naive implementation would not do any network loads at all because the stylesheet from step (2) would be reused.  But this then allows a.com to have insight into what b.com/foo.css loads.  Also the service worker could result in completely different content being returned.

At this point I think I am just going to implement the naive solution for now.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

I guess I could just avoid adding tainted sheets to the reusable sheet map here:

  https://dxr.mozilla.org/mozilla-central/source/layout/style/CSSStyleSheet.cpp#2308

Comment 23

[Ben Kelly [:bkelly, not reviewing]]

In IRC Boris pointed out the real stylesheet reuse is done in Loader.cpp using this Hash key:

  http://mxr.mozilla.org/mozilla-central/source/layout/style/Loader.h?force=1#41

Which includes CORS mode, so I think it will prevent the kind of sharing discussed in comment 21.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

I discussed this more with Boris.  Rough plan:

1) I will add a no-cors taint flag to the sheet.
2) I will propagate the taint flag during CreateChildSheet().
3) I will add the taint flag to the hash key in Loader.cpp to avoid reuse.
4) I will add the taint flag to URLValue so it can be passed to the ImageLoader from nsCSSValue.

I will also have to extend the tests further to cover the @import case.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not convinced that we need to do this.

We're already exposing a lot of information that's contained in the stylesheet through features like getComputedStyle. An attacker can already probe through through a list of ids and classnames and use getComputedStyle to get at the background-url expressed in a rule in the cross-origin stylesheet.

I think blocking these requests from going through the SW will add a lot of complexity for web developers which is why I think we should really think through if we need to do this.

Does anyone remember the specific issues that we had that caused us to block access to the CSSOM for cross-origin stylesheets? My recollection was that it was related to pointing <link rel=stylesheet>s to non-CSS resources and taking advantage of the quite forgiving CSS parser.

However I thought that we required cross-origin stylesheets to be served with a text/css mimetype. Not sure if that means that my recollection above was wrong, or if that mimetype restriction was added later?

Comment 26

[Boris Zbarsky [:bzbarsky]]

The mimetype restriction was added much later.

Comment 27

[Anne (:annevk)]

You cannot get at cross-origin @import rules (and potentially others?) and finding stuff through getComputedStyle() is much more involved.

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

So, looking at the spec issue:

  https://github.com/slightlyoff/ServiceWorker/issues/719

It seems Jake made some of the same points Jonas makes in comment 25.  The discussion with google still concluded we should hide these requests from service worker.

From the spec issue it seems service workers would also leak font source URLs which previously were not observable.

Blocking no-cors CSS being hosted on a CDN may cause some pain for developers, but service workers already require other configuration changes for safety; e.g. https.  These developers simply have to switch their CDN hosting to use CORS headers and then it should work for them.

I think we should move forward with this.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Another reason to do this is Josh found that with our fake-redirect approach to opaque tainting that the stylesheet subresources would get intercepted with the opaque final URI.  In essence, this is a case where we expose the final URI of a no-cors redirect, and thus breaks the assumptions with that plan.

So I need to implement the SW block here or come up with another fix to account for the opaque tainting.

At our service worker meeting today, though, we decided just to go ahead with this.  Mainly for the reasons in comment 28.  Sorry Jonas.

Comment 30

[Liz Henry (:lizzard) (relman/hg->git project)]

Which versions does this affect?

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Only affects versions with service workers enabled.  So right now nightly and aurora.

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, this leaks more data. But I'm not convinced that it's leaking meaningfully more data. I don't think any developer would be advised to put user-private data in any CSS file. If they do, the risk is very high that there is some way to get at that data.

I don't think "easier" vs. "harder" is meaningful here. Keep in mind that often libraries can turn "harder" into "easier".

Adding restrictions here comes at a very real cost, which is that it makes it harder to develop for the web platform. Are these costs really outweighed by the security benefits?

As far as I can tell, aren't any security benefits that makes the lives of developers easier. I.e. developers can't be less careful about what content they put on their webserver.

But there are definitely development costs here. I.e. we're making developer's lives harder.

There's also a non-negligible security cost here in the sense of false sense of security. The fact that we're making it very visible that we're protecting the embedded URLs here, makes it harder to see that getComputedStyle leaks far more information.


I don't think things are as simple as "it exposes more data so we must not do it". For example, the complexity that we have with the CORS Content-Type header has probably been a net-bad for the web. That one was my fault and I definitely regret it.

Comment 33

[(no longer active)]

Jonas, this exposes stuff that getComputedStyle() doesn't.  Therefore, the fact that getComputedStyle() exists cannot be used as a reason to not fix this...

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ehsan, did you read the last paragraph of comment 32?

Comment 35

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Or rather, all of comment 32 is about how the question isn't as simple as that.

Comment 36

[(no longer active)]

I did.  I understand the trade-off.  What I was trying to say is that this is not just about exposing more or less data, it's also about exposing new data.

</preach-to-choir>  :)

Comment 37

[(no longer active)]

(Also the discussion as to whether or not we should fix this should really happen on the spec issue...)

Comment 38

[(no longer active)]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #37)
> (Also the discussion as to whether or not we should fix this should really
> happen on the spec issue...)

https://github.com/slightlyoff/ServiceWorker/issues/719

Comment 39

[(no longer active)]

We decided to punt on this for now.

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

The service worker f2f decided this could be a v2 issue.

Comment 41

[Ben Kelly [:bkelly, not reviewing]]

I asked about this again in #whatwg IRC logs:

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

I think the current state is a bit confused and should be clarified before we take on the complexity of implementing this.

Comment 42

[Andrew Overholt [:overholt]]

If this is being discussed in the open elsewhere and Chrome is shipping with this and we're consciously holding off on doing anything here until the spec is sorted, should we keep this as sec-high?

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Overholt [:overholt] from comment #42)
> If this is being discussed in the open elsewhere and Chrome is shipping with
> this and we're consciously holding off on doing anything here until the spec
> is sorted, should we keep this as sec-high?

Thats a good question, but I don't feel comfortable lowering it myself.  I'd prefer if someone from the security team made that determination.

Comment 44

[Andrew Overholt [:overholt]]

(In reply to Ben Kelly [:bkelly] from comment #43)
> (In reply to Andrew Overholt [:overholt] from comment #42)
> > If this is being discussed in the open elsewhere and Chrome is shipping with
> > this and we're consciously holding off on doing anything here until the spec
> > is sorted, should we keep this as sec-high?
> 
> Thats a good question, but I don't feel comfortable lowering it myself.  I'd
> prefer if someone from the security team made that determination.

Dan, can that be you?

Comment 45

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

Also ni'ing Al in case he might be able to answer the question posed in comment 44. I came upon this bug while reviewing open 44+ tracked bugs.

Comment 46

[Al Billings [:abillings - ex-MoCo]]

I really don't know. I'll have to defer to Dan here.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

Moving my wip patches here to get them out of my local queue.  I'm hoping to get some spec feedback from google later this month.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Update service worker CSS wpt tests. r=ehsan

Comment 49

[Ben Kelly [:bkelly, not reviewing]]

Attachment: wip

Comment 50

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

We are in RC mode, it's too late and this is now a wontfix for Fx44.

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Ben, are you still working on this bug? Thanks

Comment 52

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #51)
> Ben, are you still working on this bug? Thanks

I asked at the service worker face-to-face and did not get much back.  Still trying to understand what the other browsers are doing, if anything.

So far internally we have a strong vote for (annevk) and a strong vote against (sicking).  If we can come to a consensus internally we could move forward.  Right now the "don't implement this" side is winning simply because its the default situation.

Comment 53

[Andrew Overholt [:overholt]]

Given that we don't know what we're going to do here, should we continue tracking this?

Comment 54

[Sylvestre Ledru [:Sylvestre]]

Yes, let's wontfix it.
Please resubmit for tracking when we have a proper fix.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

Dropping assignment until we reach some spec clarity.

Comment 56

[Andrew Overholt [:overholt]]

Do we normally keep things with a lack of spec clarity on the sec-high list?

Comment 57

[Daniel Veditz [:dveditz]]

Unhiding because the spec issue is public.

(In reply to Andrew Overholt [:overholt] from comment #56)
> Do we normally keep things with a lack of spec clarity on the sec-high list?

Those are unrelated--consider several recent TLS problems that were baked into the protocol or common implementation mistakes due to unclear specs and yet were still usable to exploit folks. Is this a Same-origin policy violation? Yes, so sec-high is a good place to start whether it's an implementation or spec bug.

If we want to argue that the attack is unlikely to yield valuable data we could lower it to moderate. Do sites serve custom CSS to users? If not then the attacking server could just download the CSS directly and read it and there's no disclosure. Is such a custom CSS rare enough to call this moderate? Do common resources loaded by the CSS redirect to different places in a way that a service worker can detect? If not then there's no real disclosure. I don't know how common or rare any of this is and without telemetry I'd prefer to err on the side of rating this too high. If we have some, though...

Comment 58

[Anne (:annevk)]

Also note that CSS in general is extremely unclear about its integration with Fetch. That did not stop us from implementing service worker support that included passing CSS through service workers (and thereby assuming something about what a CSS specification might say about the manner). 

I think we should similarly assume that nobody wants to introduce new same-origin policy violations yet for some reason Jonas keeps pushing for these which is why we end up having them. Here, but now also with our experimental implementation of <script type=module>. It would really help if the security team could make it clearer what our stance is with respect to the same-origin policy and its various extensions.

Comment 59

[Andrew Overholt [:overholt]]

So are we going to do anything here?

Comment 60

[Anne (:annevk)]

Valentin, you are fixing bug 1180145 (sec-high), but unless we also fix this bug, we are not actually closing the hole.

Daniel and bz considered that bug very much worthy of fixing. I don't understand why it's any different here. We either need to fix both, or enshrine yet another SOP exception. (Note that if we enshrine this one, the JavaScript module folks probably want an CORS-exception for their stuff too... Or at least it'll be even harder to convince them. Sigh.)

Comment 61

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I still see no point in fixing this unless other browsers do. And I also don't think that the complexity introduced by this bug, and the security bugs that are sure to result from that complexity, is worth the benefit claimed in this bug.

Comment 62

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

I have a WIP patch posted in bug 1180145, but it's not enough to cover all of the cases.
I'd rather someone else picks it up, if we decide we do want to fix it after all.

Comment 63

[Ben Kelly [:bkelly, not reviewing]]

I'm going to leave this open for now, but I don't think we will implement anything here unless other browsers implement it first.  In particular, it would be hard for us to take the compat hit unless chrome implemented it first.

Comment 64

[Andrew McCreight [:mccr8]]

I'm going to mark this WONTFIX if the decision here is to not fix it. Please reopen the bug if the decision changes.

Comment 65

[Anne (:annevk)]

Reopening as Safari has addressed this reportedly and standards-wise it's still an issue: https://github.com/w3c/ServiceWorker/issues/719#issuecomment-662900026.

Comment 66

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Before we do anything here I think we should be super mindful about negative effects on ability to write offline web applications.

In particular, a website can already read large parts of the contents of unprotected CSS files. What this bug suggests is that we specifically protect values that contain URLs, while leaving the other parts of the CSS file just as exposed as before.

As a developer, it seems really risky to rely on the default SOP to protect the contents of CSS files given how much of the file contents that is readable across sites.

A better solution to me seems to be to enable solutions that enable protecting the entire contents of CSS files, images, scripts and other resources that can be embedded (and thus partially read) cross origin.

It's definitely true that leaving this bug unfixed could cause a security issue. I.e. where there's a CSS file that contains sensitive URLs but not other sensitive CSS values. But the same argument can be made about many changes to CSS syntax, such as with the introduction of variables. They too expanded the set of files which could theoretically now expose sensitive information to an attacker. Do we know of any such instances in the few years since Service Workers or CSS variables were shipped?

Additionally, some of the URLs that we're trying to protect here will still be exposed in the form of computed values on elements in the DOM.

In short, I think implementing the proposal here has significant cost for web developers, both in terms of reduced features, and in terms of a more complex security model. And the benefit seems mainly theoretical to me?

Comment 67

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Separately, would Chrome and Safari be willing to fix [1]? If not would there actually be any security benefit for web developers?

[1] https://jsbin.com/pigihubuxa/edit?html,output

Comment 68

[Jens Stutte [:jstutte]]

Hi Anne, can you investigate further what should be our path forward here? Thank you!

Comment 69

[Anne (:annevk)]

I agree with Jonas that the situation isn't exactly great here and anyone putting secrets in CSS resources is well-advised to use Cross-Origin-Resource-Policy or equivalent.

Fixing this would help with:

1. Inadvertently leaking secret URLs in @import and similar rules not directly exposed through computed styles.
2. Leaking information about the resources tied to those secret URLs through timing attacks on caches or fetching, or equivalent attacks. (At least, it seems this would be a lot harder if you do not get granular information through service workers.)
3. Leaking secret URLs in rules that are guarded by selectors that are (close to) impossible to match.
4. Moving the web platform to a state that's acceptable to everyone (as per comment 65 Safari is shipping this).
5. Making it clear we take exceptions to the same-origin policy seriously.

So overall I continue to recommend we fix this in due course. Hope that helps.

Comment 70

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems to me that without Chrome and Safari fixing the issue in comment 67 point 1 and 3 does not seem to apply. I.e. authors would still be leaking secrets in those URLs.

It seems like currently we're aligned with Chrome, but fixing this issue would align us with Safari. So it doesn't seem like fixing this issue would get us closer to consensus? Or has Google signaled that they are planning on fixing this issue?

As for point 5, has there been concern raised publicly that Mozilla is not taking SOP seriously? If anything, Mozilla has been pushing for CORS when other vendors have not, for example for video, fonts and sendBeacon().

This leaves point 2, which I honestly don't understand so I'll assume it is valid.

Comment 71

[Anne (:annevk)]

I don't see how comment 67 exposes URLs not exposed through computed styles. It seems this will be discussed as part of https://github.com/w3c/ServiceWorker/issues/1536 and I'll try to attend that (and other things mentioned there).