Introduce support for origins that require process isolation.

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>
 #include <string>
 #include <vector>
 
 #include "base/compiler_specific.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
 #include "content/public/browser/child_process_security_policy.h"
 #include "content/public/common/resource_type.h"
 #include "storage/common/fileapi/file_system_types.h"
+#include "url/origin.h"
 
 class GURL;
 
 namespace base {
 class FilePath;
 }
 
 namespace storage {
 class FileSystemURL;
 }
@@ skipping 131 matching lines @@
 
   // Register FileSystem type and permission policy which should be used
   // for the type.  The |policy| must be a bitwise-or'd value of
   // storage::FilePermissionPolicy.
   void RegisterFileSystemPermissionPolicy(storage::FileSystemType type,
                                           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.
+  //
+  // Note that |origin| must not be unique.  URLs that render with
+  // unique origins, such as data: URLs, are not supported.  Suborigins and

[Charlie Reis, 2017/05/19 00:10:18]

Might clarify what you mean by suborigins, since it sounds like bar.foo.com at
first.  Maybe there's a link we can use, or a more specific term?

[alexmos, 2017/05/24 00:19:56]

Ah, good call - I didn't realize there might be such a confusion.  I added a
link to the spec and an explicit comment that these are not subdomains.

+  // non-standard schemes are also not supported.  Sandboxed frames (e.g.,
+  // <iframe sandbox>) *are* supported, since process placement decisions will
+  // be based on the 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.
+  bool IsIsolatedOrigin(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;
 
   typedef std::set<std::string> SchemeSet;
   typedef std::map<int, std::unique_ptr<SecurityState>> SecurityStateMap;
   typedef std::map<int, int> WorkerToMainProcessMap;
   typedef std::map<storage::FileSystemType, int> FileSystemPermissionPolicyMap;
 
   // Obtain an instance of ChildProcessSecurityPolicyImpl via GetInstance().
   ChildProcessSecurityPolicyImpl();
@@ skipping 64 matching lines @@
   // owned by this object and are protected by |lock_|.  References to them must
   // not escape this class.
   SecurityStateMap security_state_;
 
   // This maps keeps the record of which js worker thread child process
   // corresponds to which main js thread child process.
   WorkerToMainProcessMap worker_map_;
 
   FileSystemPermissionPolicyMap file_system_policy_map_;
 
+  // Protects access to isolated_origins_.  This has to be a separate lock from
+  // |lock_|, because it's possible to attempt access to isolated_origins_ via
+  // IsIsolatedOrigin() from outside ChildProcessSecurityPolicy while already
+  // holding |lock_|.  A scenario where this happens is
+  // CanAccessDataForOrigin() (grabs lock_) -> SiteInstance::GetSiteForURL() ->
+  // IsIsolatedOrigin().

[alexmos, 2017/05/16 17:26:38]

This cycle is nasty, and I'm not sure this is a good way to deal with it. :(  I
considered handling isolated origins separately before calling GetSiteForURL,
but that didn't seem clean (it'd need to be done in
CPSPI::CanAccessDataForOrigin, where isolated_origins_ are, and not on
SecurityState, but it'd still need origin_lock_ to compare against which is on
SecurityState).  Or maybe we could move the isolated origin tracking into their
own class after all, to avoid tying their state synchronization to the rest of
CPSP.  Let me know what you think.

[Charlie Reis, 2017/05/19 00:10:19]

Ooh, this is tough.  Let's find a chance to chat about this, since it's
unfortunate to need another lock...

[Charlie Reis, 2017/05/19 16:54:07]

What if we moved the GetSiteForURL call from
SecurityState::CanAccessDataForOrigin out to
ChildProcessSecurityPolicyImpl::CanAccessDataForOrigin, before acquiring lock_? 
Then we could document that both CanAccessDataForOrigin and LockToOrigin on
SecurityState expect site_urls, not arbitrary GURLs, and hopefully avoid the
need for an extra lock?

It looks like the only other use of SiteInstance in
ChildProcessSecurityPolicyImpl is another GetSiteForURL call within
LockToOrigin, but that's also done before lock_ is acquired, so it shouldn't be
a problem.

[alexmos, 2017/05/24 00:19:56]

Thanks for the suggestion!  I went ahead and followed it.  I initially thought
something like this would be problematic because we'll end up doing
CanAccessDataForOrigin over two transactions instead of one, opening the door to
races in between the lock getting acquired, but thinking about this more
carefully, this might actually be ok.

The scenario I was worried about was:
1. [IO] www.foo.com accesses a cookie and retrieves GetSiteForURL.  That queries
isolated origins and then returns "foo.com".
2. [UI] www.foo.com is added to the isolated origin list (e.g., as part of SB
clickthrough), and we load www.foo.com in a new process locked to a site URL of
"www.foo.com".
3. [IO] continuing step 1, we check whether origin_lock_ matches "foo.com"
(grabbed from step 1), and it might not if we end up using the origin_lock_ of
"www.foo.com" from step 2.

But I think steps 1-3 can't happen for the same process as step 2: we won't
change the origin lock on any existing processes, we'll only use the new origin
lock for new processes dedicated to the new isolated origin.  Even if we decide
to do something like reloading tabs containing the newly-isolated origin within
the same BI, they will reload in a new dedicated process.  So I think this is
fine. :)

+  base::Lock isolated_origins_lock_;
+
+  // Tracks origins for which the entire origin should be treated as a site
+  // when making process model decisions, rather than the origin's scheme and
+  // eTLD+1. Each of these origins requires a dedicated process.  This set is
+  // protected by |isolated_origins_lock_|.
+  std::set<url::Origin> isolated_origins_;
+
   DISALLOW_COPY_AND_ASSIGN(ChildProcessSecurityPolicyImpl);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_CHILD_PROCESS_SECURITY_POLICY_IMPL_H_