search

w3c / webappsec-csp

WebAppSec Content Security Policy
https://w3c.github.io/webappsec-csp/
Other
209 stars 78 forks source link

Inline style bits are very unclear #212

Open bzbarsky opened 7 years ago

bzbarsky commented 7 years ago

They don't seem to match browser behavior, for one thing: as far as I can tell, the spec requires the style attribute to be parsed, but not applied. How that's supposed to interact with .style manipulation is a mystery.

Furthermore, there's some confusion about cloning. Spec says a clone should not have its style attr applied, as far as I can tell, but at least some browsers do apply it, and there are web platform tests testing that it gets applied; see https://github.com/w3c/web-platform-tests/issues/5614

Please get this sorted out and bugs filed on browsers as needed....

// cc @mikewest

bzbarsky commented 7 years ago

@mikewest Deciding on the right behavior here is actually blocking some work we're trying to do soonish in Gecko, so it would be good to have some clarity on the spec here...

mikewest commented 7 years ago

Digging through commit logs, we originally made this change in WebKit bug 112270 to address a bug jQuery filed. Their expectation was that something like the following code would result in two green squares, even in the presence of a CSP that blocked inline style (style-src 'self' or similar).

var d = document.createElement('div');
d.style.background = "green";
d.style.height = "100px";
d.style.width = "100px";
d.style.margin = "1em";
document.body.appendChild(d);

document.body.appendChild(d.cloneNode());

Based on the tiny experiment at https://jsbin.com/tusohe, it looks like both Firefox and WebKit/Blink have this behavior today, and I think it makes sense to keep its broad strokes (as the alternative would presumably break some set of developers who rely on the existing behavior, along the lines of the jQuery ticket above).

I think you're correct, though, that the specs don't make sense together. In WebKit/Blink, we end up doing something that I think equates to adding a flag to the "append" call in step 2.2.2 of https://dom.spec.whatwg.org/#concept-node-clone, noting that the operation is the result of cloning, and then thread that through to a hook that's called on elements when their style attribute changes.

If you squint, that hook more or less maps to "the attribute's value must be parsed when the attribute is added or has its value changed" in https://html.spec.whatwg.org/multipage/dom.html#the-style-attribute.

Does that seem like a reasonable direction to explore?

as far as I can tell, the spec requires the style attribute to be parsed, but not applied.

The distinction between "parsing" and "applying" in https://html.spec.whatwg.org/multipage/dom.html#the-style-attribute seems like one without a difference, as it looks like "parsing" turns into actual style applied to the element. The intent of the CSP bits there is to prevent the content of the style attribute from being used to style its element, but it's not really clear to me where "styling" happens. I'd appreciate advice about how to phrase this in a way that makes more sense.

/cc @annevk @tabatkins who might have opinions.

annevk commented 7 years ago

If you manipulate the style IDL attribute that results in changes to the content attribute per all the standards. (This is not uniformly implemented that way unfortunately.) Content attributes are cloned and I don't think we want to add exceptions there. So that should all continue to work without CSP getting in the way.

It seems that getting information out of the style IDL attribute is also fine. The main problem is post-parsing (although, when does fetching happens? Ah right, not well defined...): layout. That should be prevented (as well as fetching, whenever that happens) by CSP.

I don't know how to best define that, since despite asking for a long time, the CSS WG is very resistant to defining clear imperative entry points that they then also use themselves. I suppose we could try again.

bzbarsky commented 7 years ago

So a few things, from my point of view:

  1. Ignoring what the specs might say right now, "parsing" and "applying" are conceptually separate operations for inline style. You can, conceptually, produce a CSSStyleDeclaration from the string (or from explicit DOM manipulation), but then not actually cascade that CSSStyleDeclaration if CSP says to not do so. In particular, given <div id="x" style="color: green"> and a CSP forbidding inline style, you could have x.style.color evaluate to "" if the attr is not parsed or to "green" if it is, and in either case you could conceivably have the div not actually be green. So the real question here is which model we want CSP to be using, which partially depends on what threat models it's trying to address.

  2. Modifying the style IDL attribute via its various accessors does two things in practice: (a) modify the CSSStyleDeclaration (which per above may or may not be used for styling) and (b) modify the content attribute. The spec around this is slightly broken, because it doesn't define exactly what "set the content attribute" means and also says that setting the content attribute is supposed to update the declaration; I'm not sure whether it's observable whether this happens if the content attribute is set by the CSSOM manipulation, but I expect it very much is in cases when base URIs are mutated and that what the spec says doesn't match UAs.

  3. The way all this is actually implemented in Gecko is that internally the value of a style attribute consists of two things, either one of which may be present or absent. One is the CSSStyleDeclaration object; the other is a string. setAttribute and the HTML parser set the string and create a CSSStyleDeclaration parsed from the string. Mutating the CSSStyleDeclaration changes it in-place and makes the string be absent. getAttribute uses the string if there is one, and serializes the CSSStyleDeclaration otherwise. So conceptually you could say that mutating the CSSStyleDeclaration immediately updates the string to its serialization but does not do so via the normal "set the attribute value" codepath; the lazy serialization is just an optimization. Finally, cloning in Gecko, for the specific case of the "style" attribute, clones the (CSSStyleDeclaration, string) pair directly instead of serializing and reparsing. This was added at some point as a performance optimization. That's why the web platform test in its current incarnation passes: we never go through the parsing step, and the parsing step is what currently makes the CSP check.

