CSP: Strip the fragment from reported URLs.

third_party/WebKit/Source/core/frame/csp/ContentSecurityPolicy.cpp

 /*
  * Copyright (C) 2011 Google, Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 1008 matching lines @@
     const KURL& blockedURL,
     const String& header,
     RedirectStatus redirectStatus,
     ContentSecurityPolicyHeaderType headerType,
     ContentSecurityPolicy::ViolationType violationType,
     int contextLine) {
   if (effectiveType == ContentSecurityPolicy::DirectiveType::FrameAncestors) {
     // If this load was blocked via 'frame-ancestors', then the URL of
     // |document| has not yet been initialized. In this case, we'll set both
     // 'documentURI' and 'blockedURI' to the blocked document's URL.
-    init.setDocumentURI(blockedURL.getString());
-    init.setBlockedURI(blockedURL.getString());
+    init.setDocumentURI(blockedURL.strippedForUseAsReferrer());

[elawrence, 2017/01/09 16:34:41]

In the stripURLForUseInReport() function immediately prior, we note

// 'KURL::strippedForUseAsReferrer()' dumps 'String()' for non-webby URLs.
// It's better for developers if we return the origin of those URLs rather than
nothing.

I don't know if that concern applies here?

[Mike West, 2017/02/23 08:41:19]

For the `document-uri`, that's less of a concern, as it's quite difficult to
load any non-webby URL in the content area of Chrome. It would make reports from
`ftp:`, `chrome:`, and `chrome-extension:` URLs less useful, but we also don't
actually send reports for those URLs (as there are no headers, and therefore no
ability to set `report-uri` in the policy).

Still, it's worth being consistent. I'll adjust. :)

+    init.setBlockedURI(blockedURL.strippedForUseAsReferrer());
   } else {
-    init.setDocumentURI(context->url().getString());
+    init.setDocumentURI(context->url().strippedForUseAsReferrer());
     switch (violationType) {
       case ContentSecurityPolicy::InlineViolation:
         init.setBlockedURI("inline");
         break;
       case ContentSecurityPolicy::EvalViolation:
         init.setBlockedURI("eval");
         break;
       case ContentSecurityPolicy::URLViolation:
         init.setBlockedURI(stripURLForUseInReport(
             context, blockedURL, redirectStatus, effectiveType));
@@ skipping 12 matching lines @@
   init.setSourceFile(String());
   init.setLineNumber(contextLine);
   init.setColumnNumber(0);
   init.setStatusCode(0);
 
   // TODO(mkwst): We only have referrer and status code information for
   // Documents. It would be nice to get them for Workers as well.
   if (context->isDocument()) {
     Document* document = toDocument(context);
     DCHECK(document);
     init.setReferrer(document->referrer());

[elawrence, 2017/01/09 16:34:41]

The algorithm described here
(https://www.w3.org/TR/CSP/#deprecated-serialize-violation) says that the
Referer should also be stripped. Is that not necessary because a Referrer
inherently has already been sanitized?

[Mike West, 2017/02/23 08:41:19]

Yes.

     if (!SecurityOrigin::isSecure(context->url()) && document->loader())
       init.setStatusCode(document->loader()->response().httpStatusCode());
   }
 
   std::unique_ptr<SourceLocation> location = SourceLocation::capture(context);
   if (location->lineNumber()) {
     KURL source = KURL(ParsedURLString, location->url());
     init.setSourceFile(
         stripURLForUseInReport(context, source, redirectStatus, effectiveType));
     init.setLineNumber(location->lineNumber());
@@ skipping 73 matching lines @@
   if (!document)
     return;
 
   // We need to be careful here when deciding what information to send to the
   // report-uri. Currently, we send only the current document's URL and the
   // directive that was violated. The document's URL is safe to send because
   // it's the document itself that's requesting that it be sent. You could
   // make an argument that we shouldn't send HTTPS document URLs to HTTP
   // report-uris (for the same reasons that we supress the Referer in that
   // case), but the Referer is sent implicitly whereas this request is only
   // sent explicitly. As for which directive was violated, that's pretty

[elawrence, 2017/01/09 16:34:41]

The justification for allowing HTTPS URIs to leak out over HTTP seems very weak
to me. Yes, the report is sent "explicitly" but probably without awareness of
the web developer, who didn't expect a script injection hole in his website to
begin with, and thus didn't expect the report to ever fire. We're allowing a
webdev to convert XSS into an InfoLeak, which may not be much better.

Allowing HTTP reporting endpoints for HTTPS pages seems like a rotten idea.

[Mike West, 2017/02/23 08:41:19]

I agree. I'm sure this was a good idea at the time, but it's a bad idea today.
Filed a bug, added a TODO. We should deprecate/remove that behavior.

   // harmless information.
 
   std::unique_ptr<JSONObject> cspReport = JSONObject::create();
   cspReport->setString("document-uri", violationData.documentURI());
   cspReport->setString("referrer", violationData.referrer());
   cspReport->setString("violated-directive", violationData.violatedDirective());
   cspReport->setString("effective-directive",
                        violationData.effectiveDirective());
   cspReport->setString("original-policy", violationData.originalPolicy());
   cspReport->setString("disposition", violationData.disposition());
@@ skipping 436 matching lines @@
   CSPDirectiveListVector otherVector;
   for (const auto& policy : other.m_policies) {
     if (!policy->isReportOnly())
       otherVector.push_back(policy);
   }
 
   return m_policies[0]->subsumes(otherVector);
 }
 
 }  // namespace blink