Bug 100538 – malformed X-XSS-Protection headers not reported

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Thomas Sepez]

Follow-on from https://bugs.webkit.org/show_bug.cgi?id=100423, where we noticed that a badly-formed X-XSS-Protection header is silently ignored.
Two steps required:
1. Make some noise (log a console message when parse fails)
2. Add a test for it with some bad examples.

[Thomas Sepez]

Actually, there was a test, but bad headers aren't flagged should they be accidentally introduced in other tests.

[Thomas Sepez]

Created attachment 170984 [details]
Patch for discussion

We need to discuss the approach before I cobble together changelogs and the like for actual submission.

[Thomas Sepez]

Also note that at line 326 or thereabouts there looks to be a legit bug 
    unsigned length = header.length();
vs.
   unsigned length = strippedHeader.length();

which would theoretically fail to kick in full blocking mode.  Looks like this would only be tickled by the test xss-protection-parsing-01.html if at all.

[Mike West]

Comment on attachment 170984 [details]
Patch for discussion

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

Hi Tom! Two superquick comments before bedtime.

> Source/WebCore/html/parser/XSSAuditor.cpp:222
> +            DEFINE_STATIC_LOCAL(String, consoleMessage, (ASCIILiteral("Invalid header: X-XSS-Protection: ")));

Nit: The header itself isn't invalid, just its value. It might also be worth noting the effect (that protection is enabled, regardless of the author's misstated intent). If you don't mind verbosity, I'd suggest something along the lines of "The X-XSS-Protection HTTP header's value, '[VALUE]', is malformed. The header will be ignored, and protection will be enabled."

> Source/WebCore/platform/network/HTTPParsers.cpp:331
> +        return XSSProtectionInvalid;

If you enumerate the ways things can break, you'd be able to give more detailed error messages. Returning XSSProtectionMalformedToggle might result in "The first non-whitespace character must be 0 or 1.", and XSSProtectionMalformedMode could do likewise.

If you actually moved logging into this function, you could have even more detail, but it doesn't look like that would be straightforward since there's no reference to the document anywhere in this file.

[Thomas Sepez]

Comment on attachment 170984 [details]
Patch for discussion

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

> Source/WebCore/platform/network/HTTPParsers.cpp:338
> +    if (!strippedHeader[pos++] == ';')

You moron.  Look at what you wrote here.

[Adam Barth]

Hey now!  Be nice to yourself.  :)

[Thomas Sepez]

Comment on attachment 170984 [details]
Patch for discussion

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

> Source/WebCore/platform/network/HTTPParsers.cpp:77
> +    pos = current;

This looks like its been wrong for a long time. eg given input like "mode=blo\n", we'd say that  "blo" matches against "block".

[Thomas Sepez]

Created attachment 171297 [details]
patch + changelogs for landing

- Rather than pass back an ever increasing set of enums, I added a String& failureReason argument along the lines of some of the other parsers.
- Split out success cases into xss-protection-parsing-* to match existing, and put malformed cases into malformed-*.

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1
Source/WebCore/platform/network/HTTPParsers.cpp:355:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/HTTPParsers.cpp:356:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/HTTPParsers.cpp:357:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/platform/network/HTTPParsers.cpp:358:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 4 in 18 files


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

[Thomas Sepez]

Created attachment 171300 [details]
Patch, fix style, add missing file.

[Adam Barth]

Comment on attachment 171300 [details]
Patch, fix style, add missing file.

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

> Source/WebCore/html/parser/XSSAuditor.cpp:225
> +            DEFINE_STATIC_LOCAL(String, consoleMessageStart, (ASCIILiteral("Error parsing header X-XSS-Protection: ")));
> +            DEFINE_STATIC_LOCAL(String, consoleMessageSeparator, (ASCIILiteral(": ")));
> +            DEFINE_STATIC_LOCAL(String, consoleMessageEnd, (ASCIILiteral(". The default protections will be applied.")));

There isn't really a benefit to defining these statics since you're going to concatenate them anyway.  The most efficient thing is to just write out the whole concatenation on one line with the literals and everything.

[WebKit Review Bot]

Comment on attachment 171300 [details]
Patch, fix style, add missing file.

Clearing flags on attachment: 171300

Committed r133066: <http://trac.webkit.org/changeset/133066>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.