So some plausible approaches to speccing this stuff, assuming we want to preserve the "CSP doesn't apply to .style mutations" and "CSP doesn't apply to cloning" behavior:

I do think we should clarify the CSSOM end of this stuff too, as we do this.. @zcorpan

annevk commented 7 years ago

Oh great, so Gecko wouldn't do mutation observer callbacks either if you modify the style IDL attribute... That clearly violates DOM invariants :/

bzbarsky commented 7 years ago

so Gecko wouldn't do mutation observer callbacks either if you modify the style IDL attribute

Why not? We do them. That is, when the CSSStyleDeclaration is modified we make it look from the point of view of everything outside like right after the mutation we're serializing it and then setting/parsing the resulting string, without actually doing that serialization and parsing. We fire DOMAttrModified, mutation observers, etc, as normal.

The internal state of the CSSStyleDeclaration doesn't come from parsing, though, which is observable via base URIs and the CSP interactions.

mikewest commented 7 years ago

I'll spend more time on this in the morning, but some quick thoughts now:

So the real question here is which model we want CSP to be using, which partially depends on what threat models it's trying to address.

The underlying goal of 'unsafe-inline' processing for style-src is to allow developers to mitigate the risk of direct style injection into an attribute or <style> block. I don't think the same justification applies to the style IDL attribute, so we allow CSSOM manipulations. That bit seems reasonably easy to justify.

