CSP blocks inline style when cloning a node

[Mike West]

Upstreamed from https://code.google.com/p/chromium/issues/detail?id=189184

---

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_2) AppleWebKit/537.22 (KHTML, like Gecko) Chrome/25.0.1364.160 Safari/537.22

Steps to reproduce the problem:
With the header:

   X-WebKit-CSP: default-src 'self'

Run the following:

   div1 = document.createElement('div');
   div1.style.background = 'red';
   var div2 = div1.cloneNode(true);

What is the expected behavior?
No error... assuming that JavaScript "div1.style.xxx" is not considered "inline".

What went wrong?
Returns the error:

Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

Did this work before? N/A 

Chrome version: 25.0.1364.160  Channel: stable
OS Version: OS X 10.8.2

This effects jQuery 1.9.1, and probably other versions:

http://bugs.jquery.com/ticket/13507

[Felipe Zimmerle]

Created attachment 193045 [details]
test case proposal

[Felipe Zimmerle]

Created attachment 193047 [details]
test case proposal (expected results)

[Felipe Zimmerle]

I was taking a look at the problem and I ended up doing the layout test. May be useful for someone else that is interested in this issue too.

[Mike West]

(In reply to comment #3)
> I was taking a look at the problem and I ended up doing the layout test. May be useful for someone else that is interested in this issue too.

I'd encourage you to continue! Clone node ends up writing all the attributes from the initial element to the new element in Element::cloneAttributesFromElement (http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.cpp#L2848). A little ways down the chain, this ends up triggering StyledElement::styleAttributeChanged where the CSP check is performed.

One option for a solution might be to pipe a bit of state down by adding an enum, perhaps `ChangedDirectly`/`ChangedViaClone`, and bypassing the check there in the latter case.

[Felipe Zimmerle]

Thanks for the encouraging note :)

Here goes my thoughts:

I believe that the full walk through is:

(Element::)cloneNode()
 `- (Element::)cloneAttributesFromElement()
      `- (StyledElement::)attributeChanged()
           `- (StyledElement::)styleAttributeChanged()
                `- CSP checks goes here.


I am thinking that may not be a good idea to change 
(Element::)attributeChanged signature due the fact that it
is virtual, implemented by everyone which extends Element.
Maybe more modification that we really want. Instead I am 
thinking in create a m_styleAttributesAreBeenCloned (or so)
state which could be set to true when cloneAttributesFromElement
function is called, and before it returns we can set it to
false. While true the CSP check can be ignored (at
StyledElement::styleAttributeChanged). I need to double
check if there is any race condition but apparently the
cloneNode just returns after this process been done.

I will work on a patch as soon as I arrive home.

[Mike Sherov]

Mike West, can you make this bug block jQuery's meta ticket so I can track progress? Thanks!

[Felipe Zimmerle]

Created attachment 193479 [details]
Patch

[Adam Barth]

Comment on attachment 193479 [details]
Patch

This doesn't seem like the right approach.

[Adam Barth]

Comment on attachment 193479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193479&action=review

> Source/WebCore/dom/Element.cpp:2934
>      unsigned bitfield;
> +    unsigned secondBitfield;

Overflowing to a second bit field seems bad too.

[Mike West]

Comment on attachment 193479 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193479&action=review

Thanks for the patch. I think Adam's correct to deny the review, however. Comments inline.

> Source/WebCore/ChangeLog:6
> +        Reviewed by NOBODY (OOPS!).

Please add a bit more detail about what changed and why.

> Source/WebCore/dom/Element.h:99
> +    mutable unsigned m_attributeBeenCloned : 1;

Adding another attribute to Element is expensive; it would be best to keep the object as small as possible, given the number that will be created during the process of parsing a document.

