Keep subdomains of an isolated origin in the isolated origin's SiteInstance.

content/browser/site_instance_impl_unittest.cc

Index: content/browser/site_instance_impl_unittest.cc
diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc
index 39246c9be642c82ea95c42d0daf7916b391cc30d..06e2eec979884287eae96aaca2ef8ece435314fb 100644
--- a/content/browser/site_instance_impl_unittest.cc
+++ b/content/browser/site_instance_impl_unittest.cc
@@ -905,8 +905,6 @@ TEST_F(SiteInstanceTest, IsolatedOrigins) {
       policy->IsIsolatedOrigin(url::Origin(GURL("https://isolated.foo.com"))));
   EXPECT_FALSE(policy->IsIsolatedOrigin(
       url::Origin(GURL("http://isolated.foo.com:12345"))));
-  EXPECT_FALSE(policy->IsIsolatedOrigin(
-      url::Origin(GURL("http://bar.isolated.foo.com"))));
 
   policy->AddIsolatedOrigin(url::Origin(isolated_bar_url));
   EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(isolated_bar_url)));
@@ -949,6 +947,146 @@ TEST_F(SiteInstanceTest, IsolatedOrigins) {
       nullptr, isolated_blob_foo_url));
   EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
       nullptr, isolated_filesystem_foo_url));
