Bug 100892

Summary: Support X-XSS-Protection: report=URL header syntax in XSSAuditor.
Product: WebKit Reporter: Thomas Sepez <tsepez>
Component: WebCore Misc.Assignee: Thomas Sepez <tsepez>
Status: RESOLVED FIXED    
Severity: Minor CC: abarth, dbates, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Code for discussion
none
More code for discussion
abarth: review+
Patch, add changelogs etc. none

Thomas Sepez
Reported 2012-10-31 14:45:36 PDT
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.
Attachments
Code for discussion (22.78 KB, patch)
2012-10-31 14:50 PDT, Thomas Sepez
no flags
More code for discussion (34.56 KB, patch)
2012-10-31 17:23 PDT, Thomas Sepez
abarth: review+
Patch, add changelogs etc. (43.68 KB, patch)
2012-11-01 12:15 PDT, Thomas Sepez
no flags
Thomas Sepez
Comment 1 2012-10-31 14:47:10 PDT
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
Comment 2 2012-10-31 14:50:05 PDT
Created attachment 171725 [details] Code for discussion This is the code I put together to evaluate the feature.
Adam Barth
Comment 3 2012-10-31 15:09:27 PDT
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
Comment 4 2012-10-31 15:51:18 PDT
> 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
Comment 5 2012-10-31 16:00:35 PDT
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
Comment 6 2012-10-31 16:07:20 PDT
That's probably a good idea anyway since that's how most HTTP headers work.
Thomas Sepez
Comment 7 2012-10-31 17:23:17 PDT
Created attachment 171745 [details] More code for discussion
Adam Barth
Comment 8 2012-10-31 23:07:34 PDT
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
Comment 9 2012-11-01 10:21:08 PDT
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
Comment 10 2012-11-01 11:45:35 PDT
> 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
Comment 11 2012-11-01 12:15:05 PDT
Created attachment 171907 [details] Patch, add changelogs etc. Note: some more tinkering with parseXSSProtectionHeader().
Adam Barth
Comment 12 2012-11-02 11:21:43 PDT
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 13 2012-11-02 11:51:20 PDT
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
Comment 14 2012-11-02 11:51:24 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.