732209 - Allow access to the rules of cross-origin sheets that have undergone a CORS check

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Boris Zbarsky [:bzbarsky]]

There's an open question as to what to do about @import.  Should it inherit the CORS mode of the importing sheet, or should we have a way to specify the use of CORS on imported sheets in CSS directly?

David, thoughts?

This depends on bug 696301 because that refactors things a bit to make them simpler and all.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So I have a good start on this.  I ran into one interesting issue.

Stylesheets can be modified.  For example, one can insert background image rules or whatnot into them.  When this is done, the load uses the principal of the stylesheet; in particular the URI from this principal is what's passed to content policy.  So for the items below keep in mind that content policy would like to know where the load is "really" coming from in some sense so that it can do adblocking or whatnot effectively.  At least I think it does.  Maybe it doesn't?

In any case, what do we want to do with sheets that are sent with CORS?  I see the following options:

1)  Give them the principal they were loaded from and allow modification by the page. 
    This allows the caller to insert stuff into the sheet that will then look like it
    was being loaded by the site the sheet came from, sort of.  Seems not so great.
2)  Give them the principal of the page that linked to them (ignoring @import for now)
    and allow modification by the page. This will let them sneak in loads that look like
    they come from that page, but maybe that's OK.
3)  Give them a null principal to start with and allow modification by the page.  May
    confuse content policy implementations?
4)  Give them the principal they were loaded from initially and allow modification by
    the page, but any time the page actually makes use of CORS to get access to the
    sheet, set the principal to a null principal.  Similar to 3 but not as aggressive.
5)  Give them the principal they were loaded from initially and allow modification by
    the page, but any time the page actually makes use of CORS to get access to the
    sheet, set the principal to the principal of the page.  Similar to 2 but safer for
    the page.  Maybe this is the way to go...
6)  Given them the principal they were loaded from initially and disallow modification
    by the page.  Seems suprising to authors and is actually really hard to implement in
    our code.

Thoughts?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I'm going to attach what I have so far, which does all the infrastructure except actually allowing scripted access in any new cases.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Clean up nsContentSink methods that used to take an element and now don't.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Communicate the CORS state of style link loads to the CSS loader.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Communicate the CORS state of preloads to the CSS loader.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Propagate the CORS state throughout the CSS loading process.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Enforce CORS on stylesheet loads as needed.

Comment 8

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

Comment on attachment 602559 [details] [diff] [review]
part 3.  Communicate the CORS state of preloads to the CSS loader.

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

Assuming this compiles on all platforms (ternary operator's and string references tend to be hard)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 602559 [details] [diff] [review]
part 3.  Communicate the CORS state of preloads to the CSS loader.

At least a try push was green...

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 602560 [details] [diff] [review]
part 4.  Propagate the CORS state throughout the CSS loading process.

>+  URIPrincipalAndCORSModeHashKey key(aLoadData->mURI,
>+                                    aLoadData->mLoaderPrincipal,
>+                                    aLoadData->mSheet->GetCORSMode());

one more space of indent

>-      URIAndPrincipalHashKey key(aLoadData->mURI,
>-                                 aLoadData->mLoaderPrincipal);
>+      URIPrincipalAndCORSModeHashKey key(aLoadData->mURI,
>+                                         aLoadData->mLoaderPrincipal,
>+                                        aLoadData->mSheet->GetCORSMode());

likewise, on the last line


>-      URIAndPrincipalHashKey key(aLoadData->mURI,
>-                                 aLoadData->mLoaderPrincipal);
>+      URIPrincipalAndCORSModeHashKey key(aLoadData->mURI,
>+                                         aLoadData->mLoaderPrincipal,
>+                                       aLoadData->mSheet->GetCORSMode());

again

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed those.  I'm going to push at least parts 1-4 and maybe part 5 depending on whether that seems reasonable, but I'd still love some feedback on comment 1.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Of comment 1, (4) sounds the best to me with (5) also seeming rather reasonable, but I feel like it might be a more interesting question for content policy implementors.

Comment 13

[Boris Zbarsky [:bzbarsky]]

That's a good point.  Wladimir, thoughts?  Including on whom else to cc?

Comment 14

[Wladimir Palant]

As far as I can tell, the stylesheet principal impacts content policies in two ways:

1) Requests associated with the system principal are not being passed to content policies. This means that solution (1) in comment 1 would allow web pages to bypass content policies: they load some stylesheet from chrome://global/, then insert their rules into that stylesheet. The loads initiated by this rule will have the principal of the stylesheet (system principal) and will be invisible to content policies. Solutions (4) and (5) don't have this issue but they will still have a side-effect: if a web page uses a chrome:// stylesheet, images or whatever loaded by this stylesheet won't trigger content policies (currently these loads trigger content policies even though it is a chrome:// stylesheet loading chrome:// images). Not sure whether anybody cares about seeing them, it's a rather uncommon scenario. On the other hand, solutions (2) and (3) will keep the current behavior as far as content policy calls go.

