Stop CSP from matching independent scheme/port upgrades

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

 // Copyright 2014 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 "core/frame/csp/CSPSource.h"
 
 #include "core/frame/UseCounter.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "platform/weborigin/KURL.h"
 #include "platform/weborigin/KnownPorts.h"
@@ skipping 21 matching lines @@
                         ResourceRequest::RedirectStatus redirectStatus) const {
   bool schemesMatch = m_scheme.isEmpty() ? m_policy->protocolMatchesSelf(url)
                                          : schemeMatches(url.protocol());
   if (!schemesMatch)
     return false;
   if (isSchemeOnly())
     return true;
   bool pathsMatch = (redirectStatus == RedirectStatus::FollowedRedirect) ||
                     pathMatches(url.path());
   return hostMatches(url.host()) && portMatches(url.port(), url.protocol()) &&
-         pathsMatch;
+         pathsMatch && portAndSchemeUpgradeAllowed(url.port(), url.protocol());
 }
 
 bool CSPSource::schemeMatches(const String& protocol) const {
   DCHECK_EQ(protocol, protocol.lower());
   if (m_scheme == "http")
     return protocol == "http" || protocol == "https";
   if (m_scheme == "ws")
     return protocol == "ws" || protocol == "wss";
   return protocol == m_scheme;
 }
@@ skipping 52 matching lines @@
 
   if (!port)
     return isDefaultPortForProtocol(m_port, protocol);
 
   if (!m_port)
     return isDefaultPortForProtocol(port, protocol);
 
   return false;
 }
 
+bool CSPSource::portAndSchemeUpgradeAllowed(int port,
+                                            const String& protocol) const {
+  bool isPortUpgrade = false;
+  bool isSchemeUpgrade = false;
+  bool isSchemeHttp = false;
+
+  if (m_scheme.isEmpty())
+    isSchemeHttp = m_policy->protocolIsEqual("http");

[Mike West, 2017/02/21 14:24:04]

This will return true if the page's scheme is `http`, right? Why is that
relevant?

[andypaicu, 2017/02/21 15:51:44]

I believe that a fallback mechanism is necessary if the scheme is not present to
have something to rely on.
For instance the test at line 234 fails if we don't have this in place.

[Mike West, 2017/02/22 12:03:40]

Ok, so in the case that the source doesn't specify a scheme, you're falling back
to the page's scheme. That makes sense, thanks. The naming here is weird, then.
"protocolIsEqual" doesn't tell me that I'm comparing it to the protocol of the
page to which the policy is applying.

+  else
+    isSchemeHttp = equalIgnoringCase("http", m_scheme);
+
+  if ((m_port == 80 || (m_port == 0 && isSchemeHttp)) &&
+      (port == 443 || (port == 0 && defaultPortForProtocol(protocol) == 443)))
+    isPortUpgrade = true;

[Mike West, 2017/02/21 14:24:04]

1) Nit: You need braces if the `if` clause is multi-line. I know there are lots
of places in Blink that don't do this, because we used to have very long lines
and an automated tool wrapped them, but didn't add braces. C'est la vie.

2) A similar check is in `::portMatches`. Can we remove it?

[andypaicu, 2017/02/21 15:51:44]

::portMatches is used in a lot of places and the check is similar but not quite
identical.

[Mike West, 2017/02/22 12:03:40]

Then I think we should figure out how to make it identical. :) The current patch
seems confusing.

+
+  isSchemeUpgrade = isSchemeHttp && equalIgnoringCase("https", protocol);
+
+  if (isPortUpgrade || isSchemeUpgrade)
+    return isPortUpgrade && isSchemeUpgrade;
+
+  return true;
+}
+
 bool CSPSource::subsumes(CSPSource* other) const {
   if (!schemeMatches(other->m_scheme))
     return false;
 
   if (other->isSchemeOnly() || isSchemeOnly())
     return isSchemeOnly();
 
   if ((m_hostWildcard == NoWildcard && other->m_hostWildcard == HasWildcard) ||
       (m_portWildcard == NoWildcard && other->m_portWildcard == HasWildcard)) {
     return false;
@@ skipping 78 matching lines @@
       return false;
   }
   return true;
 }
 
 DEFINE_TRACE(CSPSource) {
   visitor->trace(m_policy);
 }
 
 }  // namespace blink