CSP: Distinguish port 0 from a default or empty port

The CSP data structures in Blink currently use a value of 0 to denote an
unspecified or default-valued port in CSP source expressions. However,
origins can also have ports explicitly set to 0.

In order to resolve behavior inconsistencies between
blink::SecurityOrigin, which currently doesn't support port 0, and
url::Origin, which does, we're updating blink::SecurityOrigin to
distinguish port-0 origins from origins with unspecified
(default-valued) ports. This makes SourceListDirectiveTest.GetSources
CSP tests fail, because it means that a "self" CSPSource created by
ContentSecurityPolicy::SetupSelf from an origin with the default port
will no longer have the same port as a CSPSource created by parsing a
host-source source expression with no port.

To fix this, we update CSPSource's port_ member to have a new default
state denoting an unspecified or default-valued port, distinct from the
value 0 which now specifically represents port 0.

Bug: 1136678
Change-Id: Ic386fc2ba31e13c95676ecf050e24874d4af132e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2477044
Commit-Queue: David Van Cleve <davidvc@chromium.org>
Reviewed-by: Andy Paicu <andypaicu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#823558}

third_party/blink/renderer/core/frame/csp/content_security_policy.cc

@@ -180,7 +180,17 @@ void ContentSecurityPolicy::SetupSelf(const SecurityOrigin& security_origin) {
   // Ensure that 'self' processes correctly.
   self_protocol_ = security_origin.Protocol();
   self_source_ = MakeGarbageCollected<CSPSource>(
-      this, self_protocol_, security_origin.Host(), security_origin.Port(),
+      this, self_protocol_, security_origin.Host(),
+      // CSPSource uses port CSPSource::kPortUnspecified to represent a
+      // missing port and reserves port 0 specifically for origins with port set
+      // to 0; SecurityOrigin uses port 0 for origins with port 0 as well as for
+      // origins without ports.
+      //
+      // TODO(crbug.com/1136678): Once SecurityOrigin starts treating port 0 as
+      // a specifically set port, rather than as a sentinel for an
+      // omitted or default-valued port, modify this logic.
+      security_origin.Port() == 0 ? CSPSource::kPortUnspecified
+                                  : security_origin.Port(),
       String(), CSPSource::kNoWildcard, CSPSource::kNoWildcard);
 }
 

third_party/blink/renderer/core/frame/csp/csp_source.cc

