Changeset 292266 in webkit

Timestamp:

Apr 2, 2022 9:22:20 AM (4 months ago)

Author:

Patrick Griffis

Message:

CSP: Improve compatibility of source matching
​https://bugs.webkit.org/show_bug.cgi?id=235873

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Update expectation as passing.

• web-platform-tests/content-security-policy/connect-src/connect-src-websocket-self.sub-expected.txt:

Source/WebCore:

• Improved handling of protocol changes:
  • For host and self sources direct upgrades are allowed (ws->wss) (http->https already worked).
  • For self sources side grades are now allowed (http->ws).
  • For self sources upgrades are always allowed (*->https, *->wss).

This is documented here: ​https://www.w3.org/TR/CSP3/#match-url-to-source-expression

I also included some minor cleanups and adding of comments.

• page/csp/ContentSecurityPolicy.cpp:

(WebCore::ContentSecurityPolicy::updateSourceSelf):
(WebCore::ContentSecurityPolicy::protocolMatchesSelf const): Deleted.

• page/csp/ContentSecurityPolicy.h:

(WebCore::ContentSecurityPolicy::selfProtocol const):

• page/csp/ContentSecurityPolicySource.cpp:

(WebCore::ContentSecurityPolicySource::ContentSecurityPolicySource):
(WebCore::ContentSecurityPolicySource::matches const):
(WebCore::ContentSecurityPolicySource::schemeMatches const):
(WebCore::ContentSecurityPolicySource::portMatches const):

• page/csp/ContentSecurityPolicySource.h:
• page/csp/ContentSecurityPolicySourceList.cpp:

(WebCore::ContentSecurityPolicySourceList::isProtocolAllowedByStar const):
(WebCore::ContentSecurityPolicySourceList::parse):

Location:

trunk

Files:

• LayoutTests/imported/w3c/ChangeLog (modified) (1 diff)
• LayoutTests/imported/w3c/web-platform-tests/content-security-policy/connect-src/connect-src-websocket-self.sub-expected.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/page/csp/ContentSecurityPolicy.cpp (modified) (2 diffs)
• Source/WebCore/page/csp/ContentSecurityPolicy.h (modified) (1 diff)
• Source/WebCore/page/csp/ContentSecurityPolicySource.cpp (modified) (5 diffs)
• Source/WebCore/page/csp/ContentSecurityPolicySource.h (modified) (2 diffs)
• Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp (modified) (2 diffs)

trunk/LayoutTests/imported/w3c/ChangeLog

@@ +1 @@
+2022-04-02  Patrick Griffis  <pgriffis@igalia.com>
+
+        CSP: Improve compatibility of source matching
+        https://bugs.webkit.org/show_bug.cgi?id=235873
+
+        Reviewed by Darin Adler.
+
+        Update expectation as passing.
+
+        * web-platform-tests/content-security-policy/connect-src/connect-src-websocket-self.sub-expected.txt:
+
 2022-04-01  Tim Nguyen  <ntim@apple.com>
 

trunk/LayoutTests/imported/w3c/web-platform-tests/content-security-policy/connect-src/connect-src-websocket-self.sub-expected.txt

@@ -1 +1 @@
 
-FAIL Expecting logs: ["allowed", "allowed"] assert_unreached: unexpected log: blocked Reached unreachable code
+PASS Expecting logs: ["allowed", "allowed"]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-04-02  Patrick Griffis  <pgriffis@igalia.com>
+
+        CSP: Improve compatibility of source matching
+        https://bugs.webkit.org/show_bug.cgi?id=235873
+
+        Reviewed by Darin Adler.
+
+        - Improved handling of protocol changes:
+          - For host and self sources direct upgrades are allowed (ws->wss) (http->https already worked).
+          - For self sources side grades are now allowed (http->ws).
+          - For self sources upgrades are always allowed (*->https, *->wss).          
+        This is documented here: https://www.w3.org/TR/CSP3/#match-url-to-source-expression
+
+        I also included some minor cleanups and adding of comments.
+
+        * page/csp/ContentSecurityPolicy.cpp:
+        (WebCore::ContentSecurityPolicy::updateSourceSelf):
+        (WebCore::ContentSecurityPolicy::protocolMatchesSelf const): Deleted.
+        * page/csp/ContentSecurityPolicy.h:
+        (WebCore::ContentSecurityPolicy::selfProtocol const):
+        * page/csp/ContentSecurityPolicySource.cpp:
+        (WebCore::ContentSecurityPolicySource::ContentSecurityPolicySource):
+        (WebCore::ContentSecurityPolicySource::matches const):
+        (WebCore::ContentSecurityPolicySource::schemeMatches const):
+        (WebCore::ContentSecurityPolicySource::portMatches const):
+        * page/csp/ContentSecurityPolicySource.h:
+        * page/csp/ContentSecurityPolicySourceList.cpp:
+        (WebCore::ContentSecurityPolicySourceList::isProtocolAllowedByStar const):
+        (WebCore::ContentSecurityPolicySourceList::parse):
+
 2022-04-02  Andres Gonzalez  <andresg_22@apple.com>
 

