Bug 104479

Created attachment 178408 [details] Patch

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

Created attachment 178424 [details] WIP

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?

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.

Created attachment 183936 [details] Patch

Created attachment 183937 [details] Patch

(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!

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

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

Created attachment 187553 [details] Patch

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?

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 ?

> 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.

(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.

(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. :)

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

Created attachment 188079 [details] Patch

(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!

(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. :)

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.

The rest looks good.

Created attachment 189263 [details] Patch

(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!

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?

(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?

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.

Created attachment 189985 [details] Patch for landing

Comment on attachment 189985 [details] Patch for landing Clearing flags on attachment: 189985 Committed r143880: <http://trac.webkit.org/changeset/143880>

All reviewed patches have been landed. Closing bug.