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

RESOLVED FIXED in mozilla18

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

({dev-doc-needed})

Trunk
mozilla18
x86
Mac OS X
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

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.
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?
I'm going to attach what I have so far, which does all the infrastructure except actually allowing scripted access in any new cases.
Created attachment 602557 [details] [diff] [review]
part 1.  Clean up nsContentSink methods that used to take an element and now don't.
Attachment #602557 - Flags: review?(jonas)
Created attachment 602558 [details] [diff] [review]
part 2.  Communicate the CORS state of style link loads to the CSS loader.
Attachment #602558 - Flags: review?(jonas)
Created attachment 602559 [details] [diff] [review]
part 3.  Communicate the CORS state of preloads to the CSS loader.
Attachment #602559 - Flags: review?(jonas)
Attachment #602559 - Flags: review?(hsivonen)
Created attachment 602560 [details] [diff] [review]
part 4.  Propagate the CORS state throughout the CSS loading process.
Attachment #602560 - Flags: review?(jonas)
Attachment #602560 - Flags: review?(dbaron)
Created attachment 602561 [details] [diff] [review]
part 5.  Enforce CORS on stylesheet loads as needed.
Attachment #602561 - Flags: review?(jonas)
Assignee: nobody → bzbarsky
Whiteboard: [need plan]
Attachment #602559 - Flags: review?(hsivonen) → review+
Attachment #602557 - Flags: review?(jonas) → review+
Attachment #602558 - Flags: review?(jonas) → review+
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)
Attachment #602559 - Flags: review?(jonas) → review+
Attachment #602561 - Flags: review?(jonas) → review+
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...
Attachment #602560 - Flags: review?(jonas) → review+
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
Attachment #602560 - Flags: review?(dbaron) → review+
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.
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.
That's a good point.  Wladimir, thoughts?  Including on whom else to cc?
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.
> 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.
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?
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.
(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?
Possibly those too; I can't recall whether those use the page principal or the sheet principal...
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.
Created attachment 655872 [details] [diff] [review]
Actually use the CORS state, as discussed.

This implements option #5.
Attachment #655872 - Flags: review?(jonas)
Attachment #655872 - Flags: review?(dbaron)
Whiteboard: [need plan] → [need review]
Attachment #655872 - Flags: review?(jonas) → review+
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
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla18
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 786564
Keywords: dev-doc-needed
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.
Attachment #655872 - Flags: review?(dbaron)

Comment 25

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ccbc9935c2
https://hg.mozilla.org/mozilla-central/rev/39ccbc9935c2
You need to log in before you can comment on or make changes to this bug.