Bug 27312 – [XSSAuditor] Add support for header X-XSS-Protection

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

[Daniel Bates]

We should add support to disable the XSSAuditor using the same header used by the XSS filter in IE 8, X-XSS-Protection.

[Daniel Bates]

Created attachment 32827 [details]
Work in progress patch with tests

This patch is a work in progress. It behaves as follows:

The X-XSS-Protection header is only obeyed if its value is 0 (i.e. disabled) and "X-XSS-Protection" does appear in the HTTP parameters.

We may be walking a fine line in trying to implement this as we do no not want to open ourselves up to a CRLF attack, such as http://www.linkstofiles.com/crlf.py?url=cooki1%3dvalue1;%0d%0aX-XSS-Protection:0%0d%0a%0d%0a%3Chtml%3E%3Cbody%3E%3Cscript%3Ealert(%27owned%27)%3C/script%3E%3C/body%3E%3C/html%3E (Credit: http://packetstormsecurity.org/0812-exploits/ie80-xss.txt).

[Adam Barth]

Sam, Maciej, and I discussed this on IRC just now.  Instead of re-using the IE8 control header, I think we should create our own header with three states:

1) Not present
2) X-XSS-Auditor-Options: ignore
3) X-XSS-Auditor-Options: fullpageblock

In state (1), we should keep doing what we do now.  In state (2), we should disable the XSSAuditor for that page.  In state (3), we should stop rendering the page and show an error message (you can see an example of how to do this with the X-Frame-Options logic).

I don't think we should worry about header injection issues at this point.  The auditor doesn't cover every vulnerability anyway.  The number of XSS + header injections should be relatively low.  If this turns out to be a big problem, we can always add that feature later.

[Daniel Bates]

Created attachment 46334 [details]
Path (Work in progress)

Updated patch. Still working on this. There are some issues with the newly added DRT tests in that they are a bit flaky.

[Daniel Bates]

Created attachment 47316 [details]
Patch with test cases

[Daniel Bates]

Just to be clear, this patch <https://bugs.webkit.org/attachment.cgi?id=47316> only implements full page blocking.

[Adam Barth]

Comment on attachment 47316 [details]
Patch with test cases

This looks pretty good.  One problem though:

+ window.setTimeout(done, millisecondsToWait);

Don't want to use setTimeout to control when the test ends.  This will make the test flaky on slow bots, like ones running valgrind.

Instead, you should count how many times the load event for the frame fires.  It should fire a deterministic number of times.

Have you tried not waiting until done?  The load event for the main frame might be delayed until the iframe finishes all its loading.

[Daniel Bates]

I would prefer not use setTimeout as well.

From both observation and briefly tracing through the code, the load event does not seem to be fired on a scheduled redirect to about:blank. So, counting the number of load events fired will not work because it will not reflect the scheduled redirect. Moreover, the test fails if I do not wait for the redirect to about:blank since the test result will have the content of the <iframe> instead of being blank.

(In reply to comment #6)
> (From update of attachment 47316 [details])
> This looks pretty good.  One problem though:
> 
> + window.setTimeout(done, millisecondsToWait);
> 
> Don't want to use setTimeout to control when the test ends.  This will make the
> test flaky on slow bots, like ones running valgrind.
> 
> Instead, you should count how many times the load event for the frame fires. 
> It should fire a deterministic number of times.
> 
> Have you tried not waiting until done?  The load event for the main frame might
> be delayed until the iframe finishes all its loading.

[Adam Barth]

How do the X-Frame-Options tests get around this problem?  They might be a good model for this test.

[Daniel Bates]

Created attachment 47497 [details]
Patch with test cases

Removed use of window.setTimeout from test cases.

[Adam Barth]

The code looks good.  One final question: how does IE8's XSS filter behave if you send it:

X-XSS-Protection: full-block

?

Ideally, that wouldn't disable the IE8 filter so paranoid folks could enable our full-block and keep IE8's neutering.

[Daniel Bates]

I'll look into this.

Also, what are your thoughts about using own header for now? X-XSS-Auditor-Options?

(In reply to comment #10)
> The code looks good.  One final question: how does IE8's XSS filter behave if
> you send it:
> 
> X-XSS-Protection: full-block
> 
> ?
> 
> Ideally, that wouldn't disable the IE8 filter so paranoid folks could enable
> our full-block and keep IE8's neutering.

[Adam Barth]

> Also, what are your thoughts about using own header for now?
> X-XSS-Auditor-Options?

I'd prefer to use the same header if at all possible.  I've been told that we can use the value 12 to trigger a full-page block.  Why 12?  That's a question for the ages.

[Adam Barth]

To be clear, here are the exact semantics we want:

[[
We simply look at the first non-whitespace character of the value of the first X-XSS-Protection response header.  If it's '0', we disable protection. If it's '1', we enable protection. We ignore the rest of the line, although if the value is longer than 16 characters, we ignore the whole thing.
]]

So, for 12, we'd want that if the first two characters are "12" then we turn on the full page block regardless of what the rest of the header is (as long as it's less than or equal to 16 characters).  Crazy, I know.

[Daniel Bates]

Will update the patch. By the way, do you have a reference for the quote?

(In reply to comment #13)
> To be clear, here are the exact semantics we want:
> 
> [[
> We simply look at the first non-whitespace character of the value of the first
> X-XSS-Protection response header.  If it's '0', we disable protection. If it's
> '1', we enable protection. We ignore the rest of the line, although if the
> value is longer than 16 characters, we ignore the whole thing.
> ]]
> 
> So, for 12, we'd want that if the first two characters are "12" then we turn on
> the full page block regardless of what the rest of the header is (as long as
> it's less than or equal to 16 characters).  Crazy, I know.

[Adam Barth]

> Will update the patch. By the way, do you have a reference for the quote?

"Personal communication"  :)

I'm told it's documented publicly, but I'm not sure where.

[Daniel Bates]

Created attachment 47803 [details]
Patch with test cases

[Adam Barth]

Comment on attachment 47803 [details]
Patch with test cases

Awesome.  Thanks Dan!

What do you think about implementing X-XSS-Protection: 0?

[Daniel Bates]

Committed r54202: <http://trac.webkit.org/changeset/54202>