By the same token, we don't have a particular need to prevent developers from cloning nodes with styling that they've poked at via CSSOM (though we do want to ensure that cloning an element with an inline style attribute doesn't inadvertently apply the style, which makes things complicated).

Of the options you present, the first two seem most straightforward to define with the specs that we have. I'm not convinced that threading some creation state through attribute setting would be "much" more complicated, though. That might be worth exploring (if only because it's what Blink is doing at the moment, and it looks like we're using that state in a few other places for reasons that I'm not sure I can justify by pointing to any spec (e.g. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLFormControlElement.cpp?rcl=ae0e0ee4a1b482fc86a7edcd371179c9cfd75d87&l=146, https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp?rcl=36b8552bced16b02fb080f98738087678accf273&l=184, and https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/HTMLElement.cpp?rcl=36b8552bced16b02fb080f98738087678accf273&l=430).

The fact that this is going to turn out to be complicated to spec means that we might just be doing the wrong thing, though. Maybe I should add some metrics to Chrome to measure how often we actually clone nodes with inline styles in documents whose policy would prevent the style from applying if we just accepted the currently-specced behavior for node cloning...

annevk commented 7 years ago

The internal state of the CSSStyleDeclaration doesn't come from parsing, though, which is observable via base URIs and the CSP interactions.

I guess that could be defined. Setting the style content attribute ends up observing an internal flag. When the flag is set, the CSS is not (re)parsed. When you manipulate through CSSStyleDeclaration it sets the flag, sets the attribute, unsets the flag. That should preserve everything without violating DOM invariants.

CSP would affect whether the it takes part in the cascade and whether fetches happen.

It seems cloning in that case would be fine, as CSP would continue to control the dangerous bits.

bzbarsky commented 7 years ago

So here is a question: if an element with an inline style attribute in a document that has a CSP that allows inline style is imported or adopted into a document that has a CSP that does NOT allow them, what should happen?

bzbarsky commented 7 years ago

I filed https://github.com/w3c/csswg-drafts/issues/1559 on the CSSOM issue from https://github.com/w3c/webappsec-csp/issues/212#issuecomment-303768549 item 2.

annevk commented 7 years ago

The adoption (import is clone) case should probably fail as well.

annevk commented 6 years ago

It seems that part of this is tested by http://w3c-test.org/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html (https://github.com/w3c/web-platform-tests/blob/master/content-security-policy/style-src/inline-style-allowed-while-cloning-objects.sub.html) in a rather weird way (due to conversion from WebKit tests) and the expectations there don't always make sense to me.

I'm bringing this up as the last legacy DOM bug at https://www.w3.org/Bugs/Public/show_bug.cgi?id=25529 has mentions of Chrome special casing cloneNode() for CSP purposes, while this is not defined in the DOM Standard. (I plan on migrating that bug to GitHub.)

It'd be great if Chrome could make progress on getting this documented.

andypaicu commented 6 years ago

I will be honest this entire discussion is going over my head even after reading it twice. Any chance that someone that understands it better could write some WPT tests or something for the desired behavior?

bzbarsky commented 6 years ago

Well, that's the problem. It's not clear what the desired behavior is. What's in the spec right now is known to not be it, though.

A good start, again, would be Chrome documenting what it implemented, since it clearly didn't implement what the spec the Chrome engineers wrote says.... Then we can at least have a discussion about whether that could be the desired behavior.

bzbarsky commented 6 years ago

And to be clear, if someone wants it I can describe what Gecko has implemented here. It's not something that was done with CSP in mind at all, and probably has all sorts of weird warts, but I can describe it. Ideally every browser would do that, so we can have some idea of what we're working with.

andypaicu commented 5 years ago

Alright I was unable to find someone that knows better so I will try to take over explaining the current Chrome behavior based on my own investigation.

Currently when a style attribute is changed, a change reason is passed though (out of kDirectly, kByParser, kByCloning). If the change reason is kByCloning the CSP check is skipped entirely.

There a bunch of situations in which a kByCloning reason is used:

Also no CSP checks are done whenever setting a style property: node.style.color = "green"; will always pass

but setting the value of the style attribute does trigger CSP checks: node.style = "color: green;"; goes through the CSP inline checks

The check is done at the moment the attribute is changed not when the style is applied. The element does not have to be part of the DOM. No extra check is done as far as I can tell if you import the node into a different document.

bzbarsky commented 5 years ago

Thank you for writing that up. I just realized that https://github.com/w3c/webappsec-csp/issues/212#issuecomment-303768549 item 3 already describes the Gecko behavior. Please let me know if that's still not making sense.

andypaicu commented 5 years ago

Alright so this what I want to go with (pretty much inspired by what @bzbarsky suggested):

CSP is checked at parsing and blocks parsing the style attribute. Any direct operations go through. We define cloning the style attribute: 1) If the cloned element has an inline style attribute, check CSP. If it blocks skip step 2) 2) Copy the style cssstyledeclaration (https://drafts.csswg.org/cssom/#the-cssstyledeclaration-interface) from the element. I don't think CSSOM has a defined "copy method" but I think we can just use the cssText or something like that.

The idea is that if there is an inline style attribute it needs to pass CSP checks against the document's CSP. If it does we apply the actual cssstyledeclaration (which might be different but we don't care about the differences caused by CSSOM operations). Unfortunately, the problem is that I can't see a way to actually determine if an element has an inline style attribute because CSSOM also sets the style attribute value on every property change: https://drafts.csswg.org/cssom/#update-style-attribute-for.

https://html.spec.whatwg.org/#the-style-attribute needs to be updated to not check CSP when the source of the attribute change is coming from CSSOM methods.

importNode should just work in its current form because it relies on cloneNode with a different document as the context.

adoptNode was not discussed, it seems like a weird method to me... all it does is changed the owner document (which I can't tell if it actually does anything to actually move the node into a different document).

Overall I think this works but I don't know how to tell if the element has an inline style attribute set and what that attribute value is. Worst case I imagine we can save a copy of it on the element.

Thoughts on this approach?

bzbarsky commented 5 years ago

If the cloned element has an inline style attribute, check CSP

Which CSP? Especially in the importNode case, where there are two obvious ones.

all it does is changed the owner document

It dates back to the days when multiple DOM implementations were floating around in the same browser. But it does do various other work in some browsers, for what it's worth (e.g. prototype reparenting in Gecko).

but I don't know how to tell if the element has an inline style attribute set

Well, you're asking a slightly nonsensical question is part of the problem. Setting inline style sets the style attribute. That's just how it works. Once you set inline style, the style attribute is set.

It sounds like you are going for some sort of concept of "had the style attribute set directly via a string in the original source or page-provided setAttribute call", but I don't see why that concept is relevant, given that for valid CSS values you can achieve via inline style any value you could achieve via setAttribute. Is this really about the "original source" case? What is the real purpose of this check?

andypaicu commented 5 years ago

If the cloned element has an inline style attribute, check CSP

Which CSP? Especially in the importNode case, where there are two obvious ones.

Well the CSP of the document used throughout https://dom.spec.whatwg.org/#concept-node-clone, which for importNode is the new document.

all it does is changed the owner document

It dates back to the days when multiple DOM implementations were floating around in the same browser. But it does do various other work in some browsers, for what it's worth (e.g. prototype reparenting in Gecko).

but I don't know how to tell if the element has an inline style attribute set

Well, you're asking a slightly nonsensical question is part of the problem. Setting inline style sets the style attribute. That's just how it works. Once you set inline style, the style attribute is set.

It sounds like you are going for some sort of concept of "had the style attribute set directly via a string in the original source or page-provided setAttribute call", but I don't see why that concept is relevant, given that for valid CSS values you can achieve via inline style any value you could achieve via setAttribute. Is this really about the "original source" case? What is the real purpose of this check?

Quoting what Mike said earlier: By the same token, we don't have a particular need to prevent developers from cloning nodes with styling that they've poked at via CSSOM (though we do want to ensure that cloning an element with an inline style attribute doesn't inadvertently apply the style, which makes things complicated).

Basically what I want is to differentiate between cloning a node in these two scenarios:

1) <p style="color:green;">asd</p> <--- here style-src is checked before applying the style. When cloning this node I want to perform CSP inline checks on the style attribute

2) <p id='1'>asd</p><script>document.getElementById('1').style.color = 'green';</script> <--- here I don't want to perform CSP inline checks when I'm cloning the p element.

