Introduce `SecurityOrigin::IsSameOriginDomainWith()`.

This patch introduces a "same origin-domain" check, distinct from
`SecurityOrigin::CanAccess()`, and uses it to implement the latter. This
is a small step towards auditing the points at which we're calling
`::CanAccess()` to decide whether we ought to be performing a "same
origin", or "same origin-domain" check instead.

Bug: 1027191
Change-Id: If6007367da7e044d008f056e3f816ac422551e7c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1951001
Commit-Queue: Mike West <mkwst@chromium.org>
Reviewed-by: Dave Tapuska <dtapuska@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722439}

third_party/blink/renderer/platform/weborigin/security_origin.cc

@@ -327,58 +327,15 @@ bool SecurityOrigin::CanAccess(const SecurityOrigin* other,
     return true;
   }
 
-  // This is needed to ensure an origin can access to itself under nullified
-  // document.domain.
-  // TODO(tzik): Update the nulled domain handling and remove this condition.
-  if (this == other) {
-    detail = AccessResultDomainDetail::kDomainNotRelevant;
-    return true;
-  }
-
+  // TODO(1027191): We need to exit early in this case, as it looks like we're
+  // not correctly persisting the `agent_cluster_id_` when swapping from a
+  // remote to local frame (see `WebFrameTest.NavigateRemoteToLocalWithOpener`).
   if (IsOpaque() || other->IsOpaque()) {
     detail = AccessResultDomainDetail::kDomainNotRelevant;
     return nonce_if_opaque_ == other->nonce_if_opaque_;
   }
 
