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

content/browser/child_process_security_policy_impl.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_IMPL_H_
 #define CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_IMPL_H_
 
 #include <map>
 #include <memory>
 #include <set>
@@ skipping 175 matching lines @@
                                           int policy);
 
   // Returns true if sending system exclusive messages is allowed.
   bool CanSendMidiSysExMessage(int child_id);
 
   // Add an origin to the list of origins that require process isolation.
   // When making process model decisions for such origins, the full
   // 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
   // subdomains) and non-standard schemes are also not supported.  Sandboxed
   // frames (e.g., <iframe sandbox>)
   // *are* supported, since process placement decisions will be based on the

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

minor nit: Rewrap?

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

Oops, done.

   // URLs such frames navigate to, and not the origin of committed documents
   // (which might be unique).  If an isolated origin opens an about:blank
   // popup, it will stay in the isolated origin's process. Nested URLs
   // (filesystem: and blob:) retain process isolation behavior of their inner
   // origin.
   void AddIsolatedOrigin(const url::Origin& origin);
 
   // Register a set of isolated origins as specified on the command line with
   // the --isolate-origins flag.  |origin_list| is the flag's value, which
   // contains the list of comma-separated scheme-host-port origins. See
   // 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;
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyInProcessBrowserTest,
                            NoLeak);
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyTest, FilePermissions);
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyTest,
                            IsolateOriginsFromCommandLine);
 
   class SecurityState;
@@ skipping 84 matching lines @@
   // eTLD+1. Each of these origins requires a dedicated process.  This set is
   // protected by |lock_|.
   std::set<url::Origin> isolated_origins_;
 
   DISALLOW_COPY_AND_ASSIGN(ChildProcessSecurityPolicyImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_IMPL_H_