Stop CSP from matching independent scheme/port upgrades (content layer)

content/common/content_security_policy/csp_source.cc

Index: content/common/content_security_policy/csp_source.cc
diff --git a/content/common/content_security_policy/csp_source.cc b/content/common/content_security_policy/csp_source.cc
index 59657d16d2bfbddb3922549c059497409d957f8f..abea6f8552513ddf28e5bea2acbfb5eb09dd8b3c 100644
--- a/content/common/content_security_policy/csp_source.cc
+++ b/content/common/content_security_policy/csp_source.cc
@@ -24,16 +24,38 @@ int DefaultPortForScheme(const std::string& scheme) {
   return url::DefaultPortForScheme(scheme.data(), scheme.size());
 }
 
-bool SourceAllowScheme(const CSPSource& source,
-                       const GURL& url,
-                       CSPContext* context) {
-  if (source.scheme.empty())
-    return context->ProtocolMatchesSelf(url);
-  if (source.scheme == url::kHttpScheme)
-    return url.SchemeIsHTTPOrHTTPS();
-  if (source.scheme == url::kWsScheme)
-    return url.SchemeIsWSOrWSS();
-  return url.SchemeIs(source.scheme);
+CSPSource::SchemeMatchingResult SourceAllowScheme(const CSPSource& source,
+                                                  const GURL& url,
+                                                  CSPContext* context) {
+  const std::string& source_scheme =
+      source.scheme.empty() ? context->GetSelfScheme() : source.scheme;
+
+  if (source_scheme.empty()) {
+    if (context->ProtocolIsSelf(url))
+      return CSPSource::SchemeMatchingResult::MatchingExact;
+    return CSPSource::SchemeMatchingResult::NotMatching;
+  }
+
+  if (url.SchemeIs(source_scheme))
+    return CSPSource::SchemeMatchingResult::MatchingExact;
+
+  if ((source_scheme == url::kHttpScheme &&
+       url.SchemeIs(url::kHttpsScheme)) ||

[arthursonzogni, 2017/04/07 09:20:27]

Nit: git cl format: it should fit on one line.

[andypaicu, 2017/04/07 11:34:24]

Done. I hated the fact that the middle one does not fit but long live "git cl
format".

+      (source_scheme == url::kHttpScheme &&
+       url.SchemeIs(url::kHttpsSuboriginScheme)) ||
+      (source_scheme == url::kWsScheme &&
+       url.SchemeIs(url::kWssScheme))) {

[arthursonzogni, 2017/04/07 09:20:27]

Nit: git cl format: it should fit on one line.

[andypaicu, 2017/04/07 11:34:24]

Done. I hated the fact that the middle one does not fit but long live "git cl
format".

+    return CSPSource::SchemeMatchingResult::MatchingUpgrade;
+  }
+
+  if ((source_scheme == url::kHttpScheme &&
+       url.SchemeIs(url::kHttpSuboriginScheme)) ||
+      (source_scheme == url::kHttpsScheme &&
+       url.SchemeIs(url::kHttpsSuboriginScheme))) {
+    return CSPSource::SchemeMatchingResult::MatchingExact;
+  }
+
+  return CSPSource::SchemeMatchingResult::NotMatching;
 }
 
 bool SourceAllowHost(const CSPSource& source, const GURL& url) {
@@ -50,22 +72,34 @@ bool SourceAllowHost(const CSPSource& source, const GURL& url) {
     return url.host() == source.host;
 }
 
-bool SourceAllowPort(const CSPSource& source, const GURL& url) {
+CSPSource::PortMatchingResult SourceAllowPort(
+    const CSPSource& source, const GURL& url) {

[arthursonzogni, 2017/04/07 09:20:27]

Nit: git cl format gives me:
CSPSource::PortMatchingResult SourceAllowPort(const CSPSource& source,
                                              const GURL& url) {

[andypaicu, 2017/04/07 11:34:24]

Done

   int url_port = url.EffectiveIntPort();
 
   if (source.is_port_wildcard)
-    return true;
+    return CSPSource::PortMatchingResult::MatchingWildcard;
 
-  if (source.port == url::PORT_UNSPECIFIED)
-    return DefaultPortForScheme(url.scheme()) == url_port;
+  if (source.port == url_port) {
+    if (source.port == url::PORT_UNSPECIFIED)
+      return CSPSource::PortMatchingResult::MatchingWildcard;
+    return CSPSource::PortMatchingResult::MatchingExact;
+  }
 
-  if (source.port == url_port)
-    return true;
+  if (source.port == url::PORT_UNSPECIFIED) {
+    if (DefaultPortForScheme(url.scheme()) == url_port) {
+      return CSPSource::PortMatchingResult::MatchingWildcard;
+    }
+    return CSPSource::PortMatchingResult::NotMatching;
+  }
 
-  if (source.port == 80 && url_port == 443)
-    return true;
+  int source_port = source.port;
+  if (source_port == url::PORT_UNSPECIFIED)
+    source_port = DefaultPortForScheme(source.scheme);
+
+  if (source_port == 80 && url_port == 443)
+    return CSPSource::PortMatchingResult::MatchingUpgrade;
 
-  return false;
+  return CSPSource::PortMatchingResult::NotMatching;
 }
 
 bool SourceAllowPath(const CSPSource& source,
@@ -93,6 +127,20 @@ bool SourceAllowPath(const CSPSource& source,
   return source.path == url_path;
 }
 
+bool inline requiresUpgrade(const CSPSource::PortMatchingResult result) {
+  return result == CSPSource::PortMatchingResult::MatchingUpgrade;
+}
+bool inline requiresUpgrade(const CSPSource::SchemeMatchingResult result) {
+  return result == CSPSource::SchemeMatchingResult::MatchingUpgrade;
+}
+bool inline canUpgrade(const CSPSource::PortMatchingResult result) {
+  return result == CSPSource::PortMatchingResult::MatchingUpgrade ||
+      result == CSPSource::PortMatchingResult::MatchingWildcard;

[arthursonzogni, 2017/04/07 09:20:27]

Nit: git cl format gives me:
bool inline canUpgrade(const CSPSource::PortMatchingResult result) {
  return result == CSPSource::PortMatchingResult::MatchingUpgrade ||
         result == CSPSource::PortMatchingResult::MatchingWildcard;
}

[andypaicu, 2017/04/07 11:34:24]

Done

+}
+bool inline canUpgrade(const CSPSource::SchemeMatchingResult result) {
+  return result == CSPSource::SchemeMatchingResult::MatchingUpgrade;
+}
+
 }  // namespace
 
 CSPSource::CSPSource()
@@ -129,10 +177,20 @@ bool CSPSource::Allow(const CSPSource& source,
                       CSPContext* context,
                       bool is_redirect) {
   if (source.IsSchemeOnly())
-    return SourceAllowScheme(source, url, context);
+    return SourceAllowScheme(source, url, context) !=
+        SchemeMatchingResult::NotMatching;

[arthursonzogni, 2017/04/07 09:20:27]

Nit: git cl format gives me:
return SourceAllowScheme(source, url, context) !=
       SchemeMatchingResult::NotMatching;

[andypaicu, 2017/04/07 11:34:24]

Done

+
+  PortMatchingResult portResult = SourceAllowPort(source, url);
+  SchemeMatchingResult schemeResult = SourceAllowScheme(source, url, context);
+
+  if ((requiresUpgrade(schemeResult) || (requiresUpgrade(portResult))) &&
+      (!canUpgrade(schemeResult) || !canUpgrade(portResult))) {
+    return false;
+  }

[arthursonzogni, 2017/04/07 09:20:27]

This looks clearer to me:

if (requiresUpgrade(schemeResult) && !canUpgrade(portResult)) return false;
if (requiresUpgrade(portResult) && !canUpgrade(schemeResult)) return false;

 
-  return SourceAllowScheme(source, url, context) &&
-         SourceAllowHost(source, url) && SourceAllowPort(source, url) &&
+  return schemeResult != SchemeMatchingResult::NotMatching &&
+         SourceAllowHost(source, url) &&
+         portResult != PortMatchingResult::NotMatching &&
          SourceAllowPath(source, url, is_redirect);
 }