trunk/Source/WebCore/page/csp/ContentSecurityPolicy.cpp

@@ -240 +240 @@
 void ContentSecurityPolicy::updateSourceSelf(const SecurityOrigin& securityOrigin)
 {
-    m_selfSourceProtocol = securityOrigin.protocol();
-    m_selfSource = makeUnique<ContentSecurityPolicySource>(*this, m_selfSourceProtocol, securityOrigin.host(), securityOrigin.port(), emptyString(), false, false);
+    m_selfSourceProtocol = securityOrigin.protocol().convertToASCIILowercase();
+    m_selfSource = makeUnique<ContentSecurityPolicySource>(*this, m_selfSourceProtocol, securityOrigin.host(), securityOrigin.port(), emptyString(), false, false, IsSelfSource::Yes);
 }
 
@@ -294 +294 @@
         return downcast<Document>(*m_scriptExecutionContext).settings().allowContentSecurityPolicySourceStarToMatchAnyProtocol();
     return false;
-}
-
-bool ContentSecurityPolicy::protocolMatchesSelf(const URL& url) const
-{
-    if (equalLettersIgnoringASCIICase(m_selfSourceProtocol, "http"))
-        return url.protocolIsInHTTPFamily();
-    return equalIgnoringASCIICase(url.protocol(), m_selfSourceProtocol);
 }
 

trunk/Source/WebCore/page/csp/ContentSecurityPolicy.h

@@ -177 +177 @@
 
     // Used by ContentSecurityPolicySource
-    bool protocolMatchesSelf(const URL&) const;
+    const String& selfProtocol() const { return m_selfSourceProtocol; };
 
     void setUpgradeInsecureRequests(bool);

trunk/Source/WebCore/page/csp/ContentSecurityPolicySource.cpp