Another problem is that the inline check is no longer a binary allowed/denied check on the entire page. Since the introduction of unsafe-hashes developers can whitelist specific styles attribute values. Which means that in this scenario I need the actual original style attribute value:

Headers Content-Security-Policy: style-src 'unsafe-hashes' 'sha-askddkjahkjd' <--- sha hash of color: green

<p style='color:green' id='1'/> <p style='color:red' id='2'/>

Now if I clone both of these I should prevent the application of the second style BUT in a similar scenario where the styles are coming from CSSOM operations it should not be blocked.

The more I think about it the more I think what we should do is to prevent the style attribute value being set by the parser if it does not pass CSP checks and then there's no need to check any CSP anywhere else. If anything was blocked by CSP it's just not part of the style attribute and the node can be cloned/imported as is. This would go against he idea that if we import a node from a document that uses unsafe-inline into one that does not the style attribute should be blocked in the new document from applying the inline style but I'm happy to challenge this idea.

I did some testing for this and I found an interop discrepancy between Chrome and Firefox.

<head> <meta http-equiv="Content-Security-Policy" content="style-src 'none'"> </head> <body> <p style="background: green;"> AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA </p> <script> var p = document.getElementsByTagName('p')[0]; p.style.color='red'; console.log(p.style); console.log(p.getAttribute('style')); </script> </body>

Here both return color: red;, BUT if you comment the p.style.color='red'; line, Firefox will output an empty string and Chrome will output background: green. I think Chrome is correct per spec but maybe it should not be and instead we should prevent the setting of the style attribute if the inline style is blocked by CSP.

bzbarsky commented 5 years ago

BUT if you comment the p.style.color='red'; line, Firefox will output an empty string

That seems like a Firefox bug to me. hasAttribute("style") returns true in Firefox in this case; it's just the value that gets lost. The people who added the CSP check didn't understand the DOM code very well, and dropped the attribute value by accident...

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1513623 to track this.

and instead we should prevent the setting of the style attribute if the inline style is blocked by CSP

This seems like a pretty weird behavior to me. It does have the virtue of being very simple to spec and implement, though....

bzbarsky commented 5 years ago

Well the CSP of the document used throughout https://dom.spec.whatwg.org/#concept-node-clone

OK. So the idea is to enforce CSP from the new document on the node, even if the original document didn't have CSP blocking inline style.

Does this match current UAs?

By the same token, we don't have a particular need to prevent developers from cloning nodes with styling that they've poked at via CSSOM (though we do want to ensure that cloning an element with an inline style attribute doesn't inadvertently apply the style, which makes things complicated).

For the cloning case, where there is just the one CSP involved, I agree that we want to avoid applying the style during clone if the CSP prevents it.

The import case is less clear to me, especially around threat models and use cases.

Basically what I want is to differentiate between cloning a node in these two scenarios:

Why? What are the actual threat models this is trying to address?

BUT in a similar scenario where the styles are coming from CSSOM operations it should not be blocked

For same-document cloning this happens automatically if cloning just clones the CSSStyleDeclaration, if any.

For cross-document import, where the CSPs may be different, this comes back to threat models. If the original document whitelists "color: green", does not modify it via CSSOM, then imports into a document that only whitelists "color: red", should the green stay? What if the page started with the (allowed) "color: green", then via CSSOM set color to "red" then "green"? What if it started with "color: green" and then via CSSOM set color to "green"?

My temptation here would be to go with spec and implementation simplicity unless there are explicit security issues being addressed by adding complexity.

This would go against he idea that if we import a node from a document that uses unsafe-inline into one that does not the style attribute should be blocked

Is it blocked in current browsers? What are the threat models?