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

content/common/content_security_policy/content_security_policy.cc

Index: content/common/content_security_policy/content_security_policy.cc
diff --git a/content/common/content_security_policy/content_security_policy.cc b/content/common/content_security_policy/content_security_policy.cc
index 87b035eb1c2ef44916452f1ea3e186ae29f75d64..c0ba794acc8ac375f99305c5bc60c5e28bad8281 100644
--- a/content/common/content_security_policy/content_security_policy.cc
+++ b/content/common/content_security_policy/content_security_policy.cc
@@ -44,6 +44,16 @@ void ReportViolation(CSPContext* context,
                      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.
@@ -60,7 +70,7 @@ void ReportViolation(CSPContext* context,
   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() << "\".";
@@ -75,9 +85,9 @@ void ReportViolation(CSPContext* context,
 
   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,