2) When the stylesheet loads something, aRequestOrigin and aRequestPrincipal parameters of the content policy call depend on the stylesheet principal. Adblock Plus doesn't care about these parameters and I think that most other content policy implementations behave similarly - we care more about what is being loaded than about who is doing it (e.g. CSP also ignores these parameters). However, NoScript apparently considers the aRequestOrigin parameter, maybe Giorgio can check how these changes would affect him.

Comment 15

[Boris Zbarsky [:bzbarsky]]

> then insert their rules into that stylesheet.

They can't insert rules into a sheet they load from chrome://global, since such sheets are never sending CORS headers.  So for purposes of system-principal sheets nothing at all will change in this bug.

Thanks for the info about what Adblock Plus is doing here, and good catch on Giorgio.

Comment 16

[Giorgio Maone [:ma1]]

For aContentType == TYPE_STYLESHEET, NoScript's content policy cares about XSL loads only (BTW, they would be useful to be given a type of their own: I'm guessing them apart using the aContext argument ATM). They're not affected by this change, are they?

Comment 17

[Boris Zbarsky [:bzbarsky]]

This wouldn't be for the TYPE_STYLESHEET type; that's not affected by this change at all.  The main affected type would be TYPE_IMAGE.

Comment 18

[Giorgio Maone [:ma1]]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #17)
> This wouldn't be for the TYPE_STYLESHEET type; that's not affected by this
> change at all.  The main affected type would be TYPE_IMAGE.

That's fine then, NoScript doesn't check that one at all. What about fonts and XBL?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Possibly those too; I can't recall whether those use the page principal or the sheet principal...

Comment 20

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

I was asked to comment on comment 1.

5 sounds awesome to me! :)

The only imperfect thing that I can see is that if we do lazy loading of things like background images, some of those loads might end up happening with the loading page's principal, even though the loading page didn't specifically ask those loads to happen.

But I feel that that's something we can live with.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Actually use the CORS state, as discussed.

This implements option #5.

Comment 22

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0284b039a7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3528d25c46d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ea8106821c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/40e63320e4b5
https://hg.mozilla.org/integration/mozilla-inbound/rev/4af1510fc1c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/62b86f439a09

Comment 23

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/f0284b039a7b
https://hg.mozilla.org/mozilla-central/rev/3528d25c46d1
https://hg.mozilla.org/mozilla-central/rev/7ea8106821c2
https://hg.mozilla.org/mozilla-central/rev/40e63320e4b5
https://hg.mozilla.org/mozilla-central/rev/4af1510fc1c2
https://hg.mozilla.org/mozilla-central/rev/62b86f439a09

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 655872 [details] [diff] [review]
Actually use the CORS state, as discussed.

As I told bz on IRC, I was fine with Jonas reviewing this.

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39ccbc9935c2

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39ccbc9935c2