763879 - (CSP) implement blocking of inline stylesheets

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Ashar Javed]

Attachment: csp test input

User Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/536.5 (KHTML, like Gecko) Chrome/19.0.1084.52 Safari/536.5

Steps to reproduce:

Hi,

I have a CSP test-bed available at http://www.mobilefuxx.de/csp/xsstest/test.php. I have a restrictive CSP policy i.e., 'self' for every type of resource. 
According to the CSP specification at https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#style-src :

"Whenever the user agent would apply style from a style attribute, instead the user agent must ignore the style."

But this is not the case right now Firefox 13 & Nightly. 


Actual results:

One can apply style using the style attribute. As an attachment you will find a text file that you can use an an input vector on the test-bed available at http://www.mobilefuxx.de/csp/xsstest/test.php. 

I have tested the same vectors with Chrome 19 & Canary & they do not allow inline-style to apply.



Expected results:

Would you please look into the issue? Thanks!

Comment 1

[Ashar Javed]

Attachment: POC Screen-Shot

I am attaching a png file & in which you can see Firefox applies the inline-style.

Comment 2

[Ashar Javed]

Attachment: chrome19_screen-shot

Chrome 19 does not allow to apply inline-style & you can see this in screen-shot.

Comment 3

[Matthias Versen [:Matti]]

I'm really unsure about the component...

Comment 4

[Boris Zbarsky [:bzbarsky]]

As far as I can tell, this was a spec addition that postdates our CSP implementation....

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Yeah, this is new in the W3C stuff.  "unsafe-inline" is not currently supported by the Firefox implementation of CSP.  We should add support for this (especially in style-src context) as part of adhering to the W3C specification.

Comment 7

[frank goossens]

I'm sure imelvin, geekboy & bsterne are fully aware, but just to make sure; as per the spec, "unsafe-inline" shouldn't only be available for inline stylesheets, but also for inline javascript and is indeed implemented as such in Chrome.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

To be clear, the spec expects this to address both the text inside <style> tags and the text inside style="" attributes on elements.

Comment 9

[Sid Stamm [:geekboy or :sstamm]]

dholbert brings up mapped attributes... these are attributes of an element that are mapped to the elements stylesheet, eg:
 <rect fill="green"> --> <rect style="fill:green">
Apparently these are stored in a per-element stylesheet (at least in SVG).  It's not clear in the spec if these should be addressed; I think they should be included.

Comment 10

[Daniel Holbert [:dholbert]]

SVG <animate> & <set> tags also are ways of setting per-element style. (accessible in C++ with the "GetSMILOverrideStyleRule()" method)

If we're disabling the "style" attribute and mapped attributes, then we should definitely disable animated style, too.

Comment 11

[Daniel Holbert [:dholbert]]

er, we should definitely disable *SMIL*-animated style, that is.  (We'd still want to honor CSS transitions & animations that are defined in an accepted external stylesheet.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: demo of SMIL animation of style

For reference, here's a demo with SMIL animating 3 CSS properties in slightly different ways.

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch and some tests

Here's a first whack at a patch.  It only grabs attribute-based and style-element-based inline styles, and not the SMIL stuff that dholbert was talking about.  Includes tests.

Comment 14

[Sid Stamm [:geekboy or :sstamm]]

Oh and there's still some things to do for attachment 656700 [details] [diff] [review]: 
* Add a new base violation type (inline styles) -- for reporting
* Implement mechanism to ask the CSP object whether inline styles are allowed (right now the patch just checks if inline scripts are allowed).
* Eventually implement 'unsafe-inline' value for the style-src directive (though, that might be another bug about the syntax, not the implementation).

Comment 15

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch and some tests 2

The previous patch wasn't actually picking up style attributes, so this one does.  This patch also includes:
* tests for the SMIL animation stuff
* new base violation type (inline styles) for reporting
* tweak in the IDL to separate inline script and inline style allowing
* parser tweak to allow a new options directive value: "inline-style"

Still left todo:
* Block the SMIL style things.  Probably involves patching nsSVGAnimationElement::ParseAttribute()
* implement unsafe-inline for styles

Comment 16

[Ian Melven :imelven]

IMO unsafe-inline should could plausibly be a separate bug blocking this and bug 746978 "sync CSP directive parsing and directive names with w3c CSP 1.0 spec" or alternately just addressed as part of bug 746978

Comment 17

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #16)
> IMO unsafe-inline should could plausibly be a separate bug blocking this and
> bug 746978 "sync CSP directive parsing and directive names with w3c CSP 1.0
> spec" or alternately just addressed as part of bug 746978

in fact, thinking about it more I'd suggest we land this as soon as it's finished (the SMIL stuff) and then add unsafe-inline for CSP 1.0 compliant policies only later either in another bug or in 746978 - unsafe-inline needs to be added to script-src for CSP 1.0 compliant policies as well and it makes more sense to me to do it once and in the same way for both sources.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

I don't think we can land this as soon as it's finished, since old csp-pre-1.0 policies don't expect styles to be blocked.

Let's land this when we land the 1.0 parser.

Also, the 1.0 spec doesn't say anything about SMIL or other things (just style elements and style attributes), so I'm going to split that work out into bug 788337.  There will be lots of test cases to add, anyhow, and edge cases to find.

Comment 19

[Sid Stamm [:geekboy or :sstamm]]

Attachment: patch

This patch enables CSP to block the "inline styles" documented in the 1.0 spec: the style element and the style attribute.  khuey: could you look at this patch?

Comment 20

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 658322 [details] [diff] [review]
patch

And dbaron: could you take a look at the bits that are in /layout/style?

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Should this block other style-like attributes?  e.g. background, bgcolor, marginwidth, marginheight, etc?

Comment 22

[Sid Stamm [:geekboy or :sstamm]]

Reading the CSP 1.0 spec, there is no mention of style-like attributes, just specifically the "style" attribute.

http://www.w3.org/TR/CSP/#style-src

That's kind of a cop-out though, which is why I filed bug 788337.  We should explore other ways strings/attributes/etc can be used to assign style to elements and investigate including that in the spec and our implementation.

But for now, no.  This bug is only concerned with <style> and <foo style="">.

Comment 23

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

The check in nsHTMLCSSStyleSheet::RulesMatching looks kinda expensive; could it be cached someplace?

Otherwise this looks fine, modulo the issue that it's not clear to me what the goal of blocking inline style and not other sources of style data is.

Comment 24

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] from comment #23)
> The check in nsHTMLCSSStyleSheet::RulesMatching looks kinda expensive; could
> it be cached someplace?

Maybe we could store that in a bool, during ParseStyleAttribute() here:
 https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsStyledElement.cpp#232
...and then check that bool in the impl for GetInlineStyleRule() here:
https://mxr.mozilla.org/mozilla-central/source/content/base/src/nsStyledElement.cpp#176
...and return null if the bool is set?

