1262963 - Allow CSSOM access to cross-origin stylesheets (for testing only)

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P3)

Description

[Jet Villegas (inactive)]

Attachment: I should lop off a finger for even suggesting this patch...

I don't think we'd ever ship this, for obvious reasons. I'm filing this bug so we can review the correct way to build Firefox to allow a crawler to collect the style rules used by a site. Such a build could be used to collect CSS usage like:
https://developer.microsoft.com/en-us/microsoft-edge/platform/usage/

Comment 1

[Cameron McCormack (:heycam)]

To quote from https://developer.microsoft.com/en-us/microsoft-edge/platform/faq/where-is-firefox/:

  We would love to crawl with Firefox, and have reached out to them to see if they would be
  willing to provide a way to spin up their browser with access to external CSS stylesheets. Since
  this currently isn’t possible we don’t want to include their numbers as we don’t want those to
  affect the data we’re providing to the community.


Here is the script that is being injected (using WebDriver) when Firefox is being driven for this crawl: https://github.com/MicrosoftEdge/css-usage/blob/6432187/cssUsage.src.js#L307

How about adding a new pref to control whether read only cross-origin CSSOM access is allowed (a different pref from the one in attachment 8739166 [details] [diff] [review], since that one implies some other security loosening that we don't need here), and only taking notice of the pref if WebDriver is enabled?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Define "read only"?  Once you have your hands on a CSSStyleDeclaration from this cross-origin sheet, can you set properties on it?  If not, we'd need to add checks for this stuff on a bunch of codepaths, right?

Anyway, I'm OK with a non-default mode, using a different pref name, that would just let you do whatever the heck you want with cross-origin sheets.  And yes, only having that pref apply when webdriver is enabled is a good idea.

Comment 3

[Cameron McCormack (:heycam)]

Good point about read only -- I was thinking we would just add the pref check to GetCssRules and not InsertRule and DeleteRule, but of course you are right once we have access to objects further in the sheet that's harder to restrict.

Comment 4

[Cameron McCormack (:heycam)]

David, is there a good way of detecting, from outside of the Marionette add-on, that we are running with Web Driver enabled?

Comment 5

[Andreas Tolfsen ❲:ato❳]

There is bug 1169290 to add an IDL attribute to the `navigator` object when Marionette is active.  I’m not quite sure how to go about implementing that from JS/XPCOM.

In the meantime I guess it is possible to execute a script that sets an attribute on the `window` object:

    session.execute_script("window.wrappedJSObject.webdriver = true")

Comment 6

[Jet Villegas (inactive)]

Attachment: Updated patch with a new pref

Changing the pref name and posting a new patch. Try results: 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b77b2b2f1a7

Comment 7

[David Burns :automatedtester]

We need to implement what is in bug 1169290

Comment 8

[Jet Villegas (inactive)]

Attachment: Add an env variable for shell scripting

Comment 9

[Andrew Overholt [:overholt]]

Are we going to land this or something like it, Jet? Or is having patched builds sufficient?

Comment 10

[Jet Villegas (inactive)]

(In reply to Andrew Overholt [:overholt] from comment #9)
> Are we going to land this or something like it, Jet? Or is having patched
> builds sufficient?

I'm inclined to ship just the patch in comment 8 if it passes a sec-review. heycam: WDYT?

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1262963 - Add env var for bypassing origin check of cssom.

Review commit: https://reviewboard.mozilla.org/r/96196/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/96196/

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Comparing to the patch from comment 6, there are several effective difference:
* the pref is changed to "security.turn_off_cssom_origin_check" which I think makes more sense;
* in addition to allowing reading cssRules, it also enables insertRule and deleteRule because the security check code has been unified a while ago, so it is easier to do this;
* the origin of the stylesheet is changed to the page origin when accessed

Comment 13

[Cameron McCormack (:heycam)]

I had hoped that we could restrict this a bit more.  It makes me wary to just have a pref by itself, since addons (and end users!) can set prefs, and I think we have a history of bad-for-security prefs being set by users and addons.  Checking this pref and verifying that we are running under Marionette would be a good combination, I think, but when I tried earlier on I couldn't find a way to do that.

Andreas and David mentioned bug 1169290 above, but it's not that we want to expose "is running under Marionette" to script, rather just internally.  (Although maybe the work to do bug 1169290 would help with that.)


(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> * the origin of the stylesheet is changed to the page origin when accessed

How it this achieved in the patch, and what advantage does it have to do this?

Comment 14

[Mike Taylor [:miketaylr]]

> I had hoped that we could restrict this a bit more. 

Apparently you can set environment variables via add-ons as well (TIL): https://developer.mozilla.org/en-US/Add-ons/SDK/Low-Level_APIs/system_environment

Comment 15

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Cameron McCormack (:heycam) from comment #13)
> I had hoped that we could restrict this a bit more.  It makes me wary to
> just have a pref by itself, since addons (and end users!) can set prefs, and
> I think we have a history of bad-for-security prefs being set by users and
> addons.  Checking this pref and verifying that we are running under
> Marionette would be a good combination, I think, but when I tried earlier on
> I couldn't find a way to do that.
> 
> Andreas and David mentioned bug 1169290 above, but it's not that we want to
> expose "is running under Marionette" to script, rather just internally. 
> (Although maybe the work to do bug 1169290 would help with that.)

It doesn't seem to me there is anything that Marionette driver can do but an addon cannot. Probably a command line argument would be better if you really worry about this.

For addons, we can probably ask addon team to carefully review (or probably just reject) any addon which touches "security." prefs.

I don't actually think it is a big deal that end users can turn it on. That would be a small number of users anyway, which means it wouldn't be something useful for attackers. (If it turns out to be widespread, we can respond then. But why may users want to do that? One reason might be bug 1314311, though.)

It may allow websites to access what they couldn't access before, but it is also unlikely that sensitive information could be leaked via CSSOM, because that requires the sensitive information to be parsable as CSS first, which majority of web pages aren't.

Users can easily shoot in their foot on security especially by setting environment variables (to enable outputing ssl private keys, for example). So this isn't the worst thing they can do to hurt their own security anyway.

I guess, we could add something to the UI to alert user that some insecure setting has been activated on their instance so that they can be aware of that, in case they forget to close after using them.

I'll also ask security team to do security review on this patch as well, to see if they are concerned about this, and if they are fine with my reasoning above.

> (In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> > * the origin of the stylesheet is changed to the page origin when accessed
> 
> How it this achieved in the patch, and what advantage does it have to do
> this?

StyleSheet::SubjectSubsumesInnerPrincipal() is already doing this for cross-origin CSSOM access. I just make this pref part of its condition to do that.

Comment 16

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8815168 [details]
Bug 1262963 - Add env var for bypassing origin check of cssom.

Set sec-approval? for the new (hidden) pref which disables origin check for accessing CSSOM.

See comment 15 for reason why I don't think this is bad for security.

Comment 17

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

This is a public bug. Sec-approval is useless here. :-)
The point of sec-approval is to gate checkins for exposure on sec-critical and sec-high rated security bugs.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

So how should I ask for a security review of the patch?

Comment 19

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

Set the sec-review flag or needinfo? the person who would do the review?

Whose decision is this issue?

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

It seems I cannot set the sec-review flag. I think it is from Jet.

Comment 21

[Cameron McCormack (:heycam)]

Jet, do you think we still need to do this?

Comment 22

[Jet Villegas (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #21)
> Jet, do you think we still need to do this?

I'll find out at CSSWG F2F next week. Keeping the NI:me open.

Comment 23

[Jet Villegas (inactive)]

(In reply to Cameron McCormack (:heycam) from comment #21)
> Jet, do you think we still need to do this?

It appears we do. We can get by with just the environment variable and #ifndef RELEASE to further secure this one.

Comment 24

[Greg Whitworth]

How is this coming along?

Comment 25

[Xidorn Quan [:xidorn] UTC+11]

ni? myself for updating the patch to use environment variable instead of pref.

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8815168 [details]
Bug 1262963 - Add env var for bypassing origin check of cssom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96196/diff/1-2/

Comment 27

[Xidorn Quan [:xidorn] UTC+11]

Patch updated.

So now the switch is behind a one-off initialization from environment variable, so it is less likely to be changed. With e10s, though, new child process may still be affected if the environment variable changes.

Also it is behind RELEASE_OR_BETA, which means this environment variable would only be effective on Nightly. Hopefully this is enough for people who want to test.

heycam, could you review this patch now?

Comment 28

[Xidorn Quan [:xidorn] UTC+11]

Greg, currently I'm going to only allow that environment variable on Nightly. Does that work for you? Or you want to use beta or release version for your purpose?

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8815168 [details]
Bug 1262963 - Add env var for bypassing origin check of cssom.

https://reviewboard.mozilla.org/r/96196/#review143352

::: dom/base/nsContentUtils.h:2229
(Diff revision 2)
>    /**
> +   * Returns true if CSSOM origin check should be skipped for test.
> +   */

Please elaborate a little more in this comment about what kind of testing this is designed for, and perhaps mention the environment variable here too.

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

Greg, also, could you try using this build with your script and see if it works? https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-8930a72286deb2ca4191e68df7cb29635f3bbf68/try-win64-debug/firefox-55.0a1.en-US.win64.zip

The environment variable is MOZ_BYPASS_CSSOM_ORIGIN_CHECK.

Comment 31

[Xidorn Quan [:xidorn] UTC+11]

Oops, that's wrong one. Should be https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-8930a72286deb2ca4191e68df7cb29635f3bbf68/try-win64/firefox-55.0a1.en-US.win64.zip

Comment 32

[Greg Whitworth]

Yes, nightly only is fine - I'll update the VMs with that build and configure the var and let you all know. One quick question, will this ALWAYS be in nightly? As we'd like to update it every time we run without needing custom builds. I assume yes, but want to be sure.

Comment 33

[Cameron McCormack (:heycam)]

Yes it will.  Once you've confirmed that build works, we'll land the patch.

Comment 34

[Greg Whitworth]

Ok, we have verified this in the crawler. Please ship the patch - thank you all so much for the hard work and patience.

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8815168 [details]
Bug 1262963 - Add env var for bypassing origin check of cssom.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/96196/diff/2-3/

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/de9ea32f4238
Add env var for bypassing origin check of cssom. r=heycam

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/de9ea32f4238

Comment 38

[Xidorn Quan [:xidorn] UTC+11]

Nightly today or tomorrow should start including this :)