@@ -15,6 +15,8 @@
 
 namespace blink {
 
+constexpr int CSPSource::kPortUnspecified = -1;
+
 CSPSource::CSPSource(ContentSecurityPolicy* policy,
                      const String& scheme,
                      const String& host,
@@ -48,7 +50,8 @@ bool CSPSource::Matches(const KURL& url,
     return true;
   bool paths_match = (redirect_status == RedirectStatus::kFollowedRedirect) ||
                      PathMatches(url.GetPath());
-  PortMatchingResult ports_match = PortMatches(url.Port(), url.Protocol());
+  PortMatchingResult ports_match = PortMatches(
+      url.HasPort() ? url.Port() : kPortUnspecified, url.Protocol());
 
   // if either the scheme or the port would require an upgrade (e.g. from http
   // to https) then check that both of them can upgrade to ensure that we don't
@@ -68,7 +71,8 @@ bool CSPSource::MatchesAsSelf(const KURL& url) {
   // Step 4.
   SchemeMatchingResult schemes_match = SchemeMatches(url.Protocol());
   bool hosts_match = HostMatches(url.Host());
-  PortMatchingResult ports_match = PortMatches(url.Port(), url.Protocol());
+  PortMatchingResult ports_match = PortMatches(
+      url.HasPort() ? url.Port() : kPortUnspecified, url.Protocol());
 
   // check if the origin is exactly matching
   if (schemes_match == SchemeMatchingResult::kMatchingExact && hosts_match &&
@@ -82,9 +86,10 @@ bool CSPSource::MatchesAsSelf(const KURL& url) {
 
   bool ports_match_or_defaults =
       (ports_match == PortMatchingResult::kMatchingExact ||
-       ((IsDefaultPortForProtocol(port_, self_scheme) || port_ == 0) &&
-        (IsDefaultPortForProtocol(url.Port(), url.Protocol()) ||
-         url.Port() == 0)));
+       ((IsDefaultPortForProtocol(port_, self_scheme) ||
+         port_ == kPortUnspecified) &&
+        (!url.HasPort() ||
+         IsDefaultPortForProtocol(url.Port(), url.Protocol()))));
 
   if (hosts_match && ports_match_or_defaults &&
       (url.Protocol() == "https" || url.Protocol() == "wss" ||
@@ -159,7 +164,7 @@ CSPSource::PortMatchingResult CSPSource::PortMatches(
     return PortMatchingResult::kMatchingWildcard;
 
   if (port == port_) {
-    if (port == 0)
+    if (port == kPortUnspecified)
       return PortMatchingResult::kMatchingWildcard;
     return PortMatchingResult::kMatchingExact;
   }
@@ -168,18 +173,21 @@ CSPSource::PortMatchingResult CSPSource::PortMatches(
   is_scheme_http = scheme_.IsEmpty() ? policy_->ProtocolEqualsSelf("http")
                                      : EqualIgnoringASCIICase("http", scheme_);
 
-  if ((port_ == 80 || ((port_ == 0 || port_ == 443) && is_scheme_http)) &&
-      (port == 443 || (port == 0 && DefaultPortForProtocol(protocol) == 443)))
+  if ((port_ == 80 ||
+       ((port_ == kPortUnspecified || port_ == 443) && is_scheme_http)) &&
+      (port == 443 ||
+       (port == kPortUnspecified && DefaultPortForProtocol(protocol) == 443))) {
     return PortMatchingResult::kMatchingUpgrade;
+  }
 
-  if (!port) {
+  if (port == kPortUnspecified) {
     if (IsDefaultPortForProtocol(port_, protocol))
       return PortMatchingResult::kMatchingExact;
 
     return PortMatchingResult::kNotMatching;
   }
 
-  if (!port_) {
+  if (port_ == kPortUnspecified) {
     if (IsDefaultPortForProtocol(port, protocol))
       return PortMatchingResult::kMatchingExact;
 
@@ -261,7 +269,8 @@ CSPSource* CSPSource::Intersect(CSPSource* other) const {
   // Choose this port if the other port is empty, has wildcard or is a port for
   // a less secure scheme such as "http" whereas scheme of this is "https", in
   // which case the lengths would differ.
-  int port = (other->port_wildcard_ == kHasWildcard || !other->port_ ||
+  int port = (other->port_wildcard_ == kHasWildcard ||
+              other->port_ == kPortUnspecified ||
               scheme_.length() > other->scheme_.length())
                  ? port_
                  : other->port_;
@@ -302,12 +311,13 @@ bool CSPSource::FirstSubsumesSecond(
 network::mojom::blink::CSPSourcePtr CSPSource::ExposeForNavigationalChecks()
     const {
   return network::mojom::blink::CSPSource::New(
-      scheme_ ? scheme_ : "",                               // scheme
-      host_ ? host_ : "",                                   // host
-      port_ == 0 ? -1 /* url::PORT_UNSPECIFIED */ : port_,  // port
-      path_ ? path_ : "",                                   // path
-      host_wildcard_ == kHasWildcard,                       // is_host_wildcard
-      port_wildcard_ == kHasWildcard                        // is_port_wildcard
+      scheme_ ? scheme_ : "",             // scheme
+      host_ ? host_ : "",                 // host
+      port_ == kPortUnspecified ? -1      /* url::PORT_UNSPECIFIED */
+                                : port_,  // port
+      path_ ? path_ : "",                 // path
+      host_wildcard_ == kHasWildcard,     // is_host_wildcard
+      port_wildcard_ == kHasWildcard      // is_port_wildcard
   );
 }
 

third_party/blink/renderer/core/frame/csp/csp_source.h

@@ -20,6 +20,9 @@ class KURL;
 
 class CORE_EXPORT CSPSource final : public GarbageCollected<CSPSource> {
  public:
+  // Represents the absence of a port.
+  const static int kPortUnspecified;
+
   enum WildcardDisposition { kNoWildcard, kHasWildcard };
 
   // NotMatching is the only negative member, the rest are different types of

third_party/blink/renderer/core/frame/csp/source_list_directive.cc

@@ -161,7 +161,7 @@ void SourceListDirective::Parse(const UChar* begin, const UChar* end) {
     SkipWhile<UChar, IsSourceCharacter>(position, end);
 
     String scheme, host, path;
-    int port = 0;
+    int port = CSPSource::kPortUnspecified;
     CSPSource::WildcardDisposition host_wildcard = CSPSource::kNoWildcard;
     CSPSource::WildcardDisposition port_wildcard = CSPSource::kNoWildcard;
 
@@ -339,7 +339,7 @@ bool SourceListDirective::ParseSource(
     if (!ParsePort(begin_port, begin_path, port, port_wildcard))
       return false;
   } else {
-    *port = 0;
+    *port = CSPSource::kPortUnspecified;
   }
 
   if (begin_path != end) {
@@ -559,7 +559,7 @@ bool SourceListDirective::ParsePort(
     int* port,
     CSPSource::WildcardDisposition* port_wildcard) {
   DCHECK(begin <= end);
-  DCHECK_EQ(*port, 0);
+  DCHECK_EQ(*port, CSPSource::kPortUnspecified);
   DCHECK(*port_wildcard == CSPSource::kNoWildcard);
 
   if (!SkipExactly<UChar>(begin, end, ':'))
@@ -569,7 +569,7 @@ bool SourceListDirective::ParsePort(
     return false;
 
   if (end - begin == 1 && *begin == '*') {
-    *port = 0;
+    *port = CSPSource::kPortUnspecified;
     *port_wildcard = CSPSource::kHasWildcard;
     return true;
   }
@@ -672,18 +672,18 @@ HeapVector<Member<CSPSource>> SourceListDirective::GetSources(
   HeapVector<Member<CSPSource>> sources = list_;
   if (allow_star_) {
     sources.push_back(MakeGarbageCollected<CSPSource>(
-        policy_, "ftp", String(), 0, String(), CSPSource::kNoWildcard,
-        CSPSource::kNoWildcard));
+        policy_, "ftp", String(), CSPSource::kPortUnspecified, String(),
+        CSPSource::kNoWildcard, CSPSource::kNoWildcard));
     sources.push_back(MakeGarbageCollected<CSPSource>(
-        policy_, "ws", String(), 0, String(), CSPSource::kNoWildcard,
-        CSPSource::kNoWildcard));
+        policy_, "ws", String(), CSPSource::kPortUnspecified, String(),
+        CSPSource::kNoWildcard, CSPSource::kNoWildcard));
     sources.push_back(MakeGarbageCollected<CSPSource>(
-        policy_, "http", String(), 0, String(), CSPSource::kNoWildcard,
-        CSPSource::kNoWildcard));
+        policy_, "http", String(), CSPSource::kPortUnspecified, String(),
+        CSPSource::kNoWildcard, CSPSource::kNoWildcard));
     if (self) {
       sources.push_back(MakeGarbageCollected<CSPSource>(
-          policy_, self->GetScheme(), String(), 0, String(),
-          CSPSource::kNoWildcard, CSPSource::kNoWildcard));
+          policy_, self->GetScheme(), String(), CSPSource::kPortUnspecified,
+          String(), CSPSource::kNoWildcard, CSPSource::kNoWildcard));
     }
   } else if (allow_self_ && self) {
     sources.push_back(self);