@@ -35 +35 @@
 namespace WebCore {
 
-ContentSecurityPolicySource::ContentSecurityPolicySource(const ContentSecurityPolicy& policy, const String& scheme, const String& host, std::optional<uint16_t> port, const String& path, bool hostHasWildcard, bool portHasWildcard)
+ContentSecurityPolicySource::ContentSecurityPolicySource(const ContentSecurityPolicy& policy, const String& scheme, const String& host, std::optional<uint16_t> port, const String& path, bool hostHasWildcard, bool portHasWildcard, IsSelfSource isSelfSource)
     : m_policy(policy)
     , m_scheme(scheme)
@@ -43 +43 @@
     , m_hostHasWildcard(hostHasWildcard)
     , m_portHasWildcard(portHasWildcard)
+    , m_isSelfSource(isSelfSource == IsSelfSource::Yes)
 {
 }
@@ -48 +49 @@
 bool ContentSecurityPolicySource::matches(const URL& url, bool didReceiveRedirectResponse) const
 {
+    // https://www.w3.org/TR/CSP3/#match-url-to-source-expression.
     if (!schemeMatches(url))
         return false;
@@ -57 +59 @@
 bool ContentSecurityPolicySource::schemeMatches(const URL& url) const
 {
-    if (m_scheme.isEmpty())
-        return m_policy.protocolMatchesSelf(url);
-    if (equalLettersIgnoringASCIICase(m_scheme, "http"))
-        return url.protocolIsInHTTPFamily();
-    return equalIgnoringASCIICase(url.protocol(), m_scheme);
+    // https://www.w3.org/TR/CSP3/#match-schemes.
+    const auto& scheme = m_scheme.isEmpty() ? m_policy.selfProtocol() : m_scheme;
+    auto urlScheme = url.protocol().convertToASCIILowercase();
+
+    if (scheme == urlScheme)
+        return true;
+
+    // host-sources can do direct-upgrades.
+    if (scheme == "http" && urlScheme == "https")
+        return true;
+    if (scheme == "ws" && (urlScheme == "wss" || urlScheme == "https" || urlScheme == "http"))
+        return true;
+    if (scheme == "wss" && urlScheme == "https")
+        return true;
+
+    // self-sources can always upgrade to secure protocols and side-grade insecure protocols.
+    if ((m_isSelfSource
+        && ((urlScheme == "https" || urlScheme == "wss") || (scheme == "http" && urlScheme == "ws"))))
+        return true;
+
+    return false;
 }
 
@@ -104 +122 @@
         return true;
 
-    if ((m_port && WTF::isDefaultPortForProtocol(m_port.value(), "http")) && ((!port && url.protocolIs("https")) || (port && WTF::isDefaultPortForProtocol(port.value(), "https"))))
+    // host-source and self-source allows upgrading to a more secure scheme which allows for different ports.
+    auto defaultSecurePort = WTF::defaultPortForProtocol("https").value_or(443);
+    auto defaultInsecurePort = WTF::defaultPortForProtocol("http").value_or(80);
+    bool isUpgradeSecure = (port == defaultSecurePort) || (!port && (url.protocol() == "https" || url.protocol() == "wss"));
+    bool isCurrentUpgradable = (m_port == defaultInsecurePort) || (m_scheme == "http" && (!m_port || m_port == defaultSecurePort));
+    if (isUpgradeSecure && isCurrentUpgradable)
         return true;
 

trunk/Source/WebCore/page/csp/ContentSecurityPolicySource.h

@@ -34 +34 @@
 struct SecurityOriginData;
 
+enum class IsSelfSource : bool { No, Yes };
+
 class ContentSecurityPolicySource {
     WTF_MAKE_FAST_ALLOCATED;
 public:
-    ContentSecurityPolicySource(const ContentSecurityPolicy&, const String& scheme, const String& host, std::optional<uint16_t> port, const String& path, bool hostHasWildcard, bool portHasWildcard);
+    ContentSecurityPolicySource(const ContentSecurityPolicy&, const String& scheme, const String& host, std::optional<uint16_t> port, const String& path, bool hostHasWildcard, bool portHasWildcard, IsSelfSource);
 
     bool matches(const URL&, bool didReceiveRedirectResponse = false) const;
@@ -58 +60 @@
     bool m_hostHasWildcard;
     bool m_portHasWildcard;
+    bool m_isSelfSource;
 };
 

trunk/Source/WebCore/page/csp/ContentSecurityPolicySourceList.cpp

@@ -121 +121 @@
         return true;
 
-    // Although not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match
+    // This is counter to the CSP3 spec which only allows HTTPS but Chromium also allows it.
+    bool isAllowed = url.protocolIsInHTTPFamily() || url.protocolIs("ws") || url.protocolIs("wss") || url.protocolIs(m_policy.selfProtocol());
+    // Also not allowed by the Content Security Policy Level 3 spec., we allow a data URL to match
     // "img-src *" and either a data URL or blob URL to match "media-src *" for web compatibility.
-    bool isAllowed = url.protocolIsInHTTPFamily() || url.protocolIs("ws") || url.protocolIs("wss") || m_policy.protocolMatchesSelf(url);
     if (equalIgnoringASCIICase(m_directiveName, ContentSecurityPolicyDirectiveNames::imgSrc))
         isAllowed |= url.protocolIsData();
@@ -270 +271 @@
                 m_policy.reportDirectiveAsSourceExpression(m_directiveName, source->host.value);
             if (isValidSourceForExtensionMode(source.value()))
-                m_list.append(ContentSecurityPolicySource(m_policy, source->scheme.toString(), source->host.value.toString(), source->port.value, source->path, source->host.hasWildcard, source->port.hasWildcard));
+                m_list.append(ContentSecurityPolicySource(m_policy, source->scheme.convertToASCIILowercase(), source->host.value.toString(), source->port.value, source->path, source->host.hasWildcard, source->port.hasWildcard, IsSelfSource::No));
         } else
             m_policy.reportInvalidSourceExpression(m_directiveName, String(beginSource, buffer.position() - beginSource));