PlzNavigate: Do not disclose urls between cross-origin renderers.

content/common/content_security_policy/content_security_policy.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <sstream>
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "content/common/content_security_policy/csp_context.h"
 
 namespace content {
@@ skipping 26 matching lines @@
   return url.spec();
 }
 
 void ReportViolation(CSPContext* context,
                      const ContentSecurityPolicy& policy,
                      const CSPDirective& directive,
                      const CSPDirective::Name directive_name,
                      const GURL& url,
                      bool is_redirect,
                      const SourceLocation& source_location) {
+  GURL safe_url = context->IsOriginSafeToUseInCspViolation(url::Origin(url))

[alexmos, 2017/05/10 22:33:08]

It feels like we actually want to ask this question about the URL rather than
origin.  I.e., origins are always safe to use -- it's the URL that we sometimes
want to strip to an origin.  So what about changing this to something like
IsURLSafeForCspViolation, passing in the whole URL, and letting RFHI compare
origins internally?

[arthursonzogni, 2017/05/11 13:06:23]

Yes, that is what I did initially. But even if the url doesn't contains any
sensitive data. I still would like to clear the line/column number.

I will try to rephrase this. ShouldProtectDataInCspViolation(...)? It is hard to
make it short and meaningful.

[alexmos, 2017/05/12 01:37:19]

Yeah, it's hard, and I guess it also depends on what we decide with line/column
numbers (see below).  Perhaps we could further clarify this to
ShouldSanitizeDataInCspViolation?

Another option is to let CSPContext do this internally and call this
StripURLForUseInReport, which would take care of any necessary stripping
internally.  This is also what Blink does today.  This is nice because you end
up doing the stripping only in one place.  But I guess you'd also need
StripSourceLocationForUseInReport if we end up also doing line/column clearing
here (which can call StripURLForUseInReport internally for the
source_location.url).  

[arthursonzogni, 2017/05/15 12:20:46]

Thanks for "ShouldSanitizeDataInCspViolation".
I will apply your suggestion to strip urls in the CSPContext instead.

+                      ? url
+                      : url.GetOrigin();

[alexmos, 2017/05/10 22:33:08]

Both of these checks could use a comment explaining the rationale.

[arthursonzogni, 2017/05/11 13:06:23]

Done.

+
+  SourceLocation safe_source_location =
+      context->IsOriginSafeToUseInCspViolation(
+          url::Origin(GURL(source_location.url)))
+          ? source_location
+          : SourceLocation();

[alexmos, 2017/05/10 22:33:08]

Interesting, so we clear it out entirely?  This doesn't seem to match what the
renderer-side check is doing, which is only changing the source_location.url to
an origin and leaving the line/column numbers there (is that right?).  If we end
up changing the behavior here (e.g., not reporting line/column numbers anymore),
let's get mkwst@'s confirmation for it.  I can see that we might want to stop
leaking cross-origin line/column numbers, but it might still make sense to
include the source_location.url stripped to an origin.

[arthursonzogni, 2017/05/11 13:06:23]

I think we have to clear the line/column numbers because it might also be a
sensitive information. It tells an attacker what line of JavaScript code
triggers the navigation. mkwst@: what do you think of it?

I don't know what is the best thing between:
* SourceLocation()
* SourceLocation(source_location.url.GetOrigin(), 0, 0)  [0 means unknown]

In the next patch, I will use the second one.

[alexmos, 2017/05/12 01:37:19]

I'll leave this up to Mike.  I agree it seems more desirable to also clear out
the line/column number, but that seems like a web-facing change in how CSP works
which might need to go through the blink intent process.  Whereas stripping URLs
to origins is something that we already do in cross-origin cases.  So, it might
be better to leave the line/column numbers alone (i.e., present) in this CL, so
that there's no behavior change here, and then put together another CL to
further strip them away and have this discussion there?

+
   // We should never have a violation against `child-src` or `default-src`
   // directly; the effective directive should always be one of the explicit
   // fetch directives.
   DCHECK_NE(directive_name, CSPDirective::DefaultSrc);
   DCHECK_NE(directive_name, CSPDirective::ChildSrc);
 
   std::stringstream message;
 
   if (policy.header.type == blink::kWebContentSecurityPolicyTypeReport)
     message << "[Report Only] ";
 
   if (directive_name == CSPDirective::FormAction)
     message << "Refused to send form data to '";
   else if (directive_name == CSPDirective::FrameSrc)
     message << "Refused to frame '";
 
-  message << ElideURLForReportViolation(url)
+  message << ElideURLForReportViolation(safe_url)
           << "' because it violates the following Content Security Policy "
              "directive: \""
           << directive.ToString() << "\".";
 
   if (directive.name != directive_name)
     message << " Note that '" << CSPDirective::NameToString(directive_name)
             << "' was not explicitly set, so '"
             << CSPDirective::NameToString(directive.name)
             << "' is used as a fallback.";
 
   message << "\n";
 
   context->ReportContentSecurityPolicyViolation(CSPViolationParams(
       CSPDirective::NameToString(directive.name),
-      CSPDirective::NameToString(directive_name), message.str(), url,
+      CSPDirective::NameToString(directive_name), message.str(), safe_url,
       policy.report_endpoints, policy.header.header_value, policy.header.type,
-      is_redirect, source_location));
+      is_redirect, safe_source_location));
 }
 
 bool AllowDirective(CSPContext* context,
                     const ContentSecurityPolicy& policy,
                     const CSPDirective& directive,
                     CSPDirective::Name directive_name,
                     const GURL& url,
                     bool is_redirect,
                     const SourceLocation& source_location) {
   if (CSPSourceList::Allow(directive.source_list, url, context, is_redirect))
@@ skipping 80 matching lines @@
     is_first_policy = false;
     text << "report-uri";
     for (const std::string& endpoint : report_endpoints)
       text << " " << endpoint;
   }
 
   return text.str();
 }
 
 }  // namespace content