Bug 112270

Summary: CSP blocks inline style when cloning a node
Product: WebKit Reporter: Mike West <mkwst>
Component: WebCore Misc.Assignee: Felipe Zimmerle <felipe>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, buildbot, cmarcelo, d-r, esprehn+autocc, felipe, fmalita, mike.sherov, mkwst+watchlist, ojan.autocc, pdr, rniwa, schenney, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 110007, 85558    
Attachments:
Description Flags
test case proposal
none
test case proposal (expected results)
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Mike West 2013-03-13 10:46:42 PDT
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
Comment 1 Felipe Zimmerle 2013-03-13 20:02:54 PDT
Created attachment 193045 [details]
test case proposal
Comment 2 Felipe Zimmerle 2013-03-13 20:03:54 PDT
Created attachment 193047 [details]
test case proposal (expected results)
Comment 3 Felipe Zimmerle 2013-03-13 20:27:30 PDT
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.
Comment 4 Mike West 2013-03-14 02:29:17 PDT
(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.
Comment 5 Felipe Zimmerle 2013-03-15 12:23:53 PDT
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.
Comment 6 Mike Sherov 2013-03-15 20:01:48 PDT
Mike West, can you make this bug block jQuery's meta ticket so I can track progress? Thanks!
Comment 7 Felipe Zimmerle 2013-03-17 20:03:17 PDT
Created attachment 193479 [details]
Patch
Comment 8 Adam Barth 2013-03-17 20:09:01 PDT
Comment on attachment 193479 [details]
Patch

This doesn't seem like the right approach.
Comment 9 Adam Barth 2013-03-17 20:09:42 PDT
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.
Comment 10 Mike West 2013-03-18 01:27:03 PDT
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.
Comment 11 Felipe Zimmerle 2013-03-18 02:36:50 PDT
Hey guys, thank you for the review. I am working on the signature approach and improving the test.
Comment 12 Felipe Zimmerle 2013-03-19 07:31:27 PDT
Created attachment 193825 [details]
Patch
Comment 13 Mike West 2013-03-19 07:49:37 PDT
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...
Comment 14 Build Bot 2013-03-19 08:00:25 PDT
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
Comment 15 Build Bot 2013-03-19 09:35:24 PDT
Comment on attachment 193825 [details]
Patch

Attachment 193825 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17253017
Comment 16 Adam Barth 2013-03-19 10:19:40 PDT
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.  ;)
Comment 17 Adam Barth 2013-03-19 10:22:24 PDT
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.
Comment 18 Adam Barth 2013-03-19 10:24:06 PDT
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.
Comment 19 Build Bot 2013-03-19 10:24:28 PDT
Comment on attachment 193825 [details]
Patch

Attachment 193825 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17203307
Comment 20 Felipe Zimmerle 2013-03-19 14:07:28 PDT
Created attachment 193916 [details]
Patch
Comment 21 Felipe Zimmerle 2013-03-19 14:12:34 PDT
(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 ;)
Comment 22 Felipe Zimmerle 2013-03-19 14:19:00 PDT
(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
Comment 23 Felipe Zimmerle 2013-03-19 14:20:40 PDT
(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 :)
Comment 24 Mike West 2013-03-20 05:10:33 PDT
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.
Comment 25 Mike West 2013-03-20 05:13:39 PDT
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'.
Comment 26 Kenneth Rohde Christiansen 2013-03-20 05:57:28 PDT
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.

? :)
Comment 27 Felipe Zimmerle 2013-03-20 15:44:18 PDT
Created attachment 194139 [details]
Patch
Comment 28 Felipe Zimmerle 2013-03-20 15:47:31 PDT
(In reply to comment #24)
> 
> Nit: There's a typo in 'relies'.
> 

Fixed.
Comment 29 Felipe Zimmerle 2013-03-20 15:48:16 PDT
(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.
Comment 30 Felipe Zimmerle 2013-03-20 15:51:53 PDT
(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
};
Comment 31 Mike West 2013-03-26 08:03:57 PDT
Kenneth, I'd appreciate it if you could take some time to officially review this since Adam is out?

Thanks!
Comment 32 Adam Barth 2013-04-02 10:43:46 PDT
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.
Comment 33 Felipe Zimmerle 2013-04-03 04:20:41 PDT
Created attachment 196324 [details]
Patch
Comment 34 Mike West 2013-04-03 04:25:51 PDT
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!
Comment 35 Felipe Zimmerle 2013-04-03 05:35:14 PDT
Created attachment 196334 [details]
Patch
Comment 36 Mike West 2013-04-03 08:14:02 PDT
Comment on attachment 196334 [details]
Patch

Thanks! Bots are happy, CQing.
Comment 37 WebKit Review Bot 2013-04-03 08:43:35 PDT
Comment on attachment 196334 [details]
Patch

Clearing flags on attachment: 196334

Committed r147558: <http://trac.webkit.org/changeset/147558>
Comment 38 WebKit Review Bot 2013-04-03 08:43:41 PDT
All reviewed patches have been landed.  Closing bug.