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

content/browser/child_process_security_policy_impl.h

Index: content/browser/child_process_security_policy_impl.h
diff --git a/content/browser/child_process_security_policy_impl.h b/content/browser/child_process_security_policy_impl.h
index e99599aa98905f3413fcc2d321f3306dbfb77c0e..bb2e1ad8fdeae930291d006456e27a9b5e4a7238 100644
--- a/content/browser/child_process_security_policy_impl.h
+++ b/content/browser/child_process_security_policy_impl.h
@@ -193,6 +193,11 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // scheme+host+port tuple rather than scheme and eTLD+1 will be used.
   // SiteInstances for these origins will also use the full origin as site URL.
   //
+  // Subdomains of an isolated origin are considered to be part of that
+  // origin's site.  I.e., if https://isolated.foo.com is added as an isolated

[Charlie Reis, 2017/06/28 01:02:18]

nit: s/I.e./For example/

[alexmos, 2017/06/28 18:29:51]

Done.

+  // origin, then https://bar.isolated.foo.com will be considered part of the
+  // site for https://isolated.foo.com.
+  //
   // Note that |origin| must not be unique.  URLs that render with
   // unique origins, such as data: URLs, are not supported.  Suborigins (see
   // https://w3c.github.io/webappsec-suborigins/ -- not to be confused with
@@ -212,9 +217,37 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // AddIsolatedOrigin for definition of an isolated origin.
   void AddIsolatedOriginsFromCommandLine(const std::string& origin_list);
 
-  // Helper to check whether an origin requires origin-wide process isolation.
+  // Check whether |origin| requires origin-wide process isolation.
+  //
+  // Subdomains of an isolated origin are considered part of that isolated
+  // origin.  Thus, if https://isolated.foo.com/ had been added as an isolated
+  // origin, this will return true for https://isolated.foo.com/,
+  // https://bar.isolated.foo.com/, or https://baz.bar.isolated.foo.com/; and
+  // it will return false for https://foo.com/ or https://unisolated.foo.com/.

[Charlie Reis, 2017/06/28 01:02:18]

Maybe clarify that site URLs are not included here?  (I know we have to deal
with whether ports are considered or not somewhere-- I think that's in
SiteInstanceImpl?)

[alexmos, 2017/06/28 18:29:51]

Done.  Yeah, ports would be included in the site URL for an isolated origin in 
SiteInstanceImpl::GetSiteForURL, when it calls url::Origin::GetURL(), though
only if it's a non-default, non-empty port.  For default ports, if we mark an
etld+1 as an isolated origin, its site URL would actually match a regular site
URL for that etld+1.  Is your worry here that someone makes url::Origin out of a
GetSiteInstance()->GetSiteURL() and passes into here?  I can also add a note
specifically not to do that.

Perhaps it's easier to just drop the port if we want to reuse this to isolate
subsets of sites (to keep document.domain working across ports).  I actually
don't know if there's enough of a benefit to use ports.

[Charlie Reis, 2017/06/28 20:56:57]

I mainly wanted to point out the difference for others reading the code, since
we're careful about excluding ports in some cases.  Agreed that the type
difference will probably help distinguish the cases as well.
I think we might as well keep the port, so that we can really talk about
isolating origins in the usual definition when we're not limited to sites.

   bool IsIsolatedOrigin(const url::Origin& origin);
 
+  // This function will check whether |origin| requires process isolation, and
+  // if so, it will return true and put the most specific matching isolated
+  // origin into |result|.
+  //
+  // If |origin| does not require process isolation, this function will return
+  // false, and |result| will be a unique origin. This means that neither
+  // |origin|, nor any origins for which |origin| is a subdomain, have been
+  // registered as isolated origins.
+  //
+  // For example, if both https://isolated.com/ and
+  // https://bar.foo.isolated.com/ are registered as isolated origins, then the
+  // values returned in |result| are:
+  //   https://isolated.com/             -->  https://isolated.com/
+  //   https://foo.isolated.com/         -->  https://isolated.com/
+  //   https://bar.foo.isolated.com/     -->  https://bar.foo.isolated.com/
+  //   https://baz.bar.foo.isolated.com/ -->  https://bar.foo.isolated.com/

[Charlie Reis, 2017/06/28 01:02:18]

Maybe add a negative example?

  https://example.com    -->  (unique origin)

[alexmos, 2017/06/28 18:29:51]

Good idea, done.

+  bool TryGetMostSpecificMatchForIsolatedOrigin(const url::Origin& origin,

[Charlie Reis, 2017/06/28 01:02:18]

Maybe simplify the name to GetMatchingIsolatedOrigin?

[alexmos, 2017/06/28 18:29:51]

Done.

+                                                url::Origin* result);
+
+  // Removes a previously added isolated origin.

[Charlie Reis, 2017/06/28 01:02:18]

Might want to mention what considerations there are for calling this.  Is it
safe to do while there are active SiteInstances in that origin, or will we start
seeing undefined behavior?  (That is, do callers need to ensure that all
existing instances are closed or reloaded?)

Or should this have ForTesting in the name since it's only used from tests? 
(I'm not sure if it's safe in general.)

[alexmos, 2017/06/28 18:29:51]

I've changed it to *ForTesting for now, and added a comment hinting at the
issues if we ever need to expose this more generally.  I think you're right that
we'll likely need to ensure there are no active SiteInstances for sanity: if a
renderer is origin-locked to "isolated.foo.com" and suddenly GetSiteForURL
starts returning "foo.com", it seems that we'll kill it on cookie requests. 
Another issue might be that despite trying to minimize TOCTTOU, I think the UI
thread still checks isolated origins multiple times from different places during
transfers, etc.  So it might only be safe to allow removals from the UI thread.

We can come back to those concerns if we ever need this outside of tests.

+  void RemoveIsolatedOrigin(const url::Origin& origin);
+
  private:
   friend class ChildProcessSecurityPolicyInProcessBrowserTest;
   friend class ChildProcessSecurityPolicyTest;