Bug 104479 – CSP 1.1: Experiment with 'reflected-xss' directive.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

http://lists.w3.org/Archives/Public/public-webappsec/2012Nov/0061.html proposed folding 'X-XSS-Protection' into the CSP 1.1 spec. I'm looking into how hooking that up to XSSAuditor might look in WebKit.

Spec: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#reflected-xss--experimental

[Mike West]

Created attachment 178408 [details]
Patch

[Build Bot]

Comment on attachment 178408 [details]
Patch

Attachment 178408 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15230235

[Mike West]

Created attachment 178424 [details]
WIP

[Mike West]

Hey Tom, could you help me put together some tests for this? I'm sure that one of the XSSAuditor tests is sufficiently minimal to serve as an on/off check, but all of them that I've spot-checked seem pretty complex. Could you point me in the right direction?

[Thomas Sepez]

I'd take a look at LayoutTests/http/tests/security/xssAuditor/resource/echo-intertag.pl, which is an XSS-vulnerable script.  I'd think you might have to add a new query parameter in this script to generate a CSP header with the reflected-xss directive when present. You'd then invoke this along the lines of LayoutTests/http/tests/security/xssAuditor/script-tag.html.

[Mike West]

Created attachment 183936 [details]
Patch

[Mike West]

Created attachment 183937 [details]
Patch

[Mike West]