(It looks like that's the only non-trivial GetInlineStyleRule() impl, so I think that's the only spot that'd need tweaking).

Comment 25

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

How often does the result change?  Does it change within the lifetime of a document?

If so, maybe it could be stored in the TreeMatchContext, (ElementRuleProcessorData::mTreeMatchContext), which should be constructed once per subtree-matching operation rather than once per element.

Comment 26

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

We really need to get clarification from the CSP1.0 spec what is meant by "inline stylesheets".

It's quite clear that <style> elements and style="..." attributes that appear in the markup of the page counts as inline style. However, which of the following count as inline style?

* doc.body.appendChild(doc.createElement("style"));
* doc.body.setAttribute("style", "...");
* doc.body.style.background = "...";
* bgcolor attributes appearing in the markup
* <font> elements appearing in the markup
* doc.body.appendChild(doc.createElement("font"));
* doc.body.bgcolor = "...";
* doc.body.innerHTML = "<style>...</style>";

Some of these, are heavily used on the web.

*All* of these would be equivalent to eval() if we had support for IEs old "expression" syntax.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 658322 [details] [diff] [review]
patch

I really think you should come up with a list of all the attributes we want to block first (not just 'style').  I also think you should separate out the CSP logic into a function since once you have that list it will be quite long.

Comment 28

[Sid Stamm [:geekboy or :sstamm]]

khuey: to be clear, you want this bug and 788337 to happen at the same time then?

Comment 29

[Sid Stamm [:geekboy or :sstamm]]

Here's some context in public-webappsec:
http://lists.w3.org/Archives/Public/public-webappsec/2012Sep/0057.html

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think handling SMIL separately is fine, but I think you should handle style, background, bgcolor, font, etc here.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Sid Stamm [:geekboy] from comment #29)
> Here's some context in public-webappsec:
> http://lists.w3.org/Archives/Public/public-webappsec/2012Sep/0057.html

As Boris points out, that list needs more thinking.

You should start with the list of all inline things that affect styling (in Gecko, that means anything that involves attribute mapping, anything that ends up going through nsHTMLCSSStyleSheet (.style) and anything that ends up going through nsHTMLStyleSheet (and maybe some others, dbaron would know more)).

Comment 32

[Sid Stamm [:geekboy or :sstamm]]

Great, thanks for the clarification, Kyle.

Comment 33

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

(In reply to Sid Stamm [:geekboy] from comment #29)
> Here's some context in public-webappsec:
> http://lists.w3.org/Archives/Public/public-webappsec/2012Sep/0057.html

The data-stealing attack described in the article below could be done using just element.style.background = ...;

So while it might "feel" safer, it's arguably not.

http://scarybeastsecurity.blogspot.com.es/2009/12/generic-cross-browser-cross-domain.html

Comment 34

[Daniel Veditz [:dveditz]]

CSP is mostly about preventing text injection. If an attacker has script ability they've already won as far as CSP is concerned. For example, onclick="" is out but it's perfectly fine for script to add a click event listener. We *do* block setAttribute("onclick","") though.

Similarly for style we want to block the textually inline styles that might include insufficiently sanitized user text, but we don't care if a script manipulates the CSSOM. Using script to add text that kicks off the parser is a middle-ground and it's probably fine if we do the easiest or most obvious thing.

The biggest worry with style are selectors that let you do pretty amazing things, including the ability to sniff out anti-CSRF tokens embedded in forms. That's limited to style elements, not attributes.

Attributes are a secondary worry. You could cause some injected HTML to float on top so it functions as a rudimentary phishing XSS. You might be able to accomplish that anyway without style attributes (and without the floating) if the HTML injection was early enough in the page that you could push everything else out of site. It's a minor hurdle at best, but it might help in some situations.

<style> and <tag style=""> are out (explicit in the spec)

* doc.body.innerHTML = "<style>...</style>";
* doc.body.appendChild(doc.createElement("style"));
* doc.body.setAttribute("style", "...");

Webkit's currently blocking these because they kick off the parser and could contain unsanitized text that leads to an injection. I don't think we need to special-case these -- if they happen as a side effect of our basic inline blocking then great, if not that might be fine too. One way or the other, consistency with Chrome is probably more important than which we actually do.

* doc.body.style.background = "...";
* bgcolor attributes appearing in the markup
* <font> elements appearing in the markup
* doc.body.appendChild(doc.createElement("font"));
* doc.body.bgcolor = "...";

I don't think we need to worry about blocking any of those. Simpler is better.

* doc.body.style.cssText = "...."; 

You could argue this is the same as the setAttribute() case, but since I don't feel very strongly about blocking that one I wouldn't put a lot of effort into blocking cssText if it takes additional work.

Comment 35

[Boris Zbarsky [:bzbarsky]]

> That's limited to style elements, not attributes.

For now.  As I said on the list, there have been proposals to change that.  If that gets changed, then what?  It'll be really hard to block things via CSP that we didn't use to once it's widely deployed...

> I don't think we need to worry about blocking any of those. Simpler is better.

"Simpler" would block doc.body.style.background and doc.body.style.cssText but not the other things you list, I think.

Comment 36

[Boris Zbarsky [:bzbarsky]]

But note that this would be simpler in _our_ impl.  Maybe not in others.  Which is why the spec actually needs to define a behavior, not just "simpler"... :(

Comment 37

[Daniel Veditz [:dveditz]]

Comment on attachment 658322 [details] [diff] [review]
patch

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

::: content/base/test/Makefile.in
@@ +360,5 @@
>  		test_CSP_evalscript.html \
>  		file_CSP_evalscript_main.html \
>  		file_CSP_evalscript_main.html^headers^ \
>  		file_CSP_evalscript_main.js \
> +		test_CSP_inlinestyle.html \

This tests that inline styles are blocked by default, but nothing tests that "options inline-style" works to allow those styles.

Also should test that it does whatever we decide is correct for

* doc.body.innerHTML = "<style>...</style>";
* doc.body.appendChild(doc.createElement("style"));
* doc.body.setAttribute("style", "...");
* doc.body.style.cssText = "....";

Comment 38

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

(In reply to Daniel Veditz [:dveditz] from comment #34)
> CSP is mostly about preventing text injection. If an attacker has script
> ability they've already won as far as CSP is concerned.

Of course, but we are blocking access to eval() even though if someone can call eval() we have already lost.

The way I would put it is "CSP's intent is to prevent a page from turning untrusted data into harmful content". Generally that means that we try to prevent a page from turning string data, which could be coming from an untrusted source, into parsed javascript.

But clearly CSP is also considering CSS to be able to be "harmful content". Otherwise we wouldn't be blocking it anywhere, right?

And I think rightfully so. The article at [1] is able to turn CSS into a "send data to my server" mechanism. I.e. by getting untrusted data into CSS, and on the way getting it concatenated with sensitive data, it's able to send sensitive data to an arbitrary server.

[1] http://scarybeastsecurity.blogspot.com.es/2009/12/generic-cross-browser-cross-domain.html

Though, assuming that we apply the img-src policy even to images loaded through CSS, this particular attack should be somewhat mitigated.

I do say somewhat mitigated since people might try harder to prevent injection into markup, but forget that something like:

element.style.background = somedata;

can result in a send-sensitive-data-home attack.

> For example,
> onclick="" is out but it's perfectly fine for script to add a click event
> listener. We *do* block setAttribute("onclick","") though.

Exactly. Because onclick="" and setAttribute("onclick", "") parse string data into possibly harmful script. Whereas addEventListener or .onclick=functionObject does not.

The critical distinction isn't whether you have to call a JS function in order to launch the attack. The distinction is whether that function parses strings into harmful content.

> Similarly for style we want to block the textually inline styles that might
> include insufficiently sanitized user text, but we don't care if a script
> manipulates the CSSOM. Using script to add text that kicks off the parser is
> a middle-ground and it's probably fine if we do the easiest or most obvious
> thing.
> 
> The biggest worry with style are selectors that let you do pretty amazing
> things, including the ability to sniff out anti-CSRF tokens embedded in
> forms. That's limited to style elements, not attributes.
> 
> Attributes are a secondary worry. You could cause some injected HTML to
> float on top so it functions as a rudimentary phishing XSS. You might be
> able to accomplish that anyway without style attributes (and without the
> floating) if the HTML injection was early enough in the page that you could
> push everything else out of site. It's a minor hurdle at best, but it might
> help in some situations.

We've clearly said that CSS can cause some harm. Otherwise we wouldn't be bothering with having *any* CSS-blocking features in CSP.

> <style> and <tag style=""> are out (explicit in the spec)
> 
> * doc.body.innerHTML = "<style>...</style>";
> * doc.body.appendChild(doc.createElement("style"));
> * doc.body.setAttribute("style", "...");
> 
> Webkit's currently blocking these because they kick off the parser and could
> contain unsanitized text that leads to an injection. I don't think we need
> to special-case these -- if they happen as a side effect of our basic inline
> blocking then great, if not that might be fine too. One way or the other,
> consistency with Chrome is probably more important than which we actually do.

I don't see that these are be any different from <style> and <tag style="">.

> * doc.body.style.background = "...";
> * bgcolor attributes appearing in the markup
> * <font> elements appearing in the markup
> * doc.body.appendChild(doc.createElement("font"));
> * doc.body.bgcolor = "...";
> 
> I don't think we need to worry about blocking any of those. Simpler is
> better.

If so, I'd like to better understand what attacks we're trying to prevent by having any CSS blocking features.

I can definitely see that most of these can be used to launch only a very limited set of attacks against the page. The worst one is probably <font> elements appearing in the markup since that can be used to color/hide a large section of the page, but excluding the end tag.

However doc.body.style.background can be used to steal private data as mentioned above.

> * doc.body.style.cssText = "...."; 
> 
> You could argue this is the same as the setAttribute() case

Agreed. This is just different syntax for exactly the same thing. Whatever rule we use for setAttribute("style", ...) we should apply here too.

Comment 39

[Ian Melven :imelven]

Discussion of this and some level of consensus on the w3c webappsec mailing list here : http://lists.w3.org/Archives/Public/public-webappsec/2012Oct/0049.html

Comment 40

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

So I've been thinking that if (1) it's not entirely clear what the full set of attacks that this feature might be useful for blocking is, and (2) once we ship the feature it will be hard to make it block more than it already did, I'm somewhat inclined towards the idea that the blocking of inline style should mean that style sheets aren't parsed at all.

Comment 41

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #40)
> I'm somewhat inclined towards the idea that the blocking of inline
> style should mean that style sheets aren't parsed at all.

To clarify you mean that inline style sheets shouldn't be parsed at all (as opposed to being parsed and not applied) ?

Comment 42

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Ian Melven :imelven from comment #41)
> (In reply to David Baron [:dbaron] from comment #40)
> > I'm somewhat inclined towards the idea that the blocking of inline
> > style should mean that style sheets aren't parsed at all.
> 
> To clarify you mean that inline style sheets shouldn't be parsed at all (as
> opposed to being parsed and not applied) ?

Or are you suggesting when inline styles are not allowed, no stylesheets (inline or otherwise) should load?

Comment 43

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

(In reply to Ian Melven :imelven from comment #41)
> (In reply to David Baron [:dbaron] from comment #40)
> > I'm somewhat inclined towards the idea that the blocking of inline
> > style should mean that style sheets aren't parsed at all.
> 
> To clarify you mean that inline style sheets shouldn't be parsed at all (as
> opposed to being parsed and not applied) ?

Yes.

(In reply to Sid Stamm [:geekboy] from comment #42)
> (In reply to Ian Melven :imelven from comment #41)
> > (In reply to David Baron [:dbaron] from comment #40)
> > > I'm somewhat inclined towards the idea that the blocking of inline
> > > style should mean that style sheets aren't parsed at all.
> > 
> > To clarify you mean that inline style sheets shouldn't be parsed at all (as
> > opposed to being parsed and not applied) ?
> 
> Or are you suggesting when inline styles are not allowed, no stylesheets
> (inline or otherwise) should load?

There's separate stuff for linked style sheets, right?  I didn't mean to include non-inline-styles.

Comment 44

[Sid Stamm [:geekboy or :sstamm]]

(In reply to David Baron [:dbaron] from comment #43)
> There's separate stuff for linked style sheets, right?  I didn't mean to
> include non-inline-styles.

Absolutely.  Sounds like we're in agreement.

Comment 45

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Baron [:dbaron] from comment #40)
> So I've been thinking that if (1) it's not entirely clear what the full set
> of attacks that this feature might be useful for blocking is, and (2) once
> we ship the feature it will be hard to make it block more than it already
> did, I'm somewhat inclined towards the idea that the blocking of inline
> style should mean that style sheets aren't parsed at all.

I would say that not only should they not be parsed, but they should have no observable effect. That is, the semantics should equivalent (or as close as possible) to having a preprocessor remove all the inline style tags before the HTML is parsed.

Comment 46

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #40)
> So I've been thinking that if (1) it's not entirely clear what the full set
> of attacks that this feature might be useful for blocking is, and (2) once
> we ship the feature it will be hard to make it block more than it already
> did, I'm somewhat inclined towards the idea that the blocking of inline
> style should mean that style sheets aren't parsed at all.

After more discussion among the WG, the attacks that we are concerned with mitigating are data stealing selectors (see dveditz's comment 34) and injections that can set any arbitrary CSS property ie. an injection of a string that would be parsed as if it was a style element or attribute. cssText and any other variants should be blocked.

Injections that lead to remote content being loaded are addressed by the other CSP restrictions, for example, changing the background image of a page to do a 'phone home' data exfiltration will be checked against the allowed origins for img-src (or fall back to the default-src). 

We are not concerned with an attacker being able to e.g. change the background of a page to a different color, ie. injections that can't set arbitrary styles are not addressed.

To my understanding, Adam Barth is working on spec language along the above lines to clarify the situation.

I'm look at blocking cssText etc in addition to Sid's existing patches in the near future.

Comment 47

[Ian Melven :imelven]

According to http://dev.w3.org/csswg/cssom/ looks like CSSRule and CSSStyleDeclaration have settable cssText properties.

Comment 48

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #25)
> How often does the result change?  Does it change within the lifetime of a
> document?

With our current and forthcoming 1.0 spec compliant implementation of  CSP, it won't change over the lifetime of a document - but if/when we implement allowing CSP to be specified in a <meta> tag (which is an experimental piece of the CSP 1.1 spec and also bug 663570), it may.

Comment 49

[Ian Melven :imelven]

I'm working on tests for this and seeing some strange behaviour - in an onload handler I do var elem = document.getElementById('csstextstylediv'); elem.style.cssText = "color: #00FF00;"; - I see the text colored green, but getComputedStyle tells me the text's color is rgb(0,0,0)

Sid's existing patch seems to block elem.style.cssText without unsafe-inline which makes sense since it blocks setting elem.style, but we need to make sure the cssText property isn't actually set as well as just not applied.

Comment 50

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #49)
> I'm working on tests for this and seeing some strange behaviour - in an
> onload handler I do var elem = document.getElementById('csstextstylediv');
> elem.style.cssText = "color: #00FF00;"; - I see the text colored green, but
> getComputedStyle tells me the text's color is rgb(0,0,0)

furthermore, when I call getComputedStyle in the child page (where i am modifying elem.style.cssText), it correctly tells me the color is rgb(0, 255, 0) and then the parent's getComputedStyle gets the correct color as well...

Comment 51

[Ian Melven :imelven]

Attachment: patch v2

This takes Sid's patch, updates it for bitrot, and tweaks it to work on top of the patches for bug 783049 and bug 746978. It also adds tests for the CSP 1.0 header allowing and blocking inline styles, including setting cssText. It also caches if inline styles are allowed by CSP on the TreeMatchContext as suggested by dbaron in comment 25.

Comment 52

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

Comment on attachment 695072 [details] [diff] [review]
patch v2

Please rev the IID of nsIContentSecurityPolicy.idl

Please factor out this code that you've copy & pasted to three
different places into a single function:
  bool
  nsStyleUtil::CheckCSPForInlineStyle(nsIPrincipal* aPrincipal,
                                      nsIURI* aSourceURI,
                                      uint32_t aLineNumber,
                                      const nsSubstring& aStyleText,
                                      nsresult* aRv);
that contains the common logic (including the truncation of the text to
40 characters), returns false when the inline style should be blocked,
and sets *aRv to any non-NS_OK return value that should be propagated
(maybe only if aRv is non-null).  Also remove the includes of
nsIContentSecurityPolicy.h that you no longer need to add once you've
added this function.

In nsStyledElementNotElementCSSInlineStyle::ParseAttribute:
>+      nsIDocument* doc = OwnerDoc();
>+      if (doc) {

Unnecessary null-check.


I think you should drop the nsHTMLCSSStyleSheet.cpp and the
nsRuleProcessorData.h changes; I don't think they're needed anymore now
that you're blocking earlier.  If you think they are, could you explain?

Somebody else should review the CSPUtils.jsm and
contentSecurityPolicy.js code, and the tests.

r=dbaron on the rest

Comment 53

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

You're also missing some callers that you need to modify, though:

  nsSVGElement.cpp calls ParseStyleAttribute in nsStyledElement (could could maybe fix this by refactoring one of your existing changes)
  nsXULElement.cpp calls nsCSSParser::ParseStyleAttribute directly

You also probably need to block any of the callers of nsCSSParser::ParseProperty that deal with inline style; I think there are a bunch.

I think that means I should have a look at the revised patch.

Comment 54

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #52)

Thanks for taking a look ! 

> Comment on attachment 695072 [details] [diff] [review]
> patch v2
> 
> Please rev the IID of nsIContentSecurityPolicy.idl

This is taken care of in part 2 of bug 783049 - we would like to land this, bug 783049 and bug 736978 - after that we can flip the pref to turn on all the new CSP spec compliant functionality.

> Please factor out this code that you've copy & pasted to three
> different places into a single function:
>   bool
>   nsStyleUtil::CheckCSPForInlineStyle(nsIPrincipal* aPrincipal,
>                                       nsIURI* aSourceURI,
>                                       uint32_t aLineNumber,
>                                       const nsSubstring& aStyleText,
>                                       nsresult* aRv);
> that contains the common logic (including the truncation of the text to
> 40 characters), returns false when the inline style should be blocked,
> and sets *aRv to any non-NS_OK return value that should be propagated
> (maybe only if aRv is non-null).  Also remove the includes of
> nsIContentSecurityPolicy.h that you no longer need to add once you've
> added this function.

great suggestion, thanks, will do.

> In nsStyledElementNotElementCSSInlineStyle::ParseAttribute:
> >+      nsIDocument* doc = OwnerDoc();
> >+      if (doc) {
> 
> Unnecessary null-check.

thanks, i'll remove this.

> I think you should drop the nsHTMLCSSStyleSheet.cpp and the
> nsRuleProcessorData.h changes; I don't think they're needed anymore now
> that you're blocking earlier.  If you think they are, could you explain?

when I take out the inlineOK check in nsHTMLCSSStyleSheet::RulesMatching, this test case in the tests fails :

<div id='attrstylediv' style="color: #FF0000;">Attribute stylesheet test (should be black)</div>

(ie. the text is red instead of black)

so it seems like this at least isn't blocked elsewhere.

> Somebody else should review the CSPUtils.jsm and
> contentSecurityPolicy.js code, and the tests.
 
I'll have Sid take a look at these.

Comment 55

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #54)
> (In reply to David Baron [:dbaron] from comment #52)
> >
> > I think you should drop the nsHTMLCSSStyleSheet.cpp and the
> > nsRuleProcessorData.h changes; I don't think they're needed anymore now
> > that you're blocking earlier.  If you think they are, could you explain?
> 
> when I take out the inlineOK check in nsHTMLCSSStyleSheet::RulesMatching,
> this test case in the tests fails :
> 
> <div id='attrstylediv' style="color: #FF0000;">Attribute stylesheet test
> (should be black)</div>
> 
> (ie. the text is red instead of black)
> 
> so it seems like this at least isn't blocked elsewhere.

ah, i think i see what's happening here - Sid's comment explains it : 
          // We posted the violation warning, but don't block it here --
          // we'll block it when the style is applied (that's when the spec
          // says to do it).

so the ParseAttribute call correctly detects (and reports) the style attribute set should be blocked, but it's not actually blocked until the style is applied via nsHTMLCSSStyleSheet::RulesMatching

I'm assuming the piece of the spec (http://www.w3.org/TR/CSP/#style-src) the comment refers to is :
"Whenever the user agent would apply style from a style attribute, instead the user agent must ignore the style."

Comment 56

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #54)
>
> This is taken care of in part 2 of bug 783049 - we would like to land this,
> bug 783049 and bug 736978 - after that we can flip the pref to turn on all
> the new CSP spec compliant functionality.

typo, should be bug 746978

Comment 57

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

(In reply to Ian Melven :imelven from comment #55)
> ah, i think i see what's happening here - Sid's comment explains it : 
>           // We posted the violation warning, but don't block it here --
>           // we'll block it when the style is applied (that's when the spec
>           // says to do it).

I think that's reading *way* too much into what the specification says.  See http://dbaron.org/log/20100531-specs

I don't think it makes sense to block style elements and style attributes at totally different layers; I think both should not be parsed (which is what this patch apparently does for style elements but not style attributes, since my eyes glazed over the same code copy-pasted three times).

Comment 58

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #57)
> (In reply to Ian Melven :imelven from comment #55)
> > ah, i think i see what's happening here - Sid's comment explains it : 
> >           // We posted the violation warning, but don't block it here --
> >           // we'll block it when the style is applied (that's when the spec
> >           // says to do it).
> 
> I think that's reading *way* too much into what the specification says.  See
> http://dbaron.org/log/20100531-specs

yes, it does seem rather literal. Thanks for sharing your post, it was very good !

> I don't think it makes sense to block style elements and style attributes at
> totally different layers; I think both should not be parsed (which is what
> this patch apparently does for style elements but not style attributes,
> since my eyes glazed over the same code copy-pasted three times).

sorry about the duplicate code, i've refactored the patch as you suggested to use a nsStyleUtil::CheckCSPForInlineStyle and addressed your other review comments - i agree that it's much cleaner (and reduces chance for bugs) to have both elements and attributes blocked in the same place, i'll make this change as well.

Comment 59

[Ian Melven :imelven]

Attachment: patch v3

Address review feedback from dbaron. Also r?'ing to Sid to look at the CSP specific bits and tests.

* I refactored using nsStyleUtil::CheckCSPForInlineStyle as suggested, thanks !

* I moved the check in nsStyledElementNotElementCSSInlineStyle to ParseStyleAttribute instead of ParseAttribute.

* I also changed that check to block right away and removed the changes to nsHTMLCSSStyleSheet.cpp and nsRuleProcessorData.h as discussed.

Here are my thoughts on the points raised in comment 53:

* nsSVGElement.cpp calls ParseStyleAttribute in nsStyledElement (could maybe fix this by refactoring one of your existing changes)

i did refactor this, but having looked at http://www.w3.org/TR/SVG/styling.html it seems like the main threats is 'defacement', which we're not trying to address with CSP. SVG has CSS selectors, but i (hopefully) assume they're scoped to the SVG itself and can't mess with other bits of the document.

*  nsXULElement.cpp calls nsCSSParser::ParseStyleAttribute directly

chrome documents can't have a CSP (yet - a lot of folks would like to see this happen, but it's a large project), so it seems ok to omit a check here for now

> You also probably need to block any of the callers of 
> nsCSSParser::ParseProperty that deal with inline style; I think there are a 
> bunch.

These are the callers of nsCSSParser::ParseProperty :

* nsDOMCSSDeclaration::ParsePropertyValue
layout/style/nsDOMCSSDeclaration.cpp:240:22

    240:  nsDOMCSSDeclaration::ParsePropertyValue(const nsCSSProperty aPropID,
    241:   const nsAString& aPropValue,
    242:   bool aIsImportant)

For setting CSS property values, based on previous discussions (see comment 46), we only care about cssText, which should already be covered by this patch. If there are other properties which can be parsed into arbitrary properties like cssText, those should be blocked as well. 

* BuildStyleRule
layout/style/nsStyleAnimation.cpp:2207:1

    2207:  BuildStyleRule(nsCSSProperty aProperty,
    2208:   dom::Element* aTargetElement,
    2209:   const nsAString& aSpecifiedValue,

Sid filed a follow up to block animations (bug 788337)
but if the threats from them boil down to defacement only, that seems like a WONTFIX

* ::MappedAttrParser::ParseMappedAttrValue
content/svg/content/src/nsSVGElement.cpp:1253:19

    1253:  MappedAttrParser::ParseMappedAttrValue(nsIAtom* aMappedAttrName,
    1254:   const nsAString& aMappedAttrValue)
    1255:  {

same comment as above re SVG - threat seems limited to defacement

* mozilla::dom::CreateFontStyleRule
content/canvas/src/CanvasRenderingContext2D.cpp:1929:1

    1929:  CreateFontStyleRule(const nsAString& aFont,
    1930:   nsINode* aNode,
    1931:   StyleRule** aResult)

any font based phone-home should be blocked by the font-src rule from the CSP (or the default-src rule if there's no font-src), so this seems like it's limited to defacement also.

Comment 60

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 696791 [details] [diff] [review]
patch v3

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

A few minor nits here and there.  r=me on the CSP bits, assuming the getComputedStyle call is required for proper testing (in file_CSP_inlinestyle_main_spec_compliant_allowed.html -- OMG that's a long file name).

::: content/base/src/CSPUtils.jsm
@@ +181,5 @@
>    // Default to false if not specified.
>    this._specCompliant = (aSpecCompliant !== undefined) ? aSpecCompliant : false;
> +
> +  // Only CSP 1.0 spec compliant policies block inline styles.
> +  this._allowInlineStyles = (aSpecCompliant !== undefined) ? !aSpecCompliant : true;

Maybe use this._specCompliant since the logic is already used above:

this._allowInlineStyles = !this._specCompliant;

::: content/base/test/file_CSP_inlinestyle_main.html
@@ +15,5 @@
> +    </style>
> +
> +    <div id='linkstylediv'>Link tag (external) stylesheet test (should be green)</div>
> +    <div id='attrstylediv' style="color: #00ff00;">Attribute stylesheet test (should be green)</div>
> +    <div id='inlinestylediv'>Inline stylesheet test (should be green)</div>

Please put a comment in this file somewhere to remind the reader that CSP should *not* block inline styles for this test (since it's pre-spec CSP).

::: content/base/test/file_CSP_inlinestyle_main_spec_compliant_allowed.html
@@ +13,5 @@
> +
> +        // REVIEW : if i have this below statement, this test passes as the parent page
> +        // correctly detects that the text is colored green - if i remove this, getComputedStyle
> +        // thinks the text is black when called by the parent page
> +        getComputedStyle(elem, null).color;

I'm guessing this is because there's a race going on between test finish and the style being applied, but it's just a guess.  dbaron: is this the right way to force the style to be applied for this test?

Also: might make sense to put a comment in here that explains how this test is gonna fail if "script-src 'unsafe-inline'" is broken...

Comment 61

[Ian Melven :imelven]

Attachment: patch v4

Addressed Sid's review comments. Thank you for the review, Sid.

Comment 62

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

(In reply to Ian Melven :imelven from comment #59)
> Here are my thoughts on the points raised in comment 53:
> 
> * nsSVGElement.cpp calls ParseStyleAttribute in nsStyledElement (could maybe
> fix this by refactoring one of your existing changes)
> 
> i did refactor this, but having looked at
> http://www.w3.org/TR/SVG/styling.html it seems like the main threats is
> 'defacement', which we're not trying to address with CSP. SVG has CSS
> selectors, but i (hopefully) assume they're scoped to the SVG itself and
> can't mess with other bits of the document.

I don't see how SVG is any different from HTML here.  What's the threat here that's something other than what you're calling "defacement"?

> > You also probably need to block any of the callers of 
> > nsCSSParser::ParseProperty that deal with inline style; I think there are a 
> > bunch.
> 
> These are the callers of nsCSSParser::ParseProperty :
> 
> * nsDOMCSSDeclaration::ParsePropertyValue
> layout/style/nsDOMCSSDeclaration.cpp:240:22
> 
>     240:  nsDOMCSSDeclaration::ParsePropertyValue(const nsCSSProperty
> aPropID,
>     241:   const nsAString& aPropValue,
>     242:   bool aIsImportant)
> 
> For setting CSS property values, based on previous discussions (see comment
> 46), we only care about cssText, which should already be covered by this
> patch. If there are other properties which can be parsed into arbitrary
> properties like cssText, those should be blocked as well. 

The functionality of ParsePropertyValue is identical to the functionality of the cssText setter on a declaration.  Neither has selectors; both allow setting any property to any value.

> * BuildStyleRule
> layout/style/nsStyleAnimation.cpp:2207:1
> 
>     2207:  BuildStyleRule(nsCSSProperty aProperty,
>     2208:   dom::Element* aTargetElement,
>     2209:   const nsAString& aSpecifiedValue,
> 
> Sid filed a follow up to block animations (bug 788337)
> but if the threats from them boil down to defacement only, that seems like a
> WONTFIX

Animations may soon allow setting any property to any value.  (They already allow setting many properties to many values.)  You should block them here.


> * ::MappedAttrParser::ParseMappedAttrValue
> content/svg/content/src/nsSVGElement.cpp:1253:19
> 
>     1253:  MappedAttrParser::ParseMappedAttrValue(nsIAtom* aMappedAttrName,
>     1254:   const nsAString& aMappedAttrValue)
>     1255:  {
> 
> same comment as above re SVG - threat seems limited to defacement

I don't understand what here is not "defacement" other than threats of selector attacks.

I also think that you're not going to be able to change what this blocks later (because of compatibility), so it better block the thing it sounds like it says it blocks now, in case there are future attacks that do require blocking what it sounds like it blocks.

> * mozilla::dom::CreateFontStyleRule
> content/canvas/src/CanvasRenderingContext2D.cpp:1929:1
> 
>     1929:  CreateFontStyleRule(const nsAString& aFont,
>     1930:   nsINode* aNode,
>     1931:   StyleRule** aResult)
> 
> any font based phone-home should be blocked by the font-src rule from the
> CSP (or the default-src rule if there's no font-src), so this seems like
> it's limited to defacement also.

I'm not worried about this one.

Comment 63

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

Comment on attachment 697162 [details] [diff] [review]
patch v4

I still think you should put an IID rev in each patch that touches
nsIContentSecurityPolicy, even if you're landing 3 of them at once.
(One of them might get backed out.)

StyleRule.cpp:

  I don't think that comment is true; if this CSP policy is set, it
  shouldn't be possible to create any style rules for inline style.

  That said, it is today possible for script to get access to style
  rules for linked styles.  Should this policy block those?  Or should
  it be assumed that anybody using this will be blocking script as well?

nsDOMCSSDeclaration.cpp:

  It's still not clear to me what the threat model is that implies
  you should block SetCssText but not block SetProperty or
  SetPropertyValue (the latter being what's used to implement the
  CSS2Properties interface).

Could you rename nsStyleUtil::CheckCSPForInlineStyle to something
that makes the true/false return values clearer?  Maybe
CSPAllowsInlineStyle?  (Sorry, my fault for the bad suggestion
earlier.)

nsStyleUtil.cpp:

  In the two NS_FAILED(rv) cases, it seems like it would make more sense
  to return false.  Those failures shouldn't happen... but if they do,
  we should probably know about them.  (This will only change the
  results in
  nsStyledElementNotElementCSSInlineStyle::ParseStyleAttribute... by
  making them consistent with the handling in the other cases.)

  Furthermore, I think you should have an:
    if (aRv) {
      *aRv = NS_OK;
    }
  at the start of the method, so it always initializes aRv.


nsStyleLinkElement.cpp:

  >+    // Check CSP to see if inline stylesheets are allowed
  >+    bool inlineOK = nsStyleUtil::CheckCSPForInlineStyle(doc->NodePrincipal(),
  >+                                                        doc->GetDocumentURI(),
  >+                                                        mLineNumber, text,
  >+                                                        &rv);
  >+    NS_ENSURE_SUCCESS(rv, rv);
  >+    if (!inlineOK)
  >+      return NS_OK; 

  This can be simplified to:

  +    if (!nsStyleUtil::CheckCSPForInlineStyle(doc->NodePrincipal(),
  +                                             doc->GetDocumentURI(),
  +                                             mLineNumber, text,
  +                                             &rv)) {
  +      return rv; 
  +    }

  (Note that I'm suggesting you remove the comment; it's only
  saying the obvious.)

nsStyledElement.cpp:

  Again, no need for the inlineOK variable.

nsDOMCSSDeclaration.cpp (again):

  Same simplification as nsStyleLinkElement.cpp.

Comment 64

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

(In reply to David Baron [:dbaron] from comment #63)
> nsDOMCSSDeclaration.cpp:
> 
>   It's still not clear to me what the threat model is that implies
>   you should block SetCssText but not block SetProperty or
>   SetPropertyValue (the latter being what's used to implement the
>   CSS2Properties interface).

Also, to be clear, these two could be blocked in the common function that they call, SetPropertyValue.  (It doesn't seem like blocking RemoveProperty is necessary, unless the CSP result can change over time.)

Comment 65

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

Also, the nsDOMCSSDeclaration::SetCSSText changes are doing blocking for non-inline style (i.e., blocking for manipulation of stuff in linked style sheets), but nothing else is.  That seems inconsistent.  (See also second paragraph of StyleRule.cpp comments in comment 63; I might prefer blocking to not-blocking, but I think we should be consistent about what it is that we're blocking.)

Comment 66

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

(In reply to David Baron [:dbaron] from comment #62)
> I also think that you're not going to be able to change what this blocks
> later (because of compatibility), so it better block the thing it sounds
> like it says it blocks now, in case there are future attacks that do require
> blocking what it sounds like it blocks.

I actually want to repeat this:  this is basically why I'm minusing review requests here until I think it's right:  once people start using this, we're not going to have the chance to block more things later, because that will risk Web-compatibility problems.  So if it ought to block something, it needs to block it now.

Comment 67

[Ian Melven :imelven]

David, thank you for the review ! I really appreciate the thoroughness and that you called out your guiding principle in comment 66. 

(In reply to David Baron [:dbaron] from comment #62)
> (In reply to Ian Melven :imelven from comment #59)
> > Here are my thoughts on the points raised in comment 53:
> > 
> > * nsSVGElement.cpp calls ParseStyleAttribute in nsStyledElement (could maybe
> > fix this by refactoring one of your existing changes)
> > 
> > i did refactor this, but having looked at
> > http://www.w3.org/TR/SVG/styling.html it seems like the main threats is
> > 'defacement', which we're not trying to address with CSP. SVG has CSS
> > selectors, but i (hopefully) assume they're scoped to the SVG itself and
> > can't mess with other bits of the document.
> 
> I don't see how SVG is any different from HTML here.  What's the threat here
> that's something other than what you're calling "defacement"?

the 'selectors stealing passwords from password field' threat - I'm not sure if this would be possible for CSS in an SVG but if CSS in an SVG can contain selectors that could read fields in the document containing the SVG, we should add a check here. 

By "defacement", I mean that an injected style could change the appearance of a page eg change colors or fonts etc. 
 
> > These are the callers of nsCSSParser::ParseProperty :
> > 
> > * nsDOMCSSDeclaration::ParsePropertyValue
> > layout/style/nsDOMCSSDeclaration.cpp:240:22
> > 
> >     240:  nsDOMCSSDeclaration::ParsePropertyValue(const nsCSSProperty
> > aPropID,
> >     241:   const nsAString& aPropValue,
> >     242:   bool aIsImportant)
> > 
> > For setting CSS property values, based on previous discussions (see comment
> > 46), we only care about cssText, which should already be covered by this
> > patch. If there are other properties which can be parsed into arbitrary
> > properties like cssText, those should be blocked as well. 
> 
> The functionality of ParsePropertyValue is identical to the functionality of
> the cssText setter on a declaration.  Neither has selectors; both allow
> setting any property to any value.

ah, I didn't think ParsePropertyValue was exposed to content, but it seems as though it is - I will add this check. 

> > * BuildStyleRule
> > layout/style/nsStyleAnimation.cpp:2207:1
> > 
> >     2207:  BuildStyleRule(nsCSSProperty aProperty,
> >     2208:   dom::Element* aTargetElement,
> >     2209:   const nsAString& aSpecifiedValue,
> > 
> > Sid filed a follow up to block animations (bug 788337)
> > but if the threats from them boil down to defacement only, that seems like a
> > WONTFIX
> 
> Animations may soon allow setting any property to any value.  (They already
> allow setting many properties to many values.)  You should block them here.

thanks for the heads up on what might happen with animation - I will add this check.

> 
> > * ::MappedAttrParser::ParseMappedAttrValue
> > content/svg/content/src/nsSVGElement.cpp:1253:19
> > 
> >     1253:  MappedAttrParser::ParseMappedAttrValue(nsIAtom* aMappedAttrName,
> >     1254:   const nsAString& aMappedAttrValue)
> >     1255:  {
> > 
> > same comment as above re SVG - threat seems limited to defacement
> 
> I don't understand what here is not "defacement" other than threats of
> selector attacks.

That's the only threat we're really trying to address with blocking inline styles. Selectors, the 'phone home' attack which should be mitigated by the other *-src CSP directives) and 'defacement' are the ones that have been discussed in the WebAppSec WG.

> I also think that you're not going to be able to change what this blocks
> later (because of compatibility), so it better block the thing it sounds
> like it says it blocks now, in case there are future attacks that do require
> blocking what it sounds like it blocks.

ok, I will add a check here.

Comment 68

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #63)
> Comment on attachment 697162 [details] [diff] [review]
> patch v4
> 
> I still think you should put an IID rev in each patch that touches
> nsIContentSecurityPolicy, even if you're landing 3 of them at once.
> (One of them might get backed out.)

that is a very good point, thank you.

> StyleRule.cpp:
> 
>   I don't think that comment is true; if this CSP policy is set, it
>   shouldn't be possible to create any style rules for inline style.

I will fix this.

>   That said, it is today possible for script to get access to style
>   rules for linked styles.  Should this policy block those?  Or should
>   it be assumed that anybody using this will be blocking script as well?

I think it's acceptable to rely on the inline script blocking here to prevent this issue. 

> nsDOMCSSDeclaration.cpp:
> 
>   It's still not clear to me what the threat model is that implies
>   you should block SetCssText but not block SetProperty or
>   SetPropertyValue (the latter being what's used to implement the
>   CSS2Properties interface).

As I understand it, SetCssText is similar to being able to set a .style attribute that can set any arbitrary CSS property. Looking at SetPropertyValue/CSS2Properties it seems like it can indeed be used to set a variety of CSS properties but there's an explicit list. Part of the concern with .cssText is that new properties exposing new threats may be added and immediately would be exposed via .cssText. In discussion with Jonas and Adam Barth and Boris, we want to have some level of interoperability with Webkit, which blocks <style> and .style (not .cssText, although there's agreement it's a risk) but does not block elem.color and setting specific CSS properties from script - again, blocking inline script helps mitigate issues here, although the threats seem to be 'defacement' and 'phone home' attacks, and not the selector threat we are most concerned about. 

> Could you rename nsStyleUtil::CheckCSPForInlineStyle to something
> that makes the true/false return values clearer?  Maybe
> CSPAllowsInlineStyle?  (Sorry, my fault for the bad suggestion
> earlier.)

no problem, I'll make this change.

> nsStyleUtil.cpp:
> 
>   In the two NS_FAILED(rv) cases, it seems like it would make more sense
>   to return false.  Those failures shouldn't happen... but if they do,
>   we should probably know about them.  (This will only change the
>   results in
>   nsStyledElementNotElementCSSInlineStyle::ParseStyleAttribute... by
>   making them consistent with the handling in the other cases.)
> 
>   Furthermore, I think you should have an:
>     if (aRv) {
>       *aRv = NS_OK;
>     }
>   at the start of the method, so it always initializes aRv.

good suggestions, thanks.

> 
> nsStyleLinkElement.cpp:
> 
>   >+    // Check CSP to see if inline stylesheets are allowed
>   >+    bool inlineOK =
> nsStyleUtil::CheckCSPForInlineStyle(doc->NodePrincipal(),
>   >+                                                       
> doc->GetDocumentURI(),
>   >+                                                        mLineNumber,
> text,
>   >+                                                        &rv);
>   >+    NS_ENSURE_SUCCESS(rv, rv);
>   >+    if (!inlineOK)
>   >+      return NS_OK; 
> 
>   This can be simplified to:
> 
>   +    if (!nsStyleUtil::CheckCSPForInlineStyle(doc->NodePrincipal(),
>   +                                             doc->GetDocumentURI(),
>   +                                             mLineNumber, text,
>   +                                             &rv)) {
>   +      return rv; 
>   +    }
> 
>   (Note that I'm suggesting you remove the comment; it's only
>   saying the obvious.)
> 
> nsStyledElement.cpp:
> 
>   Again, no need for the inlineOK variable.
> 
> nsDOMCSSDeclaration.cpp (again):
> 
>   Same simplification as nsStyleLinkElement.cpp.

I will make all these changes.

Comment 69

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #64)
> (In reply to David Baron [:dbaron] from comment #63)
> > nsDOMCSSDeclaration.cpp:
> > 
> >   It's still not clear to me what the threat model is that implies
> >   you should block SetCssText but not block SetProperty or
> >   SetPropertyValue (the latter being what's used to implement the
> >   CSS2Properties interface).
> 
> Also, to be clear, these two could be blocked in the common function that
> they call, SetPropertyValue.  (It doesn't seem like blocking RemoveProperty
> is necessary, unless the CSP result can change over time.)

once CSP has <meta> support, the CSP result can but it only get more restrictive, ie. the initial CSP in an HTTP header could allow unsafe-inline and then CSP in a <meta> could block inline script, but not the inverse.

Comment 70

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #65)
> Also, the nsDOMCSSDeclaration::SetCSSText changes are doing blocking for
> non-inline style (i.e., blocking for manipulation of stuff in linked style
> sheets), but nothing else is.  That seems inconsistent.  (See also second
> paragraph of StyleRule.cpp comments in comment 63; I might prefer blocking
> to not-blocking, but I think we should be consistent about what it is that
> we're blocking.)

Great point, I think I would prefer to only block setting cssText for an inline style - is there a straight forward way to do this in nsDOMCSSDeclaration::SetCSSText ?

I agree that consistency is important here.

Comment 71

[Ian Melven :imelven]

A note to myself that there are tests for animations etc in bug 788337 which is being marked a dupe of this bug since I'm going to handle animations and style manipulations in this bug as discussed previously with dbaron

Comment 73

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #70)
> (In reply to David Baron [:dbaron] from comment #65)
> > Also, the nsDOMCSSDeclaration::SetCSSText changes are doing blocking for
> > non-inline style (i.e., blocking for manipulation of stuff in linked style
> > sheets), but nothing else is.  That seems inconsistent.  (See also second
> > paragraph of StyleRule.cpp comments in comment 63; I might prefer blocking
> > to not-blocking, but I think we should be consistent about what it is that
> > we're blocking.)
> 
> Great point, I think I would prefer to only block setting cssText for an
> inline style - is there a straight forward way to do this in
> nsDOMCSSDeclaration::SetCSSText ?
> 
> I agree that consistency is important here.

The referenced bit of comment 63 is : 

  That said, it is today possible for script to get access to style
  rules for linked styles.  Should this policy block those?  Or should
  it be assumed that anybody using this will be blocking script as well?

Actually, I now think I prefer blocking modifying styles loaded from style sheets via script as well. Even though inline script is more dangerous than inline styles, opting into inline script shouldn't necessarily mean opting into inline styles unless done so explicitly as well.

Comment 74

[Ian Melven :imelven]

Please see http://lists.w3.org/Archives/Public/public-webappsec/2013Feb/0015.html

Comment 75

[Ian Melven :imelven]

Attachment: patch v5

Address dbaron's review comments.

This patch is very restrictive in terms of blocking inline styles, it blocks everything as discussed with dbaron during patch reviews :

* setting styles via CSSOM (setProperty, removeProperty, cssText, various style attributes like elem.color)

Note that the CSP 1.0 spec now explicitly says "The user agent is also not prevented from applying style from Cascading Style Sheets Object Model (CSSOM) 

Also note that dbaron has pointed out that it seems inconsistent to block .style attributes, but allow setting styles via CSSOM, which I agree with.

* modifying rules from style sheets allowed to be loaded via style-src/default-src

* allowing SVG elements to set styles

* SMIL animations (it includes the tests from bug 788337)

I'm not marking this for review just yet - I'd like to see the response (if any) to the w3c webappsec list post I linked in the previous comment.

Comment 76

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #75)
> Created attachment 711962 [details] [diff] [review]
> patch v5

this needs this crash :

TEST-UNEXPECTED-FAIL | /tests/layout/style/test/test_rules_out_of_sheets.html | Exited with code 11 during test run
PROCESS-CRASH | /tests/layout/style/test/test_rules_out_of_sheets.html | application crashed [@ nsDOMCSSDeclaration::RemoveProperty(nsCSSProperty)]

fixed as well

Comment 77

[Ian Melven :imelven]

Attachment: patch v6

Fix the mochitest crashes, pushed to try again: 

https://tbpl.mozilla.org/?tree=Try&rev=fd1acb491461

Comment 78

[Ian Melven :imelven]

The webappsec WG discussed inline styles again during the 2/12 teleconference. 

I agreed to propose some text for the spec around SMIL animations. I also brought up the inconsistency between blocking the style attribute and allowing CSSOM modifications - my personal feeling is that when .style is blocked, CSSOM should be blocked, ie. I agree with the point dbaron brought up previously in this bug, there's (at the current time) no difference between the two.

I also suggested something along the lines of what bsmith had suggested, changing unsafe-inline in a way that could block only the style element and not the .style attribute.

My thinking here currently is that there's a set of ways to apply styles that come from an element : a <style> element or an SVG element or an <animate> element

There's another set of ways to apply styles that are used by script:
the .style attribute, other 'shorthand' attributes (eg. elem.color rather than elem.style.color) and the CSSOM (including .cssText). 

The important point here is that the ability of scripts to run will be governed by the script-src directive which we hope will usually NOT specify unsafe-inline (this is largely the point of CSP - to prevent script injections). In this case, without the ability to inject script, trying to apply styles via script will be blocked, regardless of whether style-src: unsafe-inline is used or not.

unsafe-inline should block both of these sets for backwards compatibility.

The directive to block element based styles and allow attribute/CSSOM etc based styles to be applied could be named something like 'unsafe-inline-scripted' 
(which unless there are other suggestions is what I'll propose initially to the webappsec WG). 

I'm going to r? this to dbaron. There are definitely some indentation issues in the patch because I'm not exactly sure to do when we have long arguments that don't play nicely with our 80 char line limit - advice there would be awesome. 

Other than that, everything we have discussed blocking should be blocked by this patch per comment 75. The patch also has tests for the things we've discussed blocking being correctly blocked/allowed based on whether unsafe-inline was specified in the CSP. 

Regardless of the spec discussion, in my opinion, we could land this (if it passes review, of course), and then follow up with implementing the new, less restrictive (and IMO, more useful) style-src directive to block only element based applied styles after more discussion in the WG and after we see what ends up in the spec.

Feedback very welcome ! I plan to bring this up for discussion in tomorrow's Security Engineering meeting as well.

Comment 79

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

Comment on attachment 713681 [details] [diff] [review]
patch v6

The use of env.mCSSLoader in nsDOMCSSDeclaration::ParsePropertyValue and
nsDOMCSSDeclaration::RemoveProperty is pretty weird.

I originally wrote this, but don't do this:
// Instead, just add an nsIDocument *mDocument to struct
// CSSParsingEnvironment in nsDOMCSSDeclaration.h and then update:
//   nsDOMCSSAttributeDeclaration::GetCSSParsingEnvironment
//   nsDOMCSSDeclaration::GetCSSParsingEnvironmentForRule
// to fill in the nsIDocument* pointer that they already have, and then
// use that in ParsePropertyValue and RemoveProperty (without checking it).

Instead, just use env.mPrincipal and env.mSheetURI throughout (all
changes in nsDOMCSSDeclaration), instead of trying to get them off the
document.


Also, in nsStyleLinkElement, use thisContent->NodePrincipal() instead
of doc->NodePrincipal().

And in nsStyledElementNotElementCSSInlineStyle, use NodePrincipal()
instead of doc->NodePrincipal().


r=dbaron with those changes.

Sorry for the delay getting to this.


You should probably also leave a clear TODO somewhere about not having
the check for XULElement (or add the check).

Comment 80

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #79)
> Comment on attachment 713681 [details] [diff] [review]
> patch v6
> 
> The use of env.mCSSLoader in nsDOMCSSDeclaration::ParsePropertyValue and
> nsDOMCSSDeclaration::RemoveProperty is pretty weird.
> 
> I originally wrote this, but don't do this:
> // Instead, just add an nsIDocument *mDocument to struct
> // CSSParsingEnvironment in nsDOMCSSDeclaration.h and then update:
> //   nsDOMCSSAttributeDeclaration::GetCSSParsingEnvironment
> //   nsDOMCSSDeclaration::GetCSSParsingEnvironmentForRule
> // to fill in the nsIDocument* pointer that they already have, and then
> // use that in ParsePropertyValue and RemoveProperty (without checking it).
> 
> Instead, just use env.mPrincipal and env.mSheetURI throughout (all
> changes in nsDOMCSSDeclaration), instead of trying to get them off the
> document.

that is what I was doing originally (or Sid was, rather) - but in the case where
an existing style sheet is being modified via the CSSOM, env.mPrincipal was the principal of the style sheet, not the document, and hence didn't have any CSP on it, so the modifications were incorrectly allowed.

> Also, in nsStyleLinkElement, use thisContent->NodePrincipal() instead
> of doc->NodePrincipal().

i'll give this a go, the tests should catch any issues.

> And in nsStyledElementNotElementCSSInlineStyle, use NodePrincipal()
> instead of doc->NodePrincipal().

likewise.

> 
> r=dbaron with those changes.
> 
> Sorry for the delay getting to this.

no problem, thanks for all the help on this ! 

> You should probably also leave a clear TODO somewhere about not having
> the check for XULElement (or add the check).

I'll add the comment for now to avoid the perf hit.

There's still some debate about blocking CSSOM or not (the spec says it should be allowed), currently bsmith and I are discussing further.  

I assume your position is still that if .style is blocked, CSSOM should also be blocked since they're equivalent in what they can do ?

Comment 81

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

So if you have to have the document principal, you should clearly describe that in the comments for the nsStyleUtil method.

And you should also, then, do the bit that I commented out in the review comments.

Comment 82

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] from comment #81)
> So if you have to have the document principal, you should clearly describe
> that in the comments for the nsStyleUtil method.
> 
> And you should also, then, do the bit that I commented out in the review
> comments.

Sounds good, thanks !

Comment 83

[Ian Melven :imelven]

Attachment: patch v7

Address dbaron's review feedback, marking r+

Thanks very much for all your help with this, dbaron !

Comment 84

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=c81e900306d1

Comment 85

[Sid Stamm [:geekboy or :sstamm]]

Hm, looks like the patch is causing a stylesheet test to crash in
layout/style/test/test_rules_out_of_sheets.html

See also potentially related bug 634373.

Comment 86

[Boris Zbarsky [:bzbarsky]]

>+  aCSSParseEnv.mDocument = sheet->GetOwningDocument();

Sheet can be null there.

Comment 87

[Ian Melven :imelven]

Sid: yeah, I fixed test_rules_out_of_sheets.html in the patch in comment 77 but reintroduced the crash when addressing the review feedback. I re-fixed it and pushed to try but there were other problems.

bz: Thanks for the tip on sheet being able to be null there. I'll account for that as well. 

For anyone who may be tempted to land the r+ patch: this is close but definitely not quite ready yet, so please hold off.

Comment 88

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #86)
> >+  aCSSParseEnv.mDocument = sheet->GetOwningDocument();
> 
> Sheet can be null there.

Looks like this previous code should take care of this ? :

  nsRefPtr<nsCSSStyleSheet> cssSheet(do_QueryObject(sheet));
  if (!cssSheet) {
    aCSSParseEnv.mPrincipal = nullptr;
    return;
  }

(assuming it's safe to do_QueryObject a null sheet and doing so returns a null nsRefPtr)

Comment 89

[Boris Zbarsky [:bzbarsky]]

do_QueryObject is null-safe, yes, and returns null.

Note also that a sheet may not have a document, in general.

Comment 90

[Ian Melven :imelven]

(In reply to Boris Zbarsky (:bz) from comment #89)
> do_QueryObject is null-safe, yes, and returns null.
> 
> Note also that a sheet may not have a document, in general.

Thanks for the info :)

A sheet not having a document is what was causing the crashes Sid mentions in comment 85, I have a patch now that makes it through the 'out of sheets' test, I'm just waiting to run it through the CSP tests locally and then will push it to try.

Comment 91

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=67641f5d771f

Comment 92

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5520e123f526

Comment 93

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/be239b15a0f1 for b2g mochitest-8 and mochitest-9 failures.

Comment 94

[Ian Melven :imelven]

Attachment: alternate patch v1

This is an alternate approach to blocking inline styles with CSP. It blocks everything the previous patch does, except it does not block ParsePropertyValue, SetPropertyValue and RemoveProperty - ie. it allows CSSOM manipulation via script.

The rationale for this is that IMO the inline style restrictions in CSP are not intended to block ALL application of inline styles, but more specifically, to mitigate attacks that are possible via application of inline styles when inline script is already blocked by CSP. 

I believe the reason the CSP spec says to block the style attribute as well as the <style> element is not because the style attribute can be set via Javascript, but because it can be used via any styled element that can be injected, even if script is blocked.

IMO, the CSP spec says CSSOM is allowed because by definition script is already not blocked if the CSSOM is being modified.

There are some edge cases - we block setAttribute('onclick', ...) as part of the inline script blocking (and so does WebKit FWIW), I think we should likewise
block setAttribute('style', ...) so this patch continues to do so. I am on the fence about .cssText, but it seems so close to setAttribute('style', ...) that this patch continues to block it as previously also.

The patch continues to block SMIL animations - I added a SMIL <set> test as well to make sure that was blocked. My current thinking is to propose to the WebAppSec WG to also block SMIL <set> and <animate> elements when inline styles are blocked, since any attacks via injecting these elements are also not addressed via CSP's inline script blocking. 

Based on the above reasoning, this patch conforms to the spec more closely than the previous one, and also more closely matches existing CSP implementations (ie. Webkit). Another consideration here is that we want to try to make it easier for policy authors to not have to specify unsafe-inline for doing things that are reasonably safe, like allowing scripts allowed via the CSP's script-src to modify inline styles via CSSOM. 

Also pushed to try with https://tbpl.mozilla.org/?tree=Try&rev=3f1783c6a464

Comment 95

[Ian Melven :imelven]

This had problems on B2G before so I also did a try push to all the B2G platform variants : https://tbpl.mozilla.org/?tree=Try&rev=fcf24c65e884

Comment 96

[Sid Stamm [:geekboy or :sstamm]]

Looks like the B2G tests are having focus issues.  Maybe related to bug 826147.

Comment 97

[Ian Melven :imelven]

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #96)
> Looks like the B2G tests are having focus issues.  Maybe related to bug
> 826147.

I'm not sure that's the issue - those errors are sprinkled pretty liberally throughout the logs.. I do see a lot of them though.

Comment 98

[Ian Melven :imelven]

Try push to all platforms, all tests at once to have a more consistent view of what's going on here :

https://tbpl.mozilla.org/?tree=Try&rev=75fbafc35e47

Comment 99

[Ian Melven :imelven]

(In reply to Ian Melven :imelven from comment #97)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #96)
> > Looks like the B2G tests are having focus issues.  Maybe related to bug
> > 826147.
> 
> I'm not sure that's the issue - those errors are sprinkled pretty liberally
> throughout the logs.. I do see a lot of them though.

What's strange is that some of the tests are passing and some are not :

12:45:40     INFO -  50 INFO TEST-PASS | /tests/content/base/test/test_CSP_inlinestyle.html | External Stylesheet (CSP 1.0 spec compliant) (rgb(0, 255, 0))
12:45:40     INFO -  51 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | Inline Style TAG (CSP 1.0 spec compliant) (rgb(255, 0, 0))
12:45:40     INFO -  52 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | Style Attribute (CSP 1.0 spec compliant) (rgb(255, 0, 0))
12:45:40     INFO -  53 INFO TEST-PASS | /tests/content/base/test/test_CSP_inlinestyle.html | cssText (CSP 1.0 spec compliant) (rgb(0, 0, 0))
12:45:40     INFO -  54 INFO TEST-PASS | /tests/content/base/test/test_CSP_inlinestyle.html | block the set of style.cssText
12:45:40     INFO -  55 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | XML Attribute styling (SMIL) (rgb(255, 0, 0))
12:45:40     INFO -  56 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | CSS Override styling (SMIL) (rgb(255, 0, 0))
12:45:40     INFO -  57 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | CSS Override styling via ID lookup (SMIL) (rgb(255, 0, 0))
12:45:40     INFO -  58 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CSP_inlinestyle.html | CSS Set Element styling via ID lookup (SMIL) (rgb(255, 0, 0))
12:45:40     INFO -  59 INFO TEST-PASS | /tests/content/base/test/test_CSP_inlinestyle.html | Modify loaded style sheet via cssText (rgb(0, 255, 0))

Comment 100

[Ian Melven :imelven]

Our current theory is that the failing tests are using the wrong principal (and hence the wrong CSP). Digging more today.

Thanks Blake and Sid for the help figuring this out !

Comment 101

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 729333 [details] [diff] [review]
alternate patch v1

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

I like this approach and how the threat model is clear: without scripts, we want to avoid nasty things happening.  When there are scripts, all bets are off for inline styles.
Given all the time you've spent on this, why does your patch carry my authorship?  Shouldn't you take the credit here?

I can't r+ any of the styles stuff, but the CSP things look good.

::: content/base/src/CSPUtils.jsm
@@ +182,5 @@
>    // Default to false if not specified.
>    this._specCompliant = (aSpecCompliant !== undefined) ? aSpecCompliant : false;
> +
> +  // Only CSP 1.0 spec compliant policies block inline styles.
> +  this._allowInlineStyles = !aSpecCompliant;

You might wanna clarify this comment a bit to add "by default" to the end.  It's not absolutely clear by the patch section that this is just setting up the default.

@@ +720,5 @@
>        }
>      }
>      return (this.allowsInlineScripts === that.allowsInlineScripts)
> +        && (this.allowsEvalInScripts === that.allowsEvalInScripts)
> +        && (this.allowsInlineStyles == that.allowsInlineStyles);

Please consistently use === vs ==.  I'd use three equals signs here.

::: content/base/test/file_CSP_inlinestyle_main.html
@@ +49,5 @@
> +      <animate xlink:href="#cssOverrideTestById"
> +               attributeName="fill"
> +               values="lime;green;lime"
> +               dur="2s" repeatCount="indefinite" />
> +               

Nit: trailing whitespace.  Was some in other files too, just pointing one of 'em out.

Comment 102

[Ian Melven :imelven]

Still trying to figure out what's going on with B2G and this patch, but now have the emulator running - my goal is to run the mochitest from this bug in the emulator to see how it actually behaves. I also noticed that Bug 687086 landed and bitrotted this a little bit - this patch should either address the changes that bug made as part of this patch or we should file a followup to also make inline style violations work in report-only policies.

Comment 103

[Ian Melven :imelven]

Filed bug 858836 as a followup for blocking inline styles on B2G. See also bug 858787 which is to turn on the spec compliant parser for Firefox OS. We would like to get the CSP 1.0 parser turned on for desktop Firefox ASAP. Turning it on for B2G is a more complicated affair, since it will change behavior for apps when we start blocking inline styles, which the new parser does and the old one does not (it defaults to true when asked if inline styles are allowed).

Comment 104

[Ian Melven :imelven]

Attachment: alternate patch v2

Address Sid's review comments. Also disable the inline styles CSP test in this patch for B2G, see comment 103 for more details.

Comment 105

[Ian Melven :imelven]

Attachment: alternate patch v3

Oops, forgot to put myself as patch author so as Sid suggested I can take the blame^H^H^H^H^Hcredit for this work ;)

Comment 106

[Ian Melven :imelven]

Pushed to try with bug 842657 (flip the pref to enable the CSP 1.0 parser for Firefox) : 

https://tbpl.mozilla.org/?tree=Try&rev=0222888e7fa1

Comment 107

[Ian Melven :imelven]

Attachment: alternate patch v4

I adapted the patch to the changes that bug 687086 made for reporting inline script/eval violations with a report only policy so inline style violations will also be reported too in the same manner.

Sid, could you take a quick look at the CSP reporting changes please ? 

This passes all CSP tests locally for me. I hope to get bug 858789 reviewed and landed asap which should then clear the way for this bug and bug 842657 to land together (pending review of this patch) - and then CSP 1.0 will be turned on for desktop Firefox :D

Comment 108

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 736050 [details] [diff] [review]
alternate patch v4

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

The CSP reporting stuff looks good to me.  Thanks for addressing my prior nits.

Comment 109

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

Comment on attachment 736050 [details] [diff] [review]
alternate patch v4

nsDOMCSSAttrDeclaration.cpp:

  It's preferably, both in terms of readability and performance, to keep
  the local variable |doc| so that the diff just looks like:

  +  aCSSParseEnv.mDocument = doc;

And the same in nsDOMCSSDeclaration.cpp, with the |document|
local variable.

Also in nsDOMCSSDeclaration.cpp, fix the indent here:
>-  nsresult result = cssParser.ParseDeclarations(aCssText, env.mSheetURI,
>+  result = cssParser.ParseDeclarations(aCssText, env.mSheetURI,
>                                                 env.mBaseURI,
>                                                 env.mPrincipal, decl, &changed);

Though, actually, given the model you describe in comment 94,
I don't think you should block in nsDOMCSSDeclaration::SetCSSText
at all.

That, in turn, means that you don't need any of the changes in
nsDOMCSSAttrDeclaration.cpp or nsDOMCSSDeclaration.{h,cpp}


>@@ -277,6 +285,9 @@
>     return NS_OK; // no decl, so nothing to remove
>   }
> 
>+  CSSParsingEnvironment env;
>+  GetCSSParsingEnvironment(env);
>+
>   // For nsDOMCSSAttributeDeclaration, SetCSSDeclaration will lead to
>   // Attribute setting code, which leads in turn to BeginUpdate.  We
>   // need to start the update now so that the old rule doesn't get used

Remove this stray change.


nsStyleAnimation.cpp:

  I don't see any reason to block anything here either given the model
  in comment 94.  Please remove this change.


I still think it's kind of a bad idea to have a thing that says it
blocks style but that doesn't block all style.  Though I guess
'unsafe-inline' is an obscure enough name that some other value for
style-src could be added if needed.

So r=dbaron with those comments addressed (including removal of
the blocking in nsDOMCSSDeclaration::SetCSSText and nsStyleAnimation).

In the future, please use 8 lines of context or more, and showfunc (-p),
for diffs.

Comment 110

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #109)

I'm fine with removing the check for setting cssText, that is definitely an edge case and afaik other CSP implementers don't block it.

> nsStyleAnimation.cpp:
> 
>   I don't see any reason to block anything here either given the model
>   in comment 94.  Please remove this change.

I need to do some testing to see if the attacks I'm worried about (things along the lines of bug 704482) via SMIL (<set> and <animate> elements specifically) are feasible. If they're not, I'm fine with removing this check as well. 

> I still think it's kind of a bad idea to have a thing that says it
> blocks style but that doesn't block all style.  Though I guess
> 'unsafe-inline' is an obscure enough name that some other value for
> style-src could be added if needed.

yeah, I think the spec could possible do a better job explaining the threat model and reasoning behind what inline styles are blocked and which are not. I suppose the rationale is that 'unsafe' inline style are blocked and then need to be opted back into. 

> So r=dbaron with those comments addressed (including removal of
> the blocking in nsDOMCSSDeclaration::SetCSSText and nsStyleAnimation).

Thanks for the review, David !

> In the future, please use 8 lines of context or more, and showfunc (-p),
> for diffs.

Sorry about that, I was working on a fresh Linux laptop and hadn't set up my .hgrc properly.

Comment 111

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

(In reply to Ian Melven :imelven from comment #110)
> (In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from
> comment #109)
> > nsStyleAnimation.cpp:
> > 
> >   I don't see any reason to block anything here either given the model
> >   in comment 94.  Please remove this change.
> 
> I need to do some testing to see if the attacks I'm worried about (things
> along the lines of bug 704482) via SMIL (<set> and <animate> elements
> specifically) are feasible. If they're not, I'm fine with removing this
> check as well. 

If that's the issue, I'd think the blocking belongs a bit higher in the call stack, perhaps  nsSMILCSSValueType::ValueFromString or even higher.

Comment 112

[Ian Melven :imelven]

Attachment: alternate patch v5

Addresses the review comments in comment 109, and moves the SMIL animation CSP check to nsSMILCSSValueType::ValueFromString (I looked at the call stack, this seems a decent place) as dbaron suggested.

Ran the CSP tests including the inline styles tests and they all pass locally.

Comment 113

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

Comment on attachment 750175 [details] [diff] [review]
alternate patch v5

remove the remaining one line of added comment in nsDOMCSSDeclaration.h

nsSMILCSSValueType.cpp:
>+  if (doc && !nsStyleUtil::CSPAllowsInlineStyle(doc->NodePrincipal(),
>+                                               doc->GetDocumentURI(),
>+                                               0, aString, nullptr)) {

second and third lines need one more space


r=dbaron with that

Comment 114

[Ian Melven :imelven]

Attachment: alternate patch v6

Address review comments. Will push to try when I have working DNS again :(

Comment 115

[Ian Melven :imelven]

https://tbpl.mozilla.org/?tree=Try&rev=a58be5cbbc98

Comment 116

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

The model that I have been pushing, and that I'm pretty sure we discussed with Adam Barth, is that we want to block features which allow an arbitrary set of CSS properties to be set to arbitrary values.

I.e. something like

document.body.style.background = "...";

is ok since, while it can modify several CSS properties, it's pretty clear that they are all background related.

However something like

document.styleSheets[0].cssRules[0].cssText = xhr.responseText;

is really hard to reason about what types of effects it can have on the style of a page.

So I still maintain that we should block cssText, and probably other similar properties that I don't know about.

Comment 117

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

CSSStyleSheet.insertRule seems to be another method that falls under that criteria.

Comment 118

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jonas Sicking (:sicking) from comment #116)
> The model that I have been pushing, and that I'm pretty sure we discussed
> with Adam Barth, is that we want to block features which allow an arbitrary
> set of CSS properties to be set to arbitrary values.
> 
> I.e. something like
> 
> document.body.style.background = "...";
> 
> is ok since, while it can modify several CSS properties, it's pretty clear
> that they are all background related.
> 
> However something like
> 
> document.styleSheets[0].cssRules[0].cssText = xhr.responseText;
> 
> is really hard to reason about what types of effects it can have on the
> style of a page.

Yes, but if you can execute script then you can rewrite the entire DOM so it doesn't matter much if you can do non-obvious things with the cssText sledgehammer property.

The "new" semantics (which are the same semantics that are documented in the draft spec, and the same ones implemented in WebKit, AFAICT) are basically "don't parse styles embedded in the HTML." I think that is a very clear and useful protective feature to give web developers.

Limiting how scripts can manipulate the CSS OM does not seem like a useful protection mechanism. The script can do way worse than restyle things in a surprising way. And, scripts that do need limits on CSS retrieved from external resources (e.g. the email app in B2G) need a much more powerful and flexible mechanism for filtering CSS than what you seem to be suggesting here. Consequently, I think we should keep things simple as far as these CSP directives go and then separately work on APIs for web developers to be able to filter CSS and HTML to meet their exact needs. Luckily, we have the perfect use case to drive those APIs (the aforementioned B2G email app).

> So I still maintain that we should block cssText

It would be great to get a pointer to the rationale for it.

> and probably other similar properties that I don't know about.

My point exactly. Let's not make this more complicated than it already is.

Comment 119

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

(In reply to Jonas Sicking (:sicking) from comment #116)
> The model that I have been pushing, and that I'm pretty sure we discussed
> with Adam Barth, is that we want to block features which allow an arbitrary
> set of CSS properties to be set to arbitrary values.
> 
> I.e. something like
> 
> document.body.style.background = "...";
> 
> is ok since, while it can modify several CSS properties, it's pretty clear
> that they are all background related.
> 
> However something like
> 
> document.styleSheets[0].cssRules[0].cssText = xhr.responseText;
> 
> is really hard to reason about what types of effects it can have on the
> style of a page.

I don't see what the security threat related to difficulty of reasoning about a script is.  While it's easy to reason about
  document.body.style.background = text;
, I don't think it's any easier to reason about:
  for (k in obj)
    document.body.style[k] = obj[k];
than it is to reason about:
  document.body.style.cssText = text;
.  Yet it's the first two that use the same underlying property, and the third that's different.

Comment 120

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

Sigh, we keep having this discussion over and over.

The same logic you are using could be used to argue that we should allow eval(), since if an attacker can call eval() then it can just do whatever you want anyway.

One of the purposes of CSP is to prevent a page from taking a string from an untrusted source, and then turning that string into harmful content.

A page doing eval(untrustedData) is one way that a string originating from an untrusted source could be turned into harmful script. This is why we block eval().

A page doing document.styleSheets[0].cssRules[0].cssText = untrustedData is another way that string originating from an untrusted source could be turned into harmful content.

Saying document.body.style.cssText = X is just another syntax for <body style="X">;

Likewise saying document.styleSheets[0].insertRule("X", "Y") is just another syntax for having an inline <style>X { Y }</style>.

If we are blocking one, we should block the other.

We've clearly said that CSS can be harmful, otherwise we wouldn't have CSS blocking of any form. Properties like cssText and insertRule has exactly the same capability sets as the types of inline styles that we are blocking.


Another way to think about it is that there's a lot of risk that someone will do:

document.styleSheets[0].insertRule("X", "background: " + untrustedData);

not realizing that that allows the provider of untrustedData to set arbitrary CSS properties using SQL-injection types of strings.

It would be great to have this logic documented in the spec since this same argument keeps coming up over and over.

Comment 121

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

> I don't see what the security threat related to difficulty of reasoning
> about a script is.  While it's easy to reason about
>   document.body.style.background = text;
> , I don't think it's any easier to reason about:
>   for (k in obj)
>     document.body.style[k] = obj[k];
> than it is to reason about:
>   document.body.style.cssText = text;
> .  Yet it's the first two that use the same underlying property, and the
> third that's different.

Certainly. Just like eval() is no more capable than someone building a sufficiently capable state machine written in JS. Or simply doing:

window[a][b][c](d, e, f);

There will always be ways for people to shoot themselves in the foot. CSP can't protect against all types of mistakes people can write in script.

So far CSP focuses specifically on APIs that turn strings into dangerous content, i.e. preventing SQL-injection styles of attacks.

Comment 122

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

OK, I think I get it now.  (I suppose I should have figured that out from the "Introduction" section of the CSP spec, except I don't think I read the introduction before.)  The threat model is that you're blocking script in the page (as opposed to linked) and style in the page on the principle that you don't trust the stuff that could be injected into the DOM [1], and worried that linked (and thus allowed) script that's a legitimate part of the page might take a text string from the DOM and make it part of a style sheet.

Then, indeed, there's a whole bunch of other stuff that needs to be blocked that none of the patches in this bug have attempted to block.

This also makes me think the use of "inline" in the naming is even worse than I thought it was before.  "inline" has an existing known meaning in the context of styles (styles from the style attribute and its associated OM representation); I thought CSP was expanding it to mean "inline styles and style elements", but the meaning was really entirely different.


[1] I sure hope something takes care of blocking links to script and style that are added after </head>.

Comment 123

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

Comment on attachment 750191 [details] [diff] [review]
alternate patch v6

I guess the lesson here is that I should be more aggressive about minusing reviews when I don't understand the point of the patch.

Comment 124

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Jonas Sicking (:sicking) from comment #120)
> The same logic you are using could be used to argue that we should allow
> eval(), since if an attacker can call eval() then it can just do whatever
> you want anyway.

I could be convinced that use of the CSS OM is analogous to eval(). However, keep in mind that CSP's script-src has separate 'unsafe-inline' and 'unsafe-eval' sources. If we want CSS to be consistent with JS then the CSS OM should be enabled with style-src: 'unsafe-eval'. I guess that makes sense because every value you set with the CSS OM gets parsed and "executed" by the "CSS interpreter".

However, I don't see how the simplicity or complexity of the actual CSS OM property can be used. First, it would add a lot of complexity. But, more seriously, even if we sat down right now and enumerated every CSS OM property to separate them into safe (e.g. background) and unsafe groups of properties/methods (e.g. cssText/insertRule) based on the *current* state of the CSS language, I don't think that we can expect the CSS language to keep the safe/unsafe classifications consistent over time. In particular, I don't think it is reasonable to restrict the CSS language so that no features are added that would make some new value of the background property unsafe (e.g. CSS variables, calculated values, and/or references to values from within the DOM). So, I think all of the CSS OM needs to be treated consistently; either we could be pessimistic and disable the CSS OM unless 'unsafe-eval' is used, or we could be optimistic and have the CSS OM unaffected by style-src. I do not have a strong opinion either way right now, but the important thing to me is that it must be possible, in some way, to enable the use of the CSS OM even when <style> elements and @style attributes within the document are blocked.

> One of the purposes of CSP is to prevent a page from taking a string from an
> untrusted source, and then turning that string into harmful content.

I partially agree. But, "harmful" isn't a binary measurement; there are many degrees of potential harm. Plus, there is a threat model that CSP tries to address, and certain things are out of scope for that threat model.

> Saying document.body.style.cssText = X is just another syntax for <body
> style="X">;

> Likewise saying document.styleSheets[0].insertRule("X", "Y") is just another
> syntax for having an inline <style>X { Y }</style>.

No. It is very rare for a Javascript script to have user-generated content embedded in it, but it is extremely common for HTML content to have user-generated content in it. So, document.body.style.cssText = X is more likely to be safe compared to <body style='X'> for most pages/applications.

> If we are blocking one, we should block the other. 

It would be harmful, in terms of security, to force a developer to enable the parsing of <style> and @style within the web page just to be able to dynamically style the document via trusted scripts.

> We've clearly said that CSS can be harmful, otherwise we wouldn't have CSS
> blocking of any form.

Some parts of CSS are more harmful than others. For example, certain kinds of selectors can be abused to extract secret information about of the DOM. That is the most serious harm that CSS can cause. The next most serious harm is positioning that can cause user-generated content to overlay security-sensitive parts of the document (e.g. login forms in the header of the page). There are also lesser harms that have been called "defacement" and it is debatable whether those lesser harms should be considered within the threat model for CSP.

> Properties like cssText and insertRule has exactly the
> same capability sets as the types of inline styles that we are blocking.

True, but capability is different than risk, as I tried to explain above.

Anyway, I think if we start to go through the exercise of trying to classify every part of the CSS OM as "currently safe and must be guaranteed to be safe forever," we will see that this isn't a practical course of action.

> Another way to think about it is that there's a lot of risk that someone
> will do:
> 
> document.styleSheets[0].insertRule("X", "background: " + untrustedData);
> 
> not realizing that that allows the provider of untrustedData to set
> arbitrary CSS properties using SQL-injection types of strings.

I'm not sure there's "a lot" of risk of a page doing that. It's a lot different than eval; eval has had a lot of historical uses like parsing JSON that have made eval very common.

Also, look at how and why we restrict eval() for B2G privilged apps: we restrict eval() so that we can be confident, at review time, that all of the Javascript code that is executed is contained within the app package. We don't really need that same kind of confidence for CSS.

> It would be great to have this logic documented in the spec since this same
> argument keeps coming up over and over.

The first step is to agree upon the threat model, and on the limits (if any) we are willing to impose on the CSS WG to preserve any classification of "safe" and "unsafe" CSS OM constructs in perpetuity.

Comment 125

[Ian Melven :imelven]

Sorry, I didn't see all the additional discussion in the bug and the r+ changing to r- and just pushed this to inbound. I'll back it out.

The push to inbound was :

https://hg.mozilla.org/integration/mozilla-inbound/rev/58a2011beeac

Comment 126

[Ian Melven :imelven]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8be85024c315

Comment 127

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Just to clarify, here's a strawman proposal of how CSS OM should eventually be treated:

1. By default (no style-src in policy), style-src defaults to "*" (allow all CSS features).
2. When style-src is present, all CSS features are disabled except for the sources that are given in the style-src directive.
3. URL/scheme sources are treated in the obvious way.
4. unsafe-inline unblocks style elements and style attributes.
5. unsafe-eval unblocks the entire CSS OM.
6. "*" would mean roughly "any URL/scheme AND 'unsafe-inline' AND 'unsafe-eval'."

#1-#4 are the normal behaviors specified and implemented behaviors.
#5 and #6 is a change in the spec to have 'unsafe-eval' in style-src control the CSS OM; the draft spec currently says that the CSS OM is not affected by style-src. I propose we implement #1-#6 and ship it, and then argue for the spec to be changed to incorporate 'unsafe-eval' afterward.

Comment 128

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

Comment on attachment 750191 [details] [diff] [review]
alternate patch v6

OK, after further discussion (me, sicking, bsmith, sid), we're ok going ahead with this, but we want to follow up with an additional bug on further blocking of the object model (perhaps under an 'unsafe-eval' label).

My preference (though sicking and bsmith have slight disagreement) for what needs to be blocked when unsafe-eval is not present is all CSSOM setters other that:
 * direct property setters
 * direct selectorText setters or similar (keyText)
 * direct media query setters
which basically means blocking:
 * all .cssText setters
 * anything that allows inserting a complete rule (my biggest compat worry here is keyframe and @keyframes rules)

Comment 129

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Ian Melven :imelven from comment #125)
> The push to inbound was :
> https://hg.mozilla.org/integration/mozilla-inbound/rev/58a2011beeac

hg pull m-i
hg update -c tip(tip of mozilla-inbound)
hg export -r 58a2011beeac > ian.patch (because hg graft hates puppies)
hg import ian.patch (no conflicts!)
hg push -r . m-i

https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb574cd8faf

Comment 130

[Ian Melven :imelven]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #128)
> Comment on attachment 750191 [details] [diff] [review]
> alternate patch v6
> 
> OK, after further discussion (me, sicking, bsmith, sid), we're ok going
> ahead with this, but we want to follow up with an additional bug on further
> blocking of the object model (perhaps under an 'unsafe-eval' label).
> 
> My preference (though sicking and bsmith have slight disagreement) for what
> needs to be blocked when unsafe-eval is not present is all CSSOM setters
> other that:
>  * direct property setters
>  * direct selectorText setters or similar (keyText)
>  * direct media query setters
> which basically means blocking:
>  * all .cssText setters
>  * anything that allows inserting a complete rule (my biggest compat worry
> here is keyframe and @keyframes rules)

Thank you David, Jonas, Sid and Brian !

I think this is a good path forward. Brian and I discussed having two CSP keywords along these lines previously, although that was much more based in blocking style from script vs elements/attributes. This is conceptually similar but I think it makes a *lot* of sense to frame it in the same way as unsafe-eval for scripts, since as Jonas pointed out earlier in the bug, it's the same risk : parsing arbitrary strings (the recent discussion around possibly blocking innerHTML in CSP is the same sort of thing).

In general, I agree with the positions Brian has argued above in previous comments, fwiw.

I was already planning to discuss our implementation with the WG (in particular, the SMIL animation blocking) so I'll bring up the concept of an unsafe-eval as well to see what people think. Personally, I like the idea and think it's got some nice consistency with the existing eval() blocking for script, and also Jonas' idea of blocking innerHTML via CSP somehow, although I'm still not quite sure what the directive would look like yet.

Comment 131

[Ian Melven :imelven]

(In reply to Brian Smith (:bsmith) from comment #127)

a few small (pedantic) comments : 

> 1. By default (no style-src in policy), style-src defaults to "*" (allow all
> CSS features).

style-src should fall back to default-src if one is present, but default-src cannot specify an 'unsafe-inline/eval' directive.

> 2. When style-src is present, all CSS features are disabled except for the
> sources that are given in the style-src directive.
> 3. URL/scheme sources are treated in the obvious way.
> 4. unsafe-inline unblocks style elements and style attributes.
> 5. unsafe-eval unblocks the entire CSS OM.
> 6. "*" would mean roughly "any URL/scheme AND 'unsafe-inline' AND
> 'unsafe-eval'."

for script-src you have to explicitly say 'unsafe-inline' and 'unsafe-eval' to unblock those things, * only whitelists all sources.

I like dbaron's proposal of what to block in comment 128 although I'm interested to hear sicking and bsmith's variants as well.

Comment 132

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9eb574cd8faf

Comment 133

[Daniel Veditz [:dveditz]]

This would need to be uplifted to Aurora if bug 842657 is landed there.

Comment 134

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Backport to aurora

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: CSP policies using the standard syntax and semantics will not be enforced.
Testing completed (on m-c, etc.): this has been on m-c for a week now.
Risk to taking this patch (and alternatives if risky): Not sure, because I did not write the patch, but others do not seem too concerned about it. I would check with imelven.
String or IDL/UUID changes made by this patch: nsIContentSecurityPolicy UUID changed (should not be problematic), no string changes.

Comment 135

[Ian Melven :imelven]

(In reply to Brian Smith (:bsmith) from comment #134)
> Created attachment 753881 [details] [diff] [review]
> Backport to aurora
> 
> [Approval Request Comment]
> Risk to taking this patch (and alternatives if risky): Not sure, because I
> did not write the patch, but others do not seem too concerned about it. I
> would check with imelven.

The only real risk should be for sites using CSP 1.0 headers, which are probably quite few in number, as Brian has said elsewhere. Chrome already supports these headers also. Other platforms such as B2G and Fennec have their own prefs to opt into the CSP 1.0 behaviour.

Comment 136

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 753881 [details] [diff] [review]
Backport to aurora

I'm OK with that risk since the payoff as Dan describes it sounds worthwhile. Approving for Aurora.

Comment 137

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a5adc5997af8

Comment 138

[Lukas Blakk [:lsblakk] use ?needinfo]

This has been release noted in the Beta 23 notes - currently available for preview here: https://www-dev.allizom.org/en-US/firefox/23.0beta/releasenotes/

If there are any questions or concerns about this note please get in touch.

Comment 139

[Ian Melven :imelven]

(In reply to [e-mail] [:lsblakk] from comment #138)
> This has been release noted in the Beta 23 notes - currently available for
> preview here:
> https://www-dev.allizom.org/en-US/firefox/23.0beta/releasenotes/
> 
> If there are any questions or concerns about this note please get in touch.

Thanks, looks good - you may also want to link to https://blog.mozilla.org/security/2013/06/11/content-security-policy-1-0-lands-in-firefox/ which has all the details of this change :)

Comment 140

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified in FF23, FF24+.

Comment 141

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/style-src