CSP: Port upgrade matching should only apply to insecure source schemes

https://bugs.webkit.org/show_bug.cgi?id=308747
rdar://171265255

Reviewed by Brent Fulgham.

A CSP source expression such as "frame-src https://host:80" incorrectly
matches https://host (port 443). The mismatch (80 ≠ 443) should cause
a block, but WebKit has scheme upgrade logic that treats default ports
as equivalent during http-to-https transitions. This logic does not
verify the source scheme is insecure, so it treats any source with
port 80 as upgradable — even when the source scheme is already HTTPS.

Have schemeMatches() return a SchemeMatchResult enum that distinguishes
exact matches from insecure-to-secure upgrades, and pass this to
portMatches() so the upgrade path only fires when a scheme upgrade is
actually occurring. This also fixes an edge case with schemeless source
expressions. Both functions are annotated with CSP3 spec step references.

* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number-expected.txt:
* LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html:
* LayoutTests/platform/wk2/TestExpectations:
* Source/WebCore/page/csp/ContentSecurityPolicySource.cpp:
(WebCore::ContentSecurityPolicySource::portMatches const):

Canonical link: https://commits.webkit.org/311148@main

Author: roberto-apple

LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number-expected.txt

@@ -1,3 +1,5 @@
+CONSOLE MESSAGE: Refused to load https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js because it does not appear in the script-src directive of the Content Security Policy.
+CONSOLE MESSAGE: Refused to load https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js because it does not appear in the script-src directive of the Content Security Policy.
 Tests script-src source expression matching with implicit and explicit default port numbers.
 
 
@@ -26,3 +28,28 @@ PASS
 Frame: '<!--frame5-->'
 --------
 PASS
+
+--------
+Frame: '<!--frame6-->'
+--------
+PASS
+
+--------
+Frame: '<!--frame7-->'
+--------
+PASS
+
+--------
+Frame: '<!--frame8-->'
+--------
+PASS
+
+--------
+Frame: '<!--frame9-->'
+--------
+PASS
+
+--------
+Frame: '<!--frame10-->'
+--------
+PASS

LayoutTests/http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html

