Stop CSP from matching independent scheme/port upgrades

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

Index: third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
diff --git a/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp b/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
index 3b5ed5cce1c5cc84c004ccdbf3283fdcdd77ed51..edd48a6246771a106978888ef9c819704d87d7e5 100644
--- a/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
+++ b/third_party/WebKit/Source/core/frame/csp/CSPSource.cpp
@@ -39,7 +39,7 @@ bool CSPSource::matches(const KURL& url,
   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 {
@@ -112,6 +112,29 @@ bool CSPSource::portMatches(int port, const String& protocol) const {
   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;