Bug 34436

Summary:

Change XSSAuditor block syntax

Product:

WebKit

Reporter:

Adam Barth <abarth>

Component:

Frames

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, dbates, eric, sam

Priority:

Keywords:

XSSAuditor

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

Bug Blocks:

36933

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch for landing	none

Adam Barth

Reported 2010-02-01 10:03:27 PST

Apparently, it would be better if we used the following syntax for the full-page block header for XSSAuditor: "X-XSS-Protection:" OWS "1" ";" 1*LWS "mode" "=" "block" OWS in non-ABNF form, that looks like: X-XSS-Protection: 1; mode=block

Attachments
Patch (16.37 KB, patch) 2010-02-26 15:03 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch (14.95 KB, patch) 2010-02-26 15:06 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch (14.88 KB, patch) 2010-02-27 01:02 PST, Adam Barth	no flags	Details Formatted Diff Diff
Patch for landing (14.96 KB, patch) 2010-03-19 15:33 PDT, Adam Barth	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Adam Barth

Comment 1 2010-02-01 10:09:47 PST

Oops: "X-XSS-Protection:" OWS "1" ";" "mode" "=" "block" OWS Note that this is the "new" ABNF syntax, so the following would be valid: X-XSS-Protection: 1;mode=block X-XSS-Protection: 1 ; mode =block because LWS (spaces and tabs) are allowed between each token. Also the tokens are case insensitive.

Adam Barth

Comment 2 2010-02-26 15:03:48 PST

Created attachment 49651 [details] Patch

Adam Barth

Comment 3 2010-02-26 15:06:55 PST

Created attachment 49653 [details] Patch

Daniel Bates

Comment 4 2010-02-26 23:46:05 PST

In the expected results file "xss-protection-parsing-01-expected.txt" there is a timed out fail message: ... +CONSOLE MESSAGE: line 1: Refused to execute a JavaScript script. Source code of script found within request. + +FAIL: Timed out waiting for notifyDone to be called +This tests our parsing of the X-XSS-Protection header. ...

Adam Barth

Comment 5 2010-02-27 00:28:10 PST

Comment on attachment 49653 [details] Patch Ah, that's no good.

Adam Barth

Comment 6 2010-02-27 01:02:16 PST

Created attachment 49678 [details] Patch

Eric Seidel (no email)

Comment 7 2010-03-15 13:26:24 PDT

This matches some new spec? How should a reviewer evaluate this patch?

Adam Barth

Comment 8 2010-03-15 13:28:11 PDT

> This matches some new spec? How should a reviewer evaluate this patch? It matches what some IE folks asked us to use.

Daniel Bates

Comment 9 2010-03-18 21:17:18 PDT

Comment on attachment 49678 [details] Patch > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,27 @@ > +2010-02-27 Adam Barth <abarth@webkit.org> > [...] > + * WebCore.xcodeproj/project.pbxproj: This line should be removed from the change log since this patch does not contain any changes to the WebCore Xcode project file. > -bool XSSAuditor::shouldFullPageBlockForXSSProtectionHeader() const > +XSSProtectionDisposition XSSAuditor::xssProtection() const > { > // If we detect an XSS attack and find the HTTP header "X-XSS-Protection: 12" then > // we will stop loading the page as opposed to ignoring the script. The value "12" > @@ -302,9 +302,7 @@ bool XSSAuditor::shouldFullPageBlockForXSSProtectionHeader() const Either this comment needs to removed or it needs to be updated since we are no longer using the "12" notation to do full-page blocking. > +XSSProtectionDisposition parseXSSProtectionHeader(const String& header) > +{ > + String stippedHeader = header.stripWhiteSpace(); > + > + if (stippedHeader.isEmpty()) > + return XSSProtectionEnabled; > + > + if (stippedHeader[0] == '0') > + return XSSProtectionDisabled; > + > + int length = (int)header.length(); Minor style issue, the explicit cast to type integer (i.e. "(int)") is unnecessary. r=me

Adam Barth

Comment 10 2010-03-19 15:33:27 PDT

Created attachment 51194 [details] Patch for landing

Adam Barth

Comment 11 2010-03-19 15:34:33 PDT

> This line should be removed from the change log since this patch does not > contain any changes to the WebCore Xcode project file. Fixed. > Either this comment needs to removed or it needs to be updated since we are no > longer using the "12" notation to do full-page blocking. Removed. > Minor style issue, the explicit cast to type integer (i.e. "(int)") is > unnecessary. I left it in because it's converting from unsigned to signed and other instances in this file do it too. (I think one of the ports might have a warning-as-error in this case.)

WebKit Commit Bot

Comment 12 2010-03-19 20:55:20 PDT

Comment on attachment 51194 [details] Patch for landing Clearing flags on attachment: 51194 Committed r56295: <http://trac.webkit.org/changeset/56295>

WebKit Commit Bot

Comment 13 2010-03-19 20:55:25 PDT

All reviewed patches have been landed. Closing bug.

Eric Seidel (no email)

Comment 14 2010-03-31 15:59:22 PDT

http/tests/security/xssAuditor/xss-protection-parsing-01.html has failed on the Tiger bot ever since this commit. Please fix. :)

Note You need to log in before you can comment on or make changes to this bug.