(In reply to comment #5)
> I'd take a look at LayoutTests/http/tests/security/xssAuditor/resource/echo-intertag.pl, which is an XSS-vulnerable script.  I'd think you might have to add a new query parameter in this script to generate a CSP header with the reflected-xss directive when present. You'd then invoke this along the lines of LayoutTests/http/tests/security/xssAuditor/script-tag.html.

Thanks, Tom. This was really helpful.

Would you (and Adam (hi Adam!)) mind taking a look at the implementation? I want to put some more tests together to make sure the combination of X-XSS-Protection and the new CSP directive makes sense, but this is the direction that I think makes sense.

Once we agree on a method that makes sense, I'll need to update the spec to make sure the behavior is comprehensible.

Thanks!

[Build Bot]

Comment on attachment 183937 [details]
Patch

Attachment 183937 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16036503

[Build Bot]

Comment on attachment 183937 [details]
Patch

Attachment 183937 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16044469

[Mike West]

Created attachment 187553 [details]
Patch

[Mike West]

Rebased this on top of the parsing/threading work that's ongoing.

Tom, Adam: would you mind taking a look when you have a few minutes?

[Adam Barth]

Comment on attachment 187553 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditor.cpp:193
> +static XSSProtectionDisposition combineXSSProtectionHeaderAndCSP(XSSProtectionDisposition xssProtection, ContentSecurityPolicy::ReflectedXSSDisposition reflectedXSS)
> +{
> +    if (xssProtection == XSSProtectionBlockEnabled || reflectedXSS == ContentSecurityPolicy::BlockReflectedXSS)
> +        return XSSProtectionBlockEnabled;
> +
> +    if (xssProtection == XSSProtectionEnabled || reflectedXSS == ContentSecurityPolicy::FilterReflectedXSS)
> +        return XSSProtectionEnabled;
> +
> +    if (xssProtection == XSSProtectionInvalid || reflectedXSS == ContentSecurityPolicy::ReflectedXSSInvalid)
> +        return XSSProtectionEnabled;
> +
> +    if (xssProtection == XSSProtectionDisabled || reflectedXSS == ContentSecurityPolicy::AllowReflectedXSS)
> +        return XSSProtectionDisabled;
> +
> +    // If neither header is set, enable XSSAuditor.
> +    return XSSProtectionEnabled;
> +}

Can we change XSSProtectionDisposition to use ContentSecurityPolicy::ReflectedXSSDisposition and the make this function into something like std::max ?

[Thomas Sepez]

> Tom, Adam: would you mind taking a look when you have a few minutes?
Mike, this seems reasonable to me.  My only question would be whether you have a test that checks for the reflected-xss clause mixed in with various other CSP clauses to make sure that it doesn't break the existing ones, or vice versa.

[Mike West]

(In reply to comment #13)
> (From update of attachment 187553 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=187553&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:193
> > +static XSSProtectionDisposition combineXSSProtectionHeaderAndCSP(XSSProtectionDisposition xssProtection, ContentSecurityPolicy::ReflectedXSSDisposition reflectedXSS)
> > +{
> > +    if (xssProtection == XSSProtectionBlockEnabled || reflectedXSS == ContentSecurityPolicy::BlockReflectedXSS)
> > +        return XSSProtectionBlockEnabled;
> > +
> > +    if (xssProtection == XSSProtectionEnabled || reflectedXSS == ContentSecurityPolicy::FilterReflectedXSS)
> > +        return XSSProtectionEnabled;
> > +
> > +    if (xssProtection == XSSProtectionInvalid || reflectedXSS == ContentSecurityPolicy::ReflectedXSSInvalid)
> > +        return XSSProtectionEnabled;
> > +
> > +    if (xssProtection == XSSProtectionDisabled || reflectedXSS == ContentSecurityPolicy::AllowReflectedXSS)
> > +        return XSSProtectionDisabled;
> > +
> > +    // If neither header is set, enable XSSAuditor.
> > +    return XSSProtectionEnabled;
> > +}
> 
> Can we change XSSProtectionDisposition to use ContentSecurityPolicy::ReflectedXSSDisposition and the make this function into something like std::max ?

We could get closer, but there's still the weirdness of wanting to enable the feature if neither is set, or if both are invalid. I'll take a look at integrating them more closely.

[Mike West]

(In reply to comment #14)
> > Tom, Adam: would you mind taking a look when you have a few minutes?
> Mike, this seems reasonable to me.  My only question would be whether you have a test that checks for the reflected-xss clause mixed in with various other CSP clauses to make sure that it doesn't break the existing ones, or vice versa.

Not yet. If you two are happy with the general direction, then I'll add that 5x5 set of tests. :)

[Build Bot]

Comment on attachment 187553 [details]
Patch

Attachment 187553 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16454346

New failing tests:
compositing/checkerboard.html
accessibility/anchor-linked-anonymous-block-crash.html
http/tests/cache/cancel-multiple-post-xhrs.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/3d/replace-filling-transform.html
http/tests/cache/history-only-cached-subresource-loads.html
compositing/bounds-in-flipped-writing-mode.html
accessibility/accessibility-node-reparent.html
animations/animation-border-overflow.html
accessibility/accessibility-object-detached.html
accessibility/anonymous-render-block-in-continuation-causes-crash.html
animations/animation-controller-drt-api.html
compositing/absolute-position-changed-with-composited-parent-layer.html
compositing/absolute-position-changed-in-composited-layer.html
http/tests/cache/iframe-304-crash.html
animations/3d/transform-perspective.html
http/tests/cache/cancel-during-failure-crash.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.5.html
canvas/philip/tests/2d.clearRect.basic.html
animations/3d/transform-origin-vs-functions.html
animations/animation-css-rule-types.html
canvas/philip/tests/2d.clearRect+fillRect.basic.html
canvas/philip/tests/2d.clearRect+fillRect.alpha0.html

[Mike West]

Created attachment 188079 [details]
Patch

[Mike West]

(In reply to comment #18)
> Created an attachment (id=188079) [details]
> Patch

This patch switches XSSAuditor's internals to use ContentSecurityPolicy::ReflectedXSSDisposition to determine state, and adds a pile of tests to verify behavior when mixing X-XSS-Protection and Content-Security-Policy headers.

Would you mind taking another look, Adam and Tom?

Thanks!

[Mike West]

(In reply to comment #19)
> (In reply to comment #18)
> > Created an attachment (id=188079) [details] [details]
> > Patch
> 
> This patch switches XSSAuditor's internals to use ContentSecurityPolicy::ReflectedXSSDisposition to determine state, and adds a pile of tests to verify behavior when mixing X-XSS-Protection and Content-Security-Policy headers.
> 
> Would you mind taking another look, Adam and Tom?
> 
> Thanks!

Ping. :)

[Thomas Sepez]

Comment on attachment 188079 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:1632
> +            disposition = m_policies[i]->reflectedXSSDisposition();

Could this just be dispostion = std::max(disposition, m_policies[i]->reflectedXSSDisposition())

> Source/WebCore/page/ContentSecurityPolicy.h:80
> +    };

Maybe a comment about the order being important for later comparisions.

[Thomas Sepez]

The rest looks good.

[Mike West]

Created attachment 189263 [details]
Patch

[Mike West]

(In reply to comment #21)
> (From update of attachment 188079 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188079&action=review
> 
> > Source/WebCore/page/ContentSecurityPolicy.cpp:1632
> > +            disposition = m_policies[i]->reflectedXSSDisposition();
> 
> Could this just be dispostion = std::max(disposition, m_policies[i]->reflectedXSSDisposition())
> 
> > Source/WebCore/page/ContentSecurityPolicy.h:80
> > +    };
> 
> Maybe a comment about the order being important for later comparisions.

Both good ideas, I've jammed them into the latest patch.

Thanks!

[Adam Barth]

Comment on attachment 189263 [details]
Patch

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

> Source/WebCore/html/parser/XSSAuditor.cpp:177
> +static ContentSecurityPolicy::ReflectedXSSDisposition combineXSSProtectionHeaderAndCSP(ContentSecurityPolicy::ReflectedXSSDisposition xssProtection, ContentSecurityPolicy::ReflectedXSSDisposition reflectedXSS)

I might add a comment to the ReflectedXSSDisposition declaration to remind folks to check this function if they add a new disposition.

> Source/WebCore/html/parser/XSSAuditor.cpp:266
> +        m_reportURL = xssProtectionReportURL; // FIXME: Combine the two report URLs in some reasonable way.

Do we need to make a copy of this URL to handle the threaded parser case?

[Mike West]

(In reply to comment #25)
> (From update of attachment 189263 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189263&action=review
> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:177
> > +static ContentSecurityPolicy::ReflectedXSSDisposition combineXSSProtectionHeaderAndCSP(ContentSecurityPolicy::ReflectedXSSDisposition xssProtection, ContentSecurityPolicy::ReflectedXSSDisposition reflectedXSS)
> 
> I might add a comment to the ReflectedXSSDisposition declaration to remind folks to check this function if they add a new disposition.

Makes good sense.

> > Source/WebCore/html/parser/XSSAuditor.cpp:266
> > +        m_reportURL = xssProtectionReportURL; // FIXME: Combine the two report URLs in some reasonable way.
> 
> Do we need to make a copy of this URL to handle the threaded parser case?

That sounds reasonable. Would `isolatedCopy()` be the right mechanism here?

[Adam Barth]

Comment on attachment 189263 [details]
Patch

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

>>> Source/WebCore/html/parser/XSSAuditor.cpp:266
>>> +        m_reportURL = xssProtectionReportURL; // FIXME: Combine the two report URLs in some reasonable way.
>> 
>> Do we need to make a copy of this URL to handle the threaded parser case?
> 
> That sounds reasonable. Would `isolatedCopy()` be the right mechanism here?

It looks like it's ok actually because xssProtectionReportURL comes from document->completeURL, which makes a new copy anyway.  There's an ASSERT in the threaded parser that will catch it if we mess up.

[Mike West]

Created attachment 189985 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 189985 [details]
Patch for landing

Clearing flags on attachment: 189985

Committed r143880: <http://trac.webkit.org/changeset/143880>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.