Bug 100892 – Support X-XSS-Protection: report=URL header syntax in XSSAuditor.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Thomas Sepez]

This is the tracking bug for a proposed security feature which would allow a violation report to be sent back to a site when the XSSAuditor detects a reflected XSS against it.

The current X-XSS-Protection header syntax is roughly:
    X-XSS-Protection: (0.*|1{;mode=block;})

In other words, a 0 followed by anything disables XSS protection. A 1 optionally followed by "; mode=block" enables protection.

The proposed syntax is roughly:
    X-XSS-Protection: (0.*|1{;mode=block}{;report=.*})

where the report keyword must come last and uses the rest of the header as the reporting URL.  The reporting URL is also restricted to the same origin as the page URL (upgrade from HTTP to HTTPS for reporting permitted), as a defense against site misconfiguration.

The report is JSON and is generated using the same facility as for the X-WebKit-CSP-Report-Only header.  A sample might be:
    { "xss-report": {
        "request-url": "http://example.com/vulnerable-page?q=<script>alert(/xss/)</script>",
        "request-body": ""
    }}

request-body would include the POST parameters, if any.

[Thomas Sepez]

Q1.  Should the same-origin restriction on the reporting URL be lifted?
Q2.  Should there be a way to get a report without having XSSAuditor either rewrite or block the page?

[Thomas Sepez]

Created attachment 171725 [details]
Code for discussion

This is the code I put together to evaluate the feature.

[Adam Barth]

Comment on attachment 171725 [details]
Code for discussion

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

> Source/WebCore/html/parser/XSSAuditor.cpp:238
> +                m_reportURL = KURL(m_parser->document()->url(), reportURL);

Why not just use document()->completeURL(reportURL)

> Source/WebCore/html/parser/XSSAuditor.cpp:241
> +                m_notifyReportURL = (m_reportURL.protocolIs("https") || m_reportURL.protocol() == m_parser->document()->url().protocol())
> +                    && m_reportURL.host() == m_parser->document()->url().host()
> +                    && m_reportURL.port() == m_parser->document()->url().port();

We shouldn't be doing this sort of comparison manually.  We probably should be calling MixedContentChecker::isMixedContent.  We might need to make that public.

I'm not sure we need the host or port restrictions.  If we do, we should probably have SecurityOrigin tell us about which things are the same origin.

> Source/WebCore/platform/network/HTTPParsers.cpp:382
> +        reportURL = header.substring(pos);

We should probably stop at the first ' ' or ';' character.

[Adam Barth]

> Q1.  Should the same-origin restriction on the reporting URL be lifted?

I think it makes sense to prevent HTTPS pages from sending their reports to HTTP pages.  The MixedContentChecker is a good way to do that.

> Q2.  Should there be a way to get a report without having XSSAuditor either rewrite or block the page?

I think it's fine to require rewriting or blocking for now.  The feature is on by default, so most folks will run it in rewriting mode.  The folks who turn it off explicitly probably just want the darn thing off.

[Thomas Sepez]

Well, if we're going to stop at spaces or semicolons, we should probably accept either ordering of mode= and report=.  More stuff to test though.

[Adam Barth]

That's probably a good idea anyway since that's how most HTTP headers work.

[Thomas Sepez]

Created attachment 171745 [details]
More code for discussion

[Adam Barth]

Comment on attachment 171745 [details]
More code for discussion

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

This looks great.  Can you add a test that the report is only sent once even if there are many violations on a page?

> Source/WebCore/html/parser/XSSAuditor.cpp:238
> +                m_notifyReportURL = true;

You can also null out m_reportURL if you don't want to send the report.

> Source/WebCore/html/parser/XSSAuditor.cpp:317
> +            m_notifyReportURL = false;
> +            m_originalURL = String();
> +            m_originalHTTPBody = String();

So, we only notify for the first failure, and then we drop the m_originalHTTPBody.

Do we ever drop m_originalHTTPBody if there isn't an error?  It's probably worth dropping it at some point since we don't want to keep using the memory.  I guess the whole XSSAuditor gets deleted at some point.  Maybe that's early enough.

[WebKit Review Bot]

Attachment 171745 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/http/tests/security/xssAuditor..." exit_code: 1
Source/WebCore/html/parser/XSSAuditor.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/network/HTTPParsers.cpp:86:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Thomas Sepez]

>  Can you add a test that the report is only sent once even if there are many violations on a page?
Turns out this may be tricky to do in a non-flakey manner.  The current echo-report script races the creation of the report by the save-report script, using the existence of the report file to trigger completion.  While save-report could open the report file in append mode, I'm not sure how to wait for the non-occurrence of the second report so that it doesn't trigger after the first one and always pass.

> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:238
> > +                m_notifyReportURL = true;
> 
> You can also null out m_reportURL if you don't want to send the report.

Sure.  Will do. 

> 
> > Source/WebCore/html/parser/XSSAuditor.cpp:317
> > +            m_notifyReportURL = false;
> > +            m_originalURL = String();
> > +            m_originalHTTPBody = String();
> 
> So, we only notify for the first failure, and then we drop the m_originalHTTPBody.
> 
> Do we ever drop m_originalHTTPBody if there isn't an error?  It's probably worth dropping it at some point since we don't want to keep using the memory.  I guess the whole XSSAuditor gets deleted at some point.  Maybe that's early enough.

I couldn't figure out any other appropriate point prior to XSSAuditor destruction to clean this up.

[Thomas Sepez]

Created attachment 171907 [details]
Patch, add changelogs etc.

Note: some more tinkering with parseXSSProtectionHeader().

[Adam Barth]

Comment on attachment 171907 [details]
Patch, add changelogs etc.

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

> Source/WebCore/html/parser/XSSAuditor.cpp:242
> +            m_parser->document()->addConsoleMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, "Error parsing header X-XSS-Protection: " + headerValue + ": "  + errorDetails + " at character position " + String::format("%u", errorPosition) + ". The default protections will be applied.");

String::format("%u", errorPosition)  <---  There's a fancier way to do that which avoids the extra malloc.

[WebKit Review Bot]

Comment on attachment 171907 [details]
Patch, add changelogs etc.

Clearing flags on attachment: 171907

Committed r133323: <http://trac.webkit.org/changeset/133323>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.