-  // document.domain handling, as per
-  // https://html.spec.whatwg.org/C/#dom-document-domain:
-  //
-  // 1) Neither document has set document.domain. In this case, we insist
-  //    that the scheme, host, and port of the URLs match.
-  //
-  // 2) Both documents have set document.domain. In this case, we insist
-  //    that the documents have set document.domain to the same value and
-  //    that the scheme of the URLs match. Ports do not need to match.
-  bool can_access = false;
-  if (protocol_ == other->protocol_) {
-    if (!domain_was_set_in_dom_ && !other->domain_was_set_in_dom_) {
-      detail = AccessResultDomainDetail::kDomainNotSet;
-      if (host_ == other->host_ && port_ == other->port_)
-        can_access = true;
-    } else if (domain_was_set_in_dom_ && other->domain_was_set_in_dom_) {
-      if (domain_ == other->domain_) {
-        can_access = true;
-        detail = (host_ == other->host_ && port_ == other->port_)
-                     ? AccessResultDomainDetail::kDomainMatchUnnecessary
-                     : AccessResultDomainDetail::kDomainMatchNecessary;
-      } else {
-        detail = (host_ == other->host_ && port_ == other->port_)
-                     ? AccessResultDomainDetail::kDomainMismatch
-                     : AccessResultDomainDetail::kDomainNotRelevant;
-      }
-    } else {
-      detail = (host_ == other->host_ && port_ == other->port_)
-                   ? AccessResultDomainDetail::kDomainSetByOnlyOneOrigin
-                   : AccessResultDomainDetail::kDomainNotRelevant;
-    }
-  } else {
-    detail = AccessResultDomainDetail::kDomainNotRelevant;
-  }
-
-  if (can_access && IsLocal() && !PassesFileCheck(other)) {
-    detail = AccessResultDomainDetail::kDomainNotRelevant;
-    can_access = false;
-  }
+  bool can_access = IsSameOriginDomainWith(other, detail);
 
   // Compare that the clusters are the same.
   if (can_access && !cross_agent_cluster_access_ &&
@@ -629,6 +586,65 @@ bool SecurityOrigin::AreSameOrigin(const KURL& a, const KURL& b) {
   return origin_b->IsSameOriginWith(origin_a.get());
 }
 
+bool SecurityOrigin::IsSameOriginDomainWith(
+    const SecurityOrigin* other,
+    AccessResultDomainDetail& detail) const {
+  // This is needed to ensure an origin can access to itself under nullified
+  // document.domain.
+  // TODO(tzik): Update the nulled domain handling and remove this condition.
+  if (this == other) {
+    detail = AccessResultDomainDetail::kDomainNotRelevant;
+    return true;
+  }
+
+  if (IsOpaque() || other->IsOpaque()) {
+    detail = AccessResultDomainDetail::kDomainNotRelevant;
+    return nonce_if_opaque_ == other->nonce_if_opaque_;
+  }
+
+  // document.domain handling, as per
+  // https://html.spec.whatwg.org/C/#dom-document-domain:
+  //
+  // 1) Neither document has set document.domain. In this case, we insist
+  //    that the scheme, host, and port of the URLs match.
+  //
+  // 2) Both documents have set document.domain. In this case, we insist
+  //    that the documents have set document.domain to the same value and
+  //    that the scheme of the URLs match. Ports do not need to match.
+  bool can_access = false;
+  if (protocol_ == other->protocol_) {
+    if (!domain_was_set_in_dom_ && !other->domain_was_set_in_dom_) {
+      detail = AccessResultDomainDetail::kDomainNotSet;
+      if (host_ == other->host_ && port_ == other->port_)
+        can_access = true;
+    } else if (domain_was_set_in_dom_ && other->domain_was_set_in_dom_) {
+      if (domain_ == other->domain_) {
+        can_access = true;
+        detail = (host_ == other->host_ && port_ == other->port_)
+                     ? AccessResultDomainDetail::kDomainMatchUnnecessary
+                     : AccessResultDomainDetail::kDomainMatchNecessary;
+      } else {
+        detail = (host_ == other->host_ && port_ == other->port_)
+                     ? AccessResultDomainDetail::kDomainMismatch
+                     : AccessResultDomainDetail::kDomainNotRelevant;
+      }
+    } else {
+      detail = (host_ == other->host_ && port_ == other->port_)
+                   ? AccessResultDomainDetail::kDomainSetByOnlyOneOrigin
+                   : AccessResultDomainDetail::kDomainNotRelevant;
+    }
+  } else {
+    detail = AccessResultDomainDetail::kDomainNotRelevant;
+  }
+
+  if (can_access && IsLocal() && !PassesFileCheck(other)) {
+    detail = AccessResultDomainDetail::kDomainNotRelevant;
+    can_access = false;
+  }
+
+  return can_access;
+}
+
 const KURL& SecurityOrigin::UrlWithUniqueOpaqueOrigin() {
   DCHECK(IsMainThread());
   DEFINE_STATIC_LOCAL(const KURL, url, ("data:,"));

third_party/blink/renderer/platform/weborigin/security_origin.h

@@ -139,19 +139,22 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   static bool IsSecure(const KURL&);
 
   // Returns true if this SecurityOrigin can script objects in the given
-  // SecurityOrigin. For example, call this function before allowing
-  // script from one security origin to read or write objects from
-  // another SecurityOrigin.
+  // SecurityOrigin. This check is similar to `IsSameOriginDomainWith()`, but
+  // additionally takes "universal access" flag into account, as well as the
+  // origin's agent cluster (see https://tc39.es/ecma262/#sec-agent-clusters).
+  //
+  // Note: This kind of access check should be rare; `IsSameOriginWith()` is
+  // almost certainly the right choice for new security checks.
+  //
+  // TODO(1027191): We're currently calling this method in a number of places
+  // where either `IsSameOriginWith()` or `IsSameOriginDomainWith()` might
+  // be more appropriate. We should audit its existing usage, and it might
+  // make sense to move it out of SecurityOrigin entirely to align it more
+  // tightly with `BindingSecurity` where it's clearly necessary.
   bool CanAccess(const SecurityOrigin* other) const {
     AccessResultDomainDetail unused_detail;
     return CanAccess(other, unused_detail);
   }
-
-  // Returns true if this SecurityOrigin can script objects in |other|, just
-  // as above, but also returns the category into which the access check fell.
-  //
-  // TODO(crbug.com/787905): Remove this variant once we have enough data to
-  // make decisions about `document.domain`.
   bool CanAccess(const SecurityOrigin* other, AccessResultDomainDetail&) const;
 
   // Returns true if this SecurityOrigin can read content retrieved from
@@ -294,6 +297,25 @@ class PLATFORM_EXPORT SecurityOrigin : public RefCounted<SecurityOrigin> {
   bool IsSameOriginWith(const SecurityOrigin*) const;
   static bool AreSameOrigin(const KURL& a, const KURL& b);
 
+  // This method implements HTML's "same origin-domain" check, which takes
+  // `document.domain` into account when comparing two origins.
+  //
+  // This method does not take the "universal access" flag into account. It does
+  // take the "local access" flag into account, considering `file:` origins that
+  // set the flag to be same origin-domain with all other `file:` origins that
+  // set the flag (assuming no `document.domain` mismatch).
+  //
+  // Note: Same origin-domain checks should be rare, and `IsSameOriginWith()`
+  // is almost certainly the right choice for new security checks.
+  //
+  // https://html.spec.whatwg.org/#same-origin-domain
+  bool IsSameOriginDomainWith(const SecurityOrigin* other) const {
+    AccessResultDomainDetail unused_detail;
+    return IsSameOriginDomainWith(other, unused_detail);
+  }
+  bool IsSameOriginDomainWith(const SecurityOrigin*,
+                              AccessResultDomainDetail&) const;
+
   static const KURL& UrlWithUniqueOpaqueOrigin();
 
   // Transfer origin privileges from another security origin.

third_party/blink/renderer/platform/weborigin/security_origin_test.cc

@@ -960,7 +960,7 @@ TEST_F(SecurityOriginTest, IsSameOriginWith) {
                // Opaque
                {false, "data:text/html,whatever", "data:text/html,whatever"}};
 
-  for (auto test : tests) {
+  for (const auto& test : tests) {
     SCOPED_TRACE(testing::Message() << "Origin 1: `" << test.a << "` "
                                     << "Origin 2: `" << test.b << "`\n");
     scoped_refptr<SecurityOrigin> a = SecurityOrigin::CreateFromString(test.a);
@@ -1036,4 +1036,136 @@ TEST_F(SecurityOriginTest, IsSameOriginWithWithLocalScheme) {
   EXPECT_FALSE(b->IsSameOriginWith(a.get()));
 }
 
+TEST_F(SecurityOriginTest, IsSameOriginDomainWith) {
+  struct TestCase {
+    bool same_origin_domain;
+    const char* a;
+    const char* domain_a;  // empty string === no `domain` set.
+    const char* b;
+    const char* domain_b;
+  } tests[] = {
+      {true, "https://a.com", "", "https://a.com", ""},
+      {false, "https://a.com", "a.com", "https://a.com", ""},
+      {true, "https://a.com", "a.com", "https://a.com", "a.com"},
+      {false, "https://sub.a.com", "", "https://a.com", ""},
+      {false, "https://sub.a.com", "a.com", "https://a.com", ""},
+      {true, "https://sub.a.com", "a.com", "https://a.com", "a.com"},
+      {true, "https://sub.a.com", "a.com", "https://sub.a.com", "a.com"},
+      {true, "https://sub.a.com", "a.com", "https://sub.sub.a.com", "a.com"},
+
+      // Schemes.
+      {false, "https://a.com", "", "http://a.com", ""},
+      {false, "https://a.com", "a.com", "http://a.com", ""},
+      {false, "https://a.com", "a.com", "http://a.com", "a.com"},
+      {false, "https://sub.a.com", "a.com", "http://a.com", ""},
+      {false, "https://a.com", "a.com", "http://sub.a.com", "a.com"},
+
+      // Ports? Why would they matter?
+      {true, "https://a.com:443", "", "https://a.com", ""},
+      {false, "https://a.com:444", "", "https://a.com", ""},
+      {false, "https://a.com:444", "", "https://a.com:442", ""},
+
+      {false, "https://a.com:443", "a.com", "https://a.com", ""},
+      {false, "https://a.com:444", "a.com", "https://a.com", ""},
+      {false, "https://a.com:444", "a.com", "https://a.com:442", ""},
+
+      {true, "https://a.com:443", "a.com", "https://a.com", "a.com"},
+      {true, "https://a.com:444", "a.com", "https://a.com", "a.com"},
+      {true, "https://a.com:444", "a.com", "https://a.com:442", "a.com"},
+
+      {false, "https://sub.a.com:443", "", "https://a.com", ""},
+      {false, "https://sub.a.com:444", "", "https://a.com", ""},
+      {false, "https://sub.a.com:444", "", "https://a.com:442", ""},
+
+      {false, "https://sub.a.com:443", "a.com", "https://a.com", ""},
+      {false, "https://sub.a.com:444", "a.com", "https://a.com", ""},
+      {false, "https://sub.a.com:444", "a.com", "https://a.com:442", ""},
+
+      {true, "https://sub.a.com:443", "a.com", "https://a.com", "a.com"},
+      {true, "https://sub.a.com:444", "a.com", "https://a.com", "a.com"},
+      {true, "https://sub.a.com:444", "a.com", "https://a.com:442", "a.com"},
+  };
+
+  for (const auto& test : tests) {
+    SCOPED_TRACE(testing::Message()
+                 << "Origin 1: `" << test.a << "` (`" << test.domain_a << "`)\n"
+                 << "Origin 2: `" << test.b << "` (`" << test.domain_b
+                 << "`)\n");
+    scoped_refptr<SecurityOrigin> a = SecurityOrigin::CreateFromString(test.a);
+    if (strlen(test.domain_a))
+      a->SetDomainFromDOM(test.domain_a);
+    scoped_refptr<SecurityOrigin> b = SecurityOrigin::CreateFromString(test.b);
+    if (strlen(test.domain_b))
+      b->SetDomainFromDOM(test.domain_b);
+    EXPECT_EQ(test.same_origin_domain, a->IsSameOriginDomainWith(b.get()));
+    EXPECT_EQ(test.same_origin_domain, b->IsSameOriginDomainWith(a.get()));
+
+    // Self-comparison
+    EXPECT_TRUE(a->IsSameOriginDomainWith(a.get()));
+    EXPECT_TRUE(b->IsSameOriginDomainWith(b.get()));
+
+    // DeriveNewOpaqueOrigin
+    EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(a.get()));
+    EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(a.get()));
+    EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(b.get()));
+    EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(b.get()));
+    EXPECT_FALSE(b->IsSameOriginDomainWith(a->DeriveNewOpaqueOrigin().get()));
+    EXPECT_FALSE(b->IsSameOriginDomainWith(a->DeriveNewOpaqueOrigin().get()));
+    EXPECT_FALSE(a->IsSameOriginDomainWith(b->DeriveNewOpaqueOrigin().get()));
+    EXPECT_FALSE(b->IsSameOriginDomainWith(b->DeriveNewOpaqueOrigin().get()));
+    EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(
+        a->DeriveNewOpaqueOrigin().get()));
+    EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(
+        b->DeriveNewOpaqueOrigin().get()));
+
+    // UniversalAccess does not override.
+    a->GrantUniversalAccess();
+    EXPECT_EQ(test.same_origin_domain, a->IsSameOriginDomainWith(b.get()));
+    EXPECT_EQ(test.same_origin_domain, b->IsSameOriginDomainWith(a.get()));
+  }
+}
+
+TEST_F(SecurityOriginTest, IsSameOriginDomainWithWithLocalScheme) {
+  scoped_refptr<SecurityOrigin> a =
+      SecurityOrigin::CreateFromString("file:///etc/passwd");
+  scoped_refptr<SecurityOrigin> b =
+      SecurityOrigin::CreateFromString("file:///etc/hosts");
+
+  // Self-comparison
+  EXPECT_TRUE(a->IsSameOriginDomainWith(a.get()));
+  EXPECT_TRUE(b->IsSameOriginDomainWith(b.get()));
+
+  // block_local_access_from_local_origin_ defaults to `false`:
+  EXPECT_TRUE(a->IsSameOriginDomainWith(b.get()));
+  EXPECT_TRUE(b->IsSameOriginDomainWith(a.get()));
+
+  // DeriveNewOpaqueOrigin
+  EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(a.get()));
+  EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(a.get()));
+  EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(b.get()));
+  EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(b.get()));
+  EXPECT_FALSE(b->IsSameOriginDomainWith(a->DeriveNewOpaqueOrigin().get()));
+  EXPECT_FALSE(b->IsSameOriginDomainWith(a->DeriveNewOpaqueOrigin().get()));
+  EXPECT_FALSE(a->IsSameOriginDomainWith(b->DeriveNewOpaqueOrigin().get()));
+  EXPECT_FALSE(b->IsSameOriginDomainWith(b->DeriveNewOpaqueOrigin().get()));
+  EXPECT_FALSE(a->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(
+      a->DeriveNewOpaqueOrigin().get()));
+  EXPECT_FALSE(b->DeriveNewOpaqueOrigin()->IsSameOriginDomainWith(
+      b->DeriveNewOpaqueOrigin().get()));
+
+  // Set block_local_access_from_local_origin_ to `true`:
+  a->BlockLocalAccessFromLocalOrigin();
+  EXPECT_FALSE(a->IsSameOriginDomainWith(b.get()));
+  EXPECT_FALSE(b->IsSameOriginDomainWith(a.get()));
+
+  // Self-comparison should still be true.
+  EXPECT_TRUE(a->IsSameOriginDomainWith(a.get()));
+  EXPECT_TRUE(b->IsSameOriginDomainWith(b.get()));
+
+  // UniversalAccess does not override
+  a->GrantUniversalAccess();
+  EXPECT_FALSE(a->IsSameOriginDomainWith(b.get()));
+  EXPECT_FALSE(b->IsSameOriginDomainWith(a.get()));
+}
+
 }  // namespace blink