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

content/browser/child_process_security_policy_impl.cc

Index: content/browser/child_process_security_policy_impl.cc
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index 210a3c3c7cb5c9e7c2c9ab5e7c957a6a69c6cbc9..eb26ff4e081a84bebf13d394cf060e4548a13769 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -17,6 +17,7 @@
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "build/build_config.h"
+#include "content/browser/isolated_origin_util.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/common/resource_request_body_impl.h"
 #include "content/common/site_isolation_policy.h"
@@ -1094,10 +1095,11 @@ void ChildProcessSecurityPolicyImpl::AddIsolatedOrigin(
     const url::Origin& origin) {
   CHECK(!origin.unique())
       << "Cannot register a unique origin as an isolated origin.";

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

Should this enforce SchemeIsHttpOrHttps? The domain chopping it does assumes
that the host is a network host.

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

That's a good idea.  Besides problematic subdomain matching, I also don't know
how this would interact with process-per-site behavior if we enforced this on
something like a chrome:// URL.  To address this and your other corner cases, I
moved all validity checks into a new helper and added a unit test for it.

-  CHECK(!IsIsolatedOrigin(origin))
-      << "Duplicate isolated origin: " << origin.Serialize();
 

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

Do we get into any trouble if origin's hostname is an IP literal?

Similarly/relatedly, can we get into trouble if if the origin is, say,
"http://co.uk"? (in registry_controlled_domains terms, I think this means that
GetRegistryLength() returns 0).

An interesting case here is whether we'd want to ever support a privately-listed
PSL entry (appspot.com, github.io) to "process isolate all my subdomains".
Cookies are already not assignable across these because of the PSL inclusion,
but GetRegistryLength("appspot.com") will return 0 if you include the private
registries.

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

It probably should be ok to isolate an IP address, but we shouldn't do subdomain
matching on it, similarly to Blink's document.domain logic [1].  This was
actually broken, and I added a case to handle it in
DoesOriginMatchIsolatedOrigin.  Thanks for bringing this up!

[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/webor...
I think we can indeed get into trouble with these, as subdomain matching on
http://co.uk would cause us to group http://foo.co.uk and http://bar.co.uk into
the same origin/process, which the regular site rules would not normally allow. 
So we definitely need to require that isolated origins cannot be a TLD.

I added a check for this using HostHasRegistryControlledDomain(), which seems to
be equivalent to GetRegistryLength() returning 0.  However, this also disallows
use cases like http://go/ or http://localhost/, which might actually be
legitimate usage of --isolate-origins by someone trying to protect their
intranet sites, etc.  I don't see how to easily make an exception for those, as
registry_controlled_domains doesn't have anything to check only if the host is
itself a registry, which is what we really want here.  Or maybe I missed it? 
(We could always add something, or maybe check kDafsa directly with
LookupStringInFixedSet, but that would be for another CL, and maybe worth doing
only when someone actually needs this.)
That's an interesting use case.  It'll require additional support to treat each
subdomain as its own isolated origin, i.e., the opposite of what this CL is
doing,  and when we add an isolated origin, we might specify a flag to indicate
what kind of treatment subdomains require.

   base::AutoLock lock(lock_);
+  CHECK(!isolated_origins_.count(origin))
+      << "Duplicate isolated origin: " << origin.Serialize();
+
   isolated_origins_.insert(origin);
 }
 
@@ -1114,8 +1116,38 @@ void ChildProcessSecurityPolicyImpl::AddIsolatedOriginsFromCommandLine(
 
 bool ChildProcessSecurityPolicyImpl::IsIsolatedOrigin(
     const url::Origin& origin) {
+  url::Origin unused_result;
+  return GetMatchingIsolatedOrigin(origin, &unused_result);
+}
+
+bool ChildProcessSecurityPolicyImpl::GetMatchingIsolatedOrigin(
+    const url::Origin& origin,
+    url::Origin* result) {
+  *result = url::Origin();
+  base::AutoLock lock(lock_);
+
+  // If multiple isolated origins are registered with a common domain suffix,
+  // return the most specific one.  For example, if foo.isolated.com and
+  // isolated.com are both isolated origins, bar.foo.isolated.com should return
+  // foo.isolated.com.
+  bool found = false;
+  for (auto isolated_origin : isolated_origins_) {
+    if (IsolatedOriginUtil::DoesOriginMatchIsolatedOrigin(origin,
+                                                          isolated_origin)) {
+      if (!found || result->host().length() < isolated_origin.host().length()) {
+        *result = isolated_origin;
+        found = true;
+      }
+    }
+  }
+
+  return found;
+}
+
+void ChildProcessSecurityPolicyImpl::RemoveIsolatedOriginForTesting(
+    const url::Origin& origin) {
   base::AutoLock lock(lock_);
-  return isolated_origins_.find(origin) != isolated_origins_.end();
+  isolated_origins_.erase(origin);
 }
 
 }  // namespace content