I would suggest reconsidering the signature of attributeChanged, perhaps something like 'attributeChanged(const QualifiedName&, const AtomicString&, ReasonForAttributeChange reason = AttributeSetDirectly);', and passing in AttributeSetFromClone (or something similar) in the one callsite that matters. You're correct to say that it will adjust each of Element, StyledElement, and SVGElement. That is, however, much more acceptable than adding attributes to Element.

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:6
> +if (window.testRunner) {

Nit: No braces are necessary for single-line blocks.

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:19
> +   var div2 = div1.cloneNode(true);

I'd suggest doing a bit more verification in JS. You should be able to assert that the attribute exists on both elements, that it has the same value, and that that value is applied to the element's computedStyle. Take a look at LayoutTests/fast/frames/seamless/seamless-body-margin.html for an example of how such a test might look.

[Felipe Zimmerle]

Hey guys, thank you for the review. I am working on the signature approach and improving the test.

[Felipe Zimmerle]

Created attachment 193825 [details]
Patch

[Mike West]

Comment on attachment 193825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review

Thanks for updating the patch! A few comments and questions inline.

> Source/WebCore/ChangeLog:14
> +        (WebCore::Element::attributeChanged):

Please add a bit of detail here about what changed in the signature, and why. It doesn't have to be paragraphs, just a quick note that you added an enum and what it means.

> Source/WebCore/dom/Element.h:376
> +    enum ReasonForAttributeChange { AttributeSetDirectly = 0, AttributeSetFromClone };

Style nit: please break this enum out into separate lines, similar to http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/ScriptControllerBase.h#L35.

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1
> +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.

Hrm. Where is this violation coming from?

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4
> +    <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; default-src 'self';">

You can simplify the console warnings by setting "style-src 'self'" rather than 'default-src'.

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:80
> +    <div id="ops" style="background: rgb(238, 130, 238)">

Thank you for adding this check; I hadn't thought about this case in particular, so I'm glad you did.

It seems a bit fragile, though, relying as it does on the fact that we never call setInlineStyleFromString. That said, I can't think of cases off the top of my head in which that would go wrong...

[Build Bot]

Comment on attachment 193825 [details]
Patch

Attachment 193825 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17184407

[Build Bot]

Comment on attachment 193825 [details]
Patch

Attachment 193825 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17253017

[Adam Barth]

Comment on attachment 193825 [details]
Patch

Thanks for updating your patch.  This new version is much better.  Generally speaking, we prefer explicit data flows (like in your updated patch) to implicit data flows (like in your previous patch).  I probably should have said that explicitly before.  ;)

[Adam Barth]

Comment on attachment 193825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review

> Source/WebCore/dom/Element.h:379
> -    virtual void attributeChanged(const QualifiedName&, const AtomicString&);
> +    virtual void attributeChanged(const QualifiedName&, const AtomicString&, ReasonForAttributeChange = AttributeSetDirectly);

I'm surprised there aren't more overrides of this function that need to be changed.  Did you grep the codebase completely?  Ideally these sorts of errors would show up in tests, but our test coverage isn't perfect.

[Adam Barth]

Comment on attachment 193825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review

> Source/WebCore/dom/Element.cpp:847
> -void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue)
> +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason)

Your patch is triggering a build failure on the apple-mac port:

/Volumes/Data/EWS/WebKit/Source/WebCore/dom/Element.cpp:847:114: error: unused parameter 'reason' [-Werror,-Wunused-parameter]
void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason)

The solution to this problem is to remove the parameter name.  That tells -Wunused-parameter that you know the parameter is unused.

[Build Bot]

Comment on attachment 193825 [details]
Patch

Attachment 193825 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17203307

[Felipe Zimmerle]

Created attachment 193916 [details]
Patch

[Felipe Zimmerle]

(In reply to comment #13)
> Thanks for updating the patch! A few comments and questions inline.

Thank you for the review :)

> 
> > Source/WebCore/ChangeLog:14
> > +        (WebCore::Element::attributeChanged):
> 
> Please add a bit of detail here about what changed in the signature, and why. It doesn't have to be paragraphs, just a quick note that you added an enum and what it means.

Done.

> > Source/WebCore/dom/Element.h:376
> > +    enum ReasonForAttributeChange { AttributeSetDirectly = 0, AttributeSetFromClone };
> 
> Style nit: please break this enum out into separate lines, similar to http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/ScriptControllerBase.h#L35.

Done.

> > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1
> > +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "default-src 'self'". Note that 'style-src' was not explicitly set, so 'default-src' is used as a fallback.
> 
> Hrm. Where is this violation coming from?
>

The first comes from: "../../resources/js-test-pre.js" script, the second is 
expected, from: '<div id="ops" style="background: rgb(238, 130, 238)'.

> > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4
> > +    <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; default-src 'self';">
> 
> You can simplify the console warnings by setting "style-src 'self'" rather than 'default-src'.

Done.


Thanks ;)

[Felipe Zimmerle]

