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

content/common/content_security_policy/csp_source.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_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/common/content_security_policy/csp_context.h"
 #include "url/url_canon.h"
 #include "url/url_util.h"
 
 namespace content {
 
 namespace {
 
 bool DecodePath(const base::StringPiece& path, std::string* output) {
   url::RawCanonOutputT<base::char16> unescaped;
   url::DecodeURLEscapeSequences(path.data(), path.size(), &unescaped);
   return base::UTF16ToUTF8(unescaped.data(), unescaped.length(), output);
 }
 
 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) {
   if (source.is_host_wildcard) {
     if (source.host.empty())
       return true;
     // TODO(arthursonzogni): Chrome used to, incorrectly, match *.x.y to x.y.
     // The renderer version of this function count how many times it happens.
     // It might be useful to do it outside of blink too.
     // See third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
     return base::EndsWith(url.host(), '.' + source.host,
                           base::CompareCase::INSENSITIVE_ASCII);
   } else
     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);
 
-  return false;
+  if (source_port == 80 && url_port == 443)
+    return CSPSource::PortMatchingResult::MatchingUpgrade;
+
+  return CSPSource::PortMatchingResult::NotMatching;
 }
 
 bool SourceAllowPath(const CSPSource& source,
                      const GURL& url,
                      bool is_redirect) {
   if (is_redirect)
     return true;
 
   if (source.path.empty() || url.path().empty())
     return true;
 
   std::string url_path;
   if (!DecodePath(url.path(), &url_path)) {
     // TODO(arthursonzogni): try to figure out if that could happen and how to
     // handle it.
     return false;
   }
 
   // If the path represents a directory.
   if (base::EndsWith(source.path, "/", base::CompareCase::SENSITIVE))
     return base::StartsWith(url_path, source.path,
                             base::CompareCase::SENSITIVE);
 
   // The path represents a file.
   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()
     : scheme(),
       host(),
       is_host_wildcard(false),
       port(url::PORT_UNSPECIFIED),
       is_port_wildcard(false),
       path() {}
 
@@ skipping 16 matching lines @@
 
 CSPSource::CSPSource(const CSPSource& source) = default;
 CSPSource::~CSPSource() = default;
 
 // static
 bool CSPSource::Allow(const CSPSource& source,
                       const GURL& url,
                       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

 
-  return SourceAllowScheme(source, url, context) &&
-         SourceAllowHost(source, url) && SourceAllowPort(source, url) &&
+  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 schemeResult != SchemeMatchingResult::NotMatching &&
+         SourceAllowHost(source, url) &&
+         portResult != PortMatchingResult::NotMatching &&
          SourceAllowPath(source, url, is_redirect);
 }
 
 std::string CSPSource::ToString() const {
   // scheme
   if (IsSchemeOnly())
     return scheme + ":";
 
   std::stringstream text;
   if (!scheme.empty())
@@ skipping 31 matching lines @@
 
 bool CSPSource::HasHost() const {
   return !host.empty() || is_host_wildcard;
 }
 
 bool CSPSource::HasPath() const {
   return !path.empty();
 }
 
 }  // namespace content