+
+  // Cleanup.
+  policy->RemoveIsolatedOriginForTesting(url::Origin(isolated_foo_url));
+  policy->RemoveIsolatedOriginForTesting(url::Origin(isolated_bar_url));
+}
+
+TEST_F(SiteInstanceTest, SubdomainOnIsolatedSite) {
+  GURL isolated_url("http://isolated.com");
+  GURL foo_isolated_url("http://foo.isolated.com");
+
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  policy->AddIsolatedOrigin(url::Origin(isolated_url));
+
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(isolated_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(foo_isolated_url)));
+  EXPECT_FALSE(
+      policy->IsIsolatedOrigin(url::Origin(GURL("http://unisolated.com"))));
+  EXPECT_FALSE(
+      policy->IsIsolatedOrigin(url::Origin(GURL("http://isolated.foo.com"))));
+  // Wrong scheme.
+  EXPECT_FALSE(
+      policy->IsIsolatedOrigin(url::Origin(GURL("https://foo.isolated.com"))));
+
+  // A new SiteInstance created for a subdomain on an isolated origin
+  // should use the isolated origin's host and not its own host as the site
+  // URL.
+  EXPECT_EQ(isolated_url,
+            SiteInstance::GetSiteForURL(nullptr, foo_isolated_url));
+
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, foo_isolated_url));
+
+  EXPECT_TRUE(
+      SiteInstance::IsSameWebSite(nullptr, isolated_url, foo_isolated_url));
+  EXPECT_TRUE(
+      SiteInstance::IsSameWebSite(nullptr, foo_isolated_url, isolated_url));
+
+  // Cleanup.
+  policy->RemoveIsolatedOriginForTesting(url::Origin(isolated_url));
+}
+
+TEST_F(SiteInstanceTest, SubdomainOnIsolatedOrigin) {
+  GURL foo_url("http://foo.com");
+  GURL isolated_foo_url("http://isolated.foo.com");
+  GURL bar_isolated_foo_url("http://bar.isolated.foo.com");
+  GURL baz_isolated_foo_url("http://baz.isolated.foo.com");

[ncarter (slow), 2017/06/28 20:59:19]

You might want to toss in a test somewhere that does something with "foo.com."
(with the terminal dot).  Chrome generally treats foo.com and foo.com. as
different domains.

[alexmos, 2017/06/29 21:54:02]

This is really interesting.  I played around with this, and indeed we treat them
as different hosts when we make process model decisions (i.e., a subframe
"foo.com." on "foo.com" ends up in an OOPIF with --site-per-process) and
same-origin policy (the origins are different and include the trailing dot). 
But I'm not sure if doing the same for isolated origins is the right thing to
do.  If we add process isolation to "https://accounts.google.com", we should
probably make sure that an attacker can't trivially bypass it by going to
"https://accounts.google.com." instead.  In practice, it won't get the same
cookies, as you pointed out, but it might give a way to bypass the user gesture
requirement that signin cares about and needs the process isolation for.  (Then
again, accounts.google.com. redirects to accounts.google.com, but this might
come up for another site that doesn't do this.)

The current implementation almost already does this correctly because DomainIs
ignores the dot in |origin|.  I added some coverage for "foo.bar." to be
considered part of a "foo.bar" isolated origin to SubdomainOnIsolatedSite.

We might want to look into this for regular site URLs as well, where "foo.com."
currently ends up in a different SiteInstance from "foo.com".  For example, I
don't know if process isolation we enforce for default search engines is broken
by adding a trailing dot.

Another question is what to do when someone adds an isolated origin with a host
that has the trailing dot, "foo.bar.", and then asks whether "foo.bar" should be
isolated.   To be consistent, we might also consider them the same, but there's
a corner case where foo.bar is an intranet host and bar is a TLD.  In that case,
"foo.bar" would normally stand for "foo.bar.corp.internal.com", and someone
would use "foo.bar." to override that and go to the external site.  So if
someone only tried to isolate the external site, they'd specify it with the
trailing dot, and it would make sense to treat these as different isolated
origins.  This sounds complicated though, and there are probably other cases I'm
not thinking of, so for now I've just disabled adding isolated origins with a
trailing dot until we have a use case for it and can think through it.  Let me
know if that sounds ok.

+
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  policy->AddIsolatedOrigin(url::Origin(isolated_foo_url));
+
+  EXPECT_FALSE(policy->IsIsolatedOrigin(url::Origin(foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(isolated_foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(bar_isolated_foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(baz_isolated_foo_url)));
+
+  EXPECT_EQ(foo_url, SiteInstance::GetSiteForURL(nullptr, foo_url));
+  EXPECT_EQ(isolated_foo_url,
+            SiteInstance::GetSiteForURL(nullptr, isolated_foo_url));
+  EXPECT_EQ(isolated_foo_url,
+            SiteInstance::GetSiteForURL(nullptr, bar_isolated_foo_url));
+  EXPECT_EQ(isolated_foo_url,
+            SiteInstance::GetSiteForURL(nullptr, baz_isolated_foo_url));
+
+  if (!AreAllSitesIsolatedForTesting()) {
+    EXPECT_FALSE(
+        SiteInstanceImpl::DoesSiteRequireDedicatedProcess(nullptr, foo_url));
+  }
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, isolated_foo_url));
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, bar_isolated_foo_url));
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, baz_isolated_foo_url));
+
+  EXPECT_FALSE(SiteInstance::IsSameWebSite(nullptr, foo_url, isolated_foo_url));
+  EXPECT_FALSE(SiteInstance::IsSameWebSite(nullptr, isolated_foo_url, foo_url));
+  EXPECT_FALSE(
+      SiteInstance::IsSameWebSite(nullptr, foo_url, bar_isolated_foo_url));
+  EXPECT_FALSE(
+      SiteInstance::IsSameWebSite(nullptr, bar_isolated_foo_url, foo_url));
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, bar_isolated_foo_url,
+                                          isolated_foo_url));
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, isolated_foo_url,
+                                          bar_isolated_foo_url));
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, bar_isolated_foo_url,
+                                          baz_isolated_foo_url));
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, baz_isolated_foo_url,
+                                          bar_isolated_foo_url));
+
+  // Cleanup.
+  policy->RemoveIsolatedOriginForTesting(url::Origin(isolated_foo_url));
+}
+
+TEST_F(SiteInstanceTest, MultipleIsolatedOriginsWithCommonSite) {
+  GURL foo_url("http://foo.com");
+  GURL bar_foo_url("http://bar.foo.com");
+  GURL baz_bar_foo_url("http://baz.bar.foo.com");
+  GURL qux_baz_bar_foo_url("http://qux.baz.bar.foo.com");
+
+  auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
+  policy->AddIsolatedOrigin(url::Origin(foo_url));
+  policy->AddIsolatedOrigin(url::Origin(baz_bar_foo_url));
+
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(bar_foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(baz_bar_foo_url)));
+  EXPECT_TRUE(policy->IsIsolatedOrigin(url::Origin(qux_baz_bar_foo_url)));
+
+  EXPECT_EQ(foo_url, SiteInstance::GetSiteForURL(nullptr, foo_url));
+  EXPECT_EQ(foo_url, SiteInstance::GetSiteForURL(nullptr, bar_foo_url));
+  EXPECT_EQ(baz_bar_foo_url,
+            SiteInstance::GetSiteForURL(nullptr, baz_bar_foo_url));
+  EXPECT_EQ(baz_bar_foo_url,
+            SiteInstance::GetSiteForURL(nullptr, qux_baz_bar_foo_url));
+
+  EXPECT_TRUE(
+      SiteInstanceImpl::DoesSiteRequireDedicatedProcess(nullptr, foo_url));
+  EXPECT_TRUE(
+      SiteInstanceImpl::DoesSiteRequireDedicatedProcess(nullptr, bar_foo_url));
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, baz_bar_foo_url));
+  EXPECT_TRUE(SiteInstanceImpl::DoesSiteRequireDedicatedProcess(
+      nullptr, qux_baz_bar_foo_url));
+
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, foo_url, bar_foo_url));
+  EXPECT_FALSE(SiteInstance::IsSameWebSite(nullptr, foo_url, baz_bar_foo_url));
+  EXPECT_FALSE(
+      SiteInstance::IsSameWebSite(nullptr, foo_url, qux_baz_bar_foo_url));
+
+  EXPECT_FALSE(
+      SiteInstance::IsSameWebSite(nullptr, bar_foo_url, baz_bar_foo_url));
+  EXPECT_FALSE(
+      SiteInstance::IsSameWebSite(nullptr, bar_foo_url, qux_baz_bar_foo_url));
+
+  EXPECT_TRUE(SiteInstance::IsSameWebSite(nullptr, baz_bar_foo_url,
+                                          qux_baz_bar_foo_url));
+
+  // Cleanup.
+  policy->RemoveIsolatedOriginForTesting(url::Origin(foo_url));
+  policy->RemoveIsolatedOriginForTesting(url::Origin(baz_bar_foo_url));
 }
 
 }  // namespace content