(In reply to comment #17)
> I'm surprised there aren't more overrides of this function that need to be changed.  Did you grep the codebase completely?  Ideally these sorts of errors would show up in tests, but our test coverage isn't perfect.

Me too, at first sight I was expecting a lot of overrides, that is why I suggested to other approach. After you guys comments, I figured out that they are not so many.

(zimmerle@omar)-(~/core/webkit)$ grep "attributeChanged(const QualifiedName&, const AtomicString&)" -Ri Source/* | wc -l
0

[Felipe Zimmerle]

(In reply to comment #18)
> (From update of attachment 193825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193825&action=review
> 
> > Source/WebCore/dom/Element.cpp:847
> > -void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue)
> > +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason)
> 
> Your patch is triggering a build failure on the apple-mac port:
> 
> /Volumes/Data/EWS/WebKit/Source/WebCore/dom/Element.cpp:847:114: error: unused parameter 'reason' [-Werror,-Wunused-parameter]
> void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, ReasonForAttributeChange reason)
> 
> The solution to this problem is to remove the parameter name.  That tells -Wunused-parameter that you know the parameter is unused.

Done.

Thanks :)

[Mike West]

Comment on attachment 193916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review

Thanks for taking another pass. I'll unofficially r+,cq- this for the typo noted below. Hopefully Adam can take an official pass in the morning.

> Source/WebCore/ChangeLog:9
> +        style is allowed otherwise it relays on default permission mechanism.

Nit: There's a typo in 'relies'.

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:1
> +CONSOLE MESSAGE: Refused to apply inline style because it violates the following Content Security Policy directive: "style-src 'self'".

This still bothers me, but it's not this patch's fault. :)

I assume it's coming from 'js-test-pre.js' insertStyleSheet call. If so, it should have a line number. I'll poke at that in a separate bug.

[Mike West]

Comment on attachment 193916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4
> +    <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self';">

One more thing: I think this will be even more clear if you just drop the 'script-src' directive entirely. It doesn't add anything to the test; you're only testing the behavior of 'style-src'.

[Kenneth Rohde Christiansen]

Comment on attachment 193916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review

> Source/WebCore/dom/Element.h:379
> +    enum ReasonForAttributeChange {
> +        AttributeSetDirectly,
> +        AttributeSetFromClone
> +    };

That doesnt sounds like a "reason" to me. More like who set it. Did you find similarly names things in WebCore, if not, take a look around

ByClone?

> LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt:38
> +ops.
> +ops.
> +ops.

? :)

[Felipe Zimmerle]

Created attachment 194139 [details]
Patch

[Felipe Zimmerle]

(In reply to comment #24)
> 
> Nit: There's a typo in 'relies'.
> 

Fixed.

[Felipe Zimmerle]

(In reply to comment #25)

> > LayoutTests/http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html:4
> > +    <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self';">
> 
> One more thing: I think this will be even more clear if you just drop the 'script-src' directive entirely. It doesn't add anything to the test; you're only testing the behavior of 'style-src'.

Done.

[Felipe Zimmerle]

(In reply to comment #26)
> (From update of attachment 193916 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193916&action=review
> 
> > Source/WebCore/dom/Element.h:379
> > +    enum ReasonForAttributeChange {
> > +        AttributeSetDirectly,
> > +        AttributeSetFromClone
> > +    };
> 
> That doesnt sounds like a "reason" to me. More like who set it. Did you find similarly names things in WebCore, if not, take a look around
> 

After talk to Kenneth, We agreed to change it to:

enum AttributeModifyReason {
    ModifiedDirectly,
    ModifiedByCloning
};

[Mike West]

Kenneth, I'd appreciate it if you could take some time to officially review this since Adam is out?

Thanks!

[Adam Barth]

Comment on attachment 194139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=194139&action=review

> LayoutTests/ChangeLog:9
> +        * http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects-expected.txt: Added.
> +        * http/tests/security/contentSecurityPolicy/inline-style-allowed-while-cloning-objects.html: Added.

These files don't appear to be in the patch.  Assuming they do what they say, this patch LGTM.

[Felipe Zimmerle]

Created attachment 196324 [details]
Patch

[Mike West]

Comment on attachment 196324 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196324&action=review

> Source/WebCore/dom/Element.cpp:850
> +void Element::attributeChanged(const QualifiedName& name, const AtomicString& newValue, AttributeModifyReason)

My apologies, I missed this in the last version of the patch: "AttributeModifyReason" sounds very strange. Please change it to "AttributeModificationReason" so that it's a reasonable noun, and I'll CQ the patch for you.

Thanks!

[Felipe Zimmerle]

Created attachment 196334 [details]
Patch

[Mike West]

Comment on attachment 196334 [details]
Patch

Thanks! Bots are happy, CQing.

[WebKit Review Bot]

Comment on attachment 196334 [details]
Patch

Clearing flags on attachment: 196334

Committed r147558: <http://trac.webkit.org/changeset/147558>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.