@@ -19,6 +19,21 @@
 
     // Tests that HTTPS URL with explicit default port number matches 'self'.
     ["yes", "script-src 'self'", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
+
+    // Tests that HTTPS URL does not match HTTPS source expression with opposite scheme's default port number.
+    ["no", "script-src https://127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
+
+    // Tests that HTTPS URL matches HTTP source expression with HTTPS default port number (upgrade with matching port).
+    ["yes", "script-src http://127.0.0.1:8443", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
+
+    // Tests that HTTPS URL matches WS source expression with HTTP default port (ws scheme upgrade with port upgrade).
+    ["yes", "script-src ws://127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
+
+    // Tests that HTTPS URL matches schemeless source expression with HTTP default port on HTTP page.
+    ["yes", "script-src 127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
+
+    // Tests that HTTPS URL does not match WSS source expression with HTTP default port (secure scheme, no port upgrade).
+    ["no", "script-src wss://127.0.0.1:8000", "https://127.0.0.1:8443/security/contentSecurityPolicy/resources/script.js"],
 ];
 </script>
 </head>

LayoutTests/platform/wk2/TestExpectations

@@ -420,7 +420,6 @@ fast/preloader/document-write-2.html [ Pass Failure ]
 
 # Internals.registerDefaultPortForProtocol() does not affect NetworkProcess. We should
 # look to remove it and write these test to make use of an HTTP server running on port 80.
-http/tests/security/contentSecurityPolicy/script-src-parsing-implicit-and-explicit-port-number.html
 http/tests/security/http-0.9/default-port-script-blocked.html
 http/tests/security/http-0.9/image-default-port-allowed.html
 http/tests/security/http-0.9/image-on-HTTP-0.9-default-port-page-allowed-ref-test.html

Source/WebCore/page/csp/ContentSecurityPolicySource.cpp

@@ -52,36 +52,54 @@ ContentSecurityPolicySource::ContentSecurityPolicySource(const ContentSecurityPo
 bool ContentSecurityPolicySource::matches(const URL& url, bool didReceiveRedirectResponse) const
 {
     // https://www.w3.org/TR/CSP3/#match-url-to-source-expression.
-    if (!schemeMatches(url))
+    auto schemeMatch = schemeMatches(url);
+    if (schemeMatch == SchemeMatchResult::NoMatch)
         return false;
     if (isSchemeOnly())
         return true;
-    return hostMatches(url) && portMatches(url) && (didReceiveRedirectResponse || pathMatches(url));
+    auto shouldUpgradePorts = (schemeMatch == SchemeMatchResult::InsecureUpgradeMatch) ? ShouldUpgradePorts::Yes : ShouldUpgradePorts::No;
+    return hostMatches(url) && portMatches(url, shouldUpgradePorts) && (didReceiveRedirectResponse || pathMatches(url));
 }
 
-bool ContentSecurityPolicySource::schemeMatches(const URL& url) const
+SchemeMatchResult ContentSecurityPolicySource::schemeMatches(const URL& url) const
 {
     // https://www.w3.org/TR/CSP3/#match-schemes.
-    auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme;
+    const auto& scheme = m_scheme.isEmpty() ? m_policy->selfProtocol() : m_scheme;
     auto urlScheme = url.protocol();
 
+    // Step 1.1: A matches B.
     if (scheme == urlScheme)
-        return true;
+        return SchemeMatchResult::Match;
 
-    // host-sources can do direct-upgrades.
+    // Step 1.2: A is "http" and B is "https".
     if (scheme == "http"_s && urlScheme == "https"_s)
-        return true;
-    if (scheme == "ws"_s && (urlScheme == "wss"_s || urlScheme == "https"_s || urlScheme == "http"_s))
-        return true;
-    if (scheme == "wss"_s && urlScheme == "https"_s)
-        return true;
+        return SchemeMatchResult::InsecureUpgradeMatch;
+
+    // Step 1.3: A is "ws" and B is "wss", "http", or "https".
+    if (scheme == "ws"_s) {
+        if (urlScheme == "wss"_s || urlScheme == "https"_s)
+            return SchemeMatchResult::InsecureUpgradeMatch;
+        if (urlScheme == "http"_s)
+            return SchemeMatchResult::Match;
+    }
 
-    // self-sources can always upgrade to secure protocols and side-grade insecure protocols.
-    if ((m_isSelfSource
-        && ((urlScheme == "https"_s || urlScheme == "wss"_s) || (scheme == "http"_s && urlScheme == "ws"_s))))
-        return true;
+    // Step 1.4: A is "wss" and B is "https".
+    if (scheme == "wss"_s && urlScheme == "https"_s)
+        return SchemeMatchResult::Match;
+
+    // self-sources can always upgrade to secure protocols and side-grade insecure protocols. (§6.7.2.8 step 4 NOTE)
+    if (m_isSelfSource) {
+        if (urlScheme == "https"_s || urlScheme == "wss"_s) {
+            if (scheme == "http"_s || scheme == "ws"_s)
+                return SchemeMatchResult::InsecureUpgradeMatch;
+            return SchemeMatchResult::Match;
+        }
+        if (scheme == "http"_s && urlScheme == "ws"_s)
+            return SchemeMatchResult::Match;
+    }
 
-    return false;
+    // Step 2: return "Does Not Match".
+    return SchemeMatchResult::NoMatch;
 }
 
 static bool NODELETE wildcardMatches(StringView host, const String& hostWithWildcard)
@@ -117,30 +135,44 @@ bool ContentSecurityPolicySource::pathMatches(const URL& url) const
     return path == m_path;
 }
 
-bool ContentSecurityPolicySource::portMatches(const URL& url) const
+bool ContentSecurityPolicySource::portMatches(const URL& url, ShouldUpgradePorts shouldUpgradePorts) const
 {
+    // https://www.w3.org/TR/CSP3/#match-ports
+    // input is the source expression's port-part (m_port).
+    // url is the URL being checked against the policy.
+
+    // Step 1: Assert input is null, "*", or a sequence of one or more ASCII digits.
+    ASSERT(!(m_portHasWildcard && m_port));
+
+    // Step 2: wildcard port matches any URL.
     if (m_portHasWildcard)
         return true;
 
-    std::optional<uint16_t> port = url.port();
+    std::optional<uint16_t> urlPort = url.port();
 
-    if (port == m_port)
+    // Steps 3-4: if normalizedInput equals url's port (including null), return "Matches".
+    if (urlPort == m_port)
         return true;
 
-    // host-source and self-source allows upgrading to a more secure scheme which allows for different ports.
-    auto defaultSecurePort = WTF::defaultPortForProtocol("https"_s).value_or(443);
-    auto defaultInsecurePort = WTF::defaultPortForProtocol("http"_s).value_or(80);
-    bool isUpgradeSecure = (port == defaultSecurePort) || (!port && (url.protocol() == "https"_s || url.protocol() == "wss"_s));
-    bool isCurrentUpgradable = (m_port == defaultInsecurePort) || (m_scheme == "http"_s && (!m_port || m_port == defaultSecurePort));
-    if (isUpgradeSecure && isCurrentUpgradable)
-        return true;
+    // When upgrading from an insecure to secure scheme, treat default
+    // ports as equivalent since they differ across schemes (80 vs 443).
+    if (shouldUpgradePorts == ShouldUpgradePorts::Yes) {
+        auto defaultSecurePort = WTF::defaultPortForProtocol("https"_s).value_or(443);
+        auto defaultInsecurePort = WTF::defaultPortForProtocol("http"_s).value_or(80);
+        bool urlOnSecureDefaultPort = (urlPort == defaultSecurePort) || (!urlPort && (url.protocol() == "https"_s || url.protocol() == "wss"_s));
+        bool sourcePortIsUpgradable = !m_port || m_port == defaultInsecurePort || m_port == defaultSecurePort;
+        if (urlOnSecureDefaultPort && sourcePortIsUpgradable)
+            return true;
+    }
 
-    if (!port)
+    // Step 5: if url's port is null, check if source port is the default for url's scheme.
+    if (!urlPort)
         return WTF::isDefaultPortForProtocol(m_port.value(), url.protocol());
 
     if (!m_port)
-        return WTF::isDefaultPortForProtocol(port.value(), url.protocol());
+        return WTF::isDefaultPortForProtocol(urlPort.value(), url.protocol());
 
+    // Step 6: return "Does Not Match".
     return false;
 }
 

Source/WebCore/page/csp/ContentSecurityPolicySource.h

@@ -36,6 +36,8 @@ class ContentSecurityPolicy;
 class SecurityOriginData;
 
 enum class IsSelfSource : bool { No, Yes };
+enum class SchemeMatchResult : uint8_t { NoMatch, Match, InsecureUpgradeMatch };
+enum class ShouldUpgradePorts : bool { No, Yes };
 
 class ContentSecurityPolicySource {
     WTF_MAKE_TZONE_ALLOCATED(ContentSecurityPolicySource);
@@ -47,10 +49,10 @@ class ContentSecurityPolicySource {
     operator SecurityOriginData() const;
 
 private:
-    bool schemeMatches(const URL&) const;
+    SchemeMatchResult schemeMatches(const URL&) const;
     bool NODELETE hostMatches(const URL&) const;
     bool pathMatches(const URL&) const;
-    bool portMatches(const URL&) const;
+    bool portMatches(const URL&, ShouldUpgradePorts) const;
     bool NODELETE isSchemeOnly() const;
 
     const CheckedRef<const ContentSecurityPolicy> m_policy;