Introduce ChildProcessSecurityPolicyImpl::Handle.

This change introduces a Handle object so that Mojo services can
preserve the security state beyond the lifetime of the
RenderProcessHostImpl object. This allows consistent security
checks to occur even during the period when the renderer process is
shutting down and there are still pending Mojo operations in flight.
This will be used to remove all remaining uses of
ChildProcessSecurityPolicyImpl::HasSecurityState() in follow-up CLs.

- Implements new Handle object that allows security checks to provide
  consistent results after ChildProcessSecurityPolicyImpl::Remove() is
  called.
- Convert blob code to use Handle instead of the HasSecurityState()
  workaround.

This is an updated version of https://crrev.com/c/1534368 . Further
discussion of the history and reasons for this CL can be found there.

Bug: 1035399, 943887
Change-Id: I6165fad4308643a1ddc845690443e8efceac65f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1975165Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#732296}

content/browser/blob_storage/blob_registry_wrapper.cc

@@ -20,32 +20,22 @@ namespace {
 
 class BindingDelegate : public storage::BlobRegistryImpl::Delegate {
  public:
-  explicit BindingDelegate(int process_id) : process_id_(process_id) {}
+  explicit BindingDelegate(
+      ChildProcessSecurityPolicyImpl::Handle security_policy_handle)
+      : security_policy_handle_(std::move(security_policy_handle)) {}
   ~BindingDelegate() override {}
 
   bool CanReadFile(const base::FilePath& file) override {
-    ChildProcessSecurityPolicyImpl* security_policy =
-        ChildProcessSecurityPolicyImpl::GetInstance();
-    return security_policy->CanReadFile(process_id_, file);
+    return security_policy_handle_.CanReadFile(file);
   }
   bool CanReadFileSystemFile(const storage::FileSystemURL& url) override {
-    ChildProcessSecurityPolicyImpl* security_policy =
-        ChildProcessSecurityPolicyImpl::GetInstance();
-    return security_policy->CanReadFileSystemFile(process_id_, url);
+    return security_policy_handle_.CanReadFileSystemFile(url);
   }
   bool CanCommitURL(const GURL& url) override {
-    ChildProcessSecurityPolicyImpl* security_policy =
-        ChildProcessSecurityPolicyImpl::GetInstance();
-    return security_policy->CanCommitURL(process_id_, url);
+    return security_policy_handle_.CanCommitURL(url);
   }
-  bool IsProcessValid() override {
-    ChildProcessSecurityPolicyImpl* security_policy =
-        ChildProcessSecurityPolicyImpl::GetInstance();
-    return security_policy->HasSecurityState(process_id_);
-  }
-
  private:
-  const int process_id_;
+  ChildProcessSecurityPolicyImpl::Handle security_policy_handle_;
 };
 
 }  // namespace
@@ -69,8 +59,11 @@ void BlobRegistryWrapper::Bind(
     int process_id,
     mojo::PendingReceiver<blink::mojom::BlobRegistry> receiver) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-  blob_registry_->Bind(std::move(receiver),
-                       std::make_unique<BindingDelegate>(process_id));
+  blob_registry_->Bind(
+      std::move(receiver),
+      std::make_unique<BindingDelegate>(
+          ChildProcessSecurityPolicyImpl::GetInstance()->CreateHandle(
+              process_id)));
 }
 
 BlobRegistryWrapper::~BlobRegistryWrapper() {}

content/browser/browser_context.cc

@@ -456,13 +456,13 @@ void BrowserContext::NotifyWillBeDestroyed(BrowserContext* browser_context) {
     }
   }
 
-  // Clean up any isolated origins associated with this BrowserContext.  This
-  // should be safe now that all RenderProcessHosts are destroyed, since future
-  // navigations or security decisions shouldn't ever need to consult these
-  // isolated origins.
+  // Clean up any isolated origins and other security state associated with this
+  // BrowserContext.  This should be safe now that all RenderProcessHosts are
+  // destroyed, since future navigations or security decisions shouldn't ever
+  // need to consult these isolated origins and other security state.
   ChildProcessSecurityPolicyImpl* policy =
       ChildProcessSecurityPolicyImpl::GetInstance();
-  policy->RemoveIsolatedOriginsForBrowserContext(*browser_context);
+  policy->RemoveStateForBrowserContext(*browser_context);
 }
 
 void BrowserContext::EnsureResourceContextInitialized(BrowserContext* context) {

content/browser/child_process_security_policy_impl.h

@@ -53,6 +53,68 @@ class SiteInstance;
 class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
     : public ChildProcessSecurityPolicy {
  public:
+  // Handle used to access the security state for a specific process.
+  //
+  // Objects that require the security state to be preserved beyond the
+  // lifetime of the RenderProcessHostImpl should hold an instance of this
+  // object and use it to answer security policy questions. (e.g. Mojo services
+  // created by RPHI that can receive calls after RPHI destruction). This
+  // object should only be called on the UI and IO threads.
+  //
+  // Note: Some security methods, like CanAccessDataForOrigin(), require
+  // information from the BrowserContext to make its decisions. These methods
+  // will fall back to failsafe values if called after BrowserContext
+  // destruction. Callers should be prepared to gracefully handle this or
+  // ensure that they don't make any calls after BrowserContext destruction.
+  class CONTENT_EXPORT Handle {
+   public:
+    Handle();
+    Handle(Handle&&);
+    Handle(const Handle&) = delete;
+    ~Handle();
+
+    Handle& operator=(const Handle&) = delete;
+    Handle& operator=(Handle&&);
+
+    // Returns true if this object has a valid process ID.
+    // Returns false if this object was created with the default constructor,
+    // the contents of this object was transferred to another Handle via
+    // std::move(), or ChildProcessSecurityPolicyImpl::CreateHandle()
+    // created this object after the process has already been destructed.
+    bool is_valid() const;
+
+    // Whether the process is allowed to commit a document from the given URL.
+    bool CanCommitURL(const GURL& url);
+
+    // Before servicing a child process's request to upload a file to the web,
+    // the browser should call this method to determine whether the process has
+    // the capability to upload the requested file.
+    bool CanReadFile(const base::FilePath& file);
+
+    // Explicit read permissions check for FileSystemURL specified files.
+    bool CanReadFileSystemFile(const storage::FileSystemURL& url);
+
+    // Returns true if the process is permitted to read and modify the data for
+    // the origin of |url|. This is currently used to protect data such as
+    // cookies, passwords, and local storage. Does not affect cookies attached
+    // to or set by network requests.
+    //
+    // This can only return false for processes locked to a particular origin,
+    // which can happen for any origin when the --site-per-process flag is used,
+    // or for isolated origins that require a dedicated process (see
+    // AddIsolatedOrigins).
+    bool CanAccessDataForOrigin(const GURL& url);
+    bool CanAccessDataForOrigin(const url::Origin& origin);
+
+   private:
+    friend class ChildProcessSecurityPolicyImpl;
+    explicit Handle(int child_id);
+
+    // The ID of the child process that this handle is associated with or
+    // ChildProcessHost::kInvalidUniqueID if the handle is no longer valid.
+    int child_id_;
+  };
+
   // Object can only be created through GetInstance() so the constructor is
   // private.
   ~ChildProcessSecurityPolicyImpl() override;
@@ -281,14 +343,14 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // Returns true if sending system exclusive messages is allowed.
   bool CanSendMidiSysExMessage(int child_id);
 
-  // Remove all isolated origins associated with |browser_context|.  This is
+  // Remove all isolated origins associated with |browser_context| and clear any
+  // pointers that may reference |browser_context|.  This is
   // typically used when |browser_context| is being destroyed and assumes that
   // no processes are running or will run for that profile; this makes the
   // isolated origin removal safe.  Note that |browser_context| cannot be null;
   // i.e., isolated origins that apply globally to all profiles cannot
   // currently be removed, since that is not safe to do at runtime.
-  void RemoveIsolatedOriginsForBrowserContext(
-      const BrowserContext& browser_context);
+  void RemoveStateForBrowserContext(const BrowserContext& browser_context);
 
   // Check whether |origin| requires origin-wide process isolation within
   // |isolation_context|.
@@ -332,9 +394,23 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
   // process associated with |child_id|.
   void LogKilledProcessOriginLock(int child_id);
 
+  // Creates a Handle object for a specific child process ID.
+  //
+  // This handle can be used to extend the lifetime of policy state beyond
+  // the Remove() call for |child_id|. This should be used by objects that can
+  // outlive the RenderProcessHostImpl object associated with |child_id| and
+  // need to be able to make policy decisions after RPHI destruction. (e.g.
+  // Mojo services created by RPHI)
+  //
+  // Returns a valid Handle for any |child_id| that is present in
+  // |security_state_|. Otherwise it returns a Handle that returns false for
+  // all policy checks.
+  Handle CreateHandle(int child_id);
+
  private:
   friend class ChildProcessSecurityPolicyInProcessBrowserTest;
   friend class ChildProcessSecurityPolicyTest;
+  friend class ChildProcessSecurityPolicyImpl::Handle;
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyInProcessBrowserTest,
                            NoLeak);
   FRIEND_TEST_ALL_PREFIXES(ChildProcessSecurityPolicyTest, FilePermissions);
@@ -502,6 +578,12 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
                           IsolatedOriginSource source,
                           BrowserContext* browser_context = nullptr);
 
+  bool AddProcessReference(int child_id);
+  bool AddProcessReferenceLocked(int child_id) EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  void RemoveProcessReference(int child_id);
+  void RemoveProcessReferenceLocked(int child_id)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
+
   // Creates the value to place in the "killed_process_origin_lock" crash key
   // based on the contents of |security_state|.
   static std::string GetKilledProcessOriginLock(
@@ -539,6 +621,13 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
 
   FileSystemPermissionPolicyMap file_system_policy_map_ GUARDED_BY(lock_);
 
+  // Contains a mapping between child process ID and the number of outstanding
+  // references that want to keep the SecurityState for each process alive.
+  // This object and Handles created by this object increment/decrement
+  // the counts in this map and only destroy a SecurityState object for a
+  // process when its count goes to zero.
+  std::map<int, int> process_reference_counts_ GUARDED_BY(lock_);
+
   // You must acquire this lock before reading or writing isolated_origins_.
   // You must not block while holding this lock.
   //

content/browser/site_instance_impl_unittest.cc

@@ -1371,7 +1371,7 @@ TEST_F(SiteInstanceTest, StartIsolatingSite) {
   EXPECT_FALSE(IsIsolatedOrigin(blank_url));
 
   // Cleanup.
-  policy->RemoveIsolatedOriginsForBrowserContext(*context());
+  policy->RemoveStateForBrowserContext(*context());
 }
 
 TEST_F(SiteInstanceTest, CreateForURL) {

storage/browser/blob/blob_registry_impl.h

@@ -32,7 +32,6 @@ class COMPONENT_EXPORT(STORAGE_BROWSER) BlobRegistryImpl
     virtual bool CanReadFile(const base::FilePath& file) = 0;
     virtual bool CanReadFileSystemFile(const FileSystemURL& url) = 0;
     virtual bool CanCommitURL(const GURL& url) = 0;
-    virtual bool IsProcessValid() = 0;
   };
 
   BlobRegistryImpl(base::WeakPtr<BlobStorageContext> context,

storage/browser/blob/blob_url_store_impl.cc

@@ -78,12 +78,7 @@ void BlobURLStoreImpl::Register(mojo::PendingRemote<blink::mojom::Blob> blob,
     std::move(callback).Run();
     return;
   }
-  // Only report errors when we don't have permission to commit and
-  // the process is valid. The process check is a temporary solution to
-  // handle cases where this method is run after the
-  // process associated with |delegate_| has been destroyed.
-  // See https://crbug.com/933089 for details.
-  if (!delegate_->CanCommitURL(url) && delegate_->IsProcessValid()) {
+  if (!delegate_->CanCommitURL(url)) {
     mojo::ReportBadMessage(
         "Non committable URL passed to BlobURLStore::Register");
     std::move(callback).Run();
@@ -107,12 +102,7 @@ void BlobURLStoreImpl::Revoke(const GURL& url) {
     mojo::ReportBadMessage("Invalid scheme passed to BlobURLStore::Revoke");
     return;
   }
-  // Only report errors when we don't have permission to commit and
-  // the process is valid. The process check is a temporary solution to
-  // handle cases where this method is run after the
-  // process associated with |delegate_| has been destroyed.
-  // See https://crbug.com/933089 for details.
-  if (!delegate_->CanCommitURL(url) && delegate_->IsProcessValid()) {
+  if (!delegate_->CanCommitURL(url)) {
     mojo::ReportBadMessage(
         "Non committable URL passed to BlobURLStore::Revoke");
     return;

storage/browser/blob/blob_url_store_impl_unittest.cc

@@ -216,17 +216,6 @@ TEST_F(BlobURLStoreImplTest, RevokeCantCommit) {
   EXPECT_EQ(1u, bad_messages_.size());
 }
 
-TEST_F(BlobURLStoreImplTest, RevokeCantCommit_ProcessNotValid) {
-  delegate_.can_commit_url_result = false;
-  delegate_.is_process_valid_result = false;
-
-  mojo::Remote<BlobURLStore> url_store(CreateURLStore());
-  url_store->Revoke(kValidUrl);
-  url_store.FlushForTesting();
-  EXPECT_TRUE(bad_messages_.empty());
-  EXPECT_FALSE(context_->GetBlobFromPublicURL(kValidUrl));
-}
-
 TEST_F(BlobURLStoreImplTest, RevokeURLWithFragment) {
   mojo::Remote<BlobURLStore> url_store(CreateURLStore());
   url_store->Revoke(kFragmentUrl);

storage/browser/test/mock_blob_registry_delegate.cc

@@ -15,8 +15,5 @@ bool MockBlobRegistryDelegate::CanReadFileSystemFile(const FileSystemURL& url) {
 bool MockBlobRegistryDelegate::CanCommitURL(const GURL& url) {
   return can_commit_url_result;
 }
-bool MockBlobRegistryDelegate::IsProcessValid() {
-  return is_process_valid_result;
-}
 
 }  // namespace storage

storage/browser/test/mock_blob_registry_delegate.h

@@ -17,12 +17,10 @@ class MockBlobRegistryDelegate : public BlobRegistryImpl::Delegate {
   bool CanReadFile(const base::FilePath& file) override;
   bool CanReadFileSystemFile(const FileSystemURL& url) override;
   bool CanCommitURL(const GURL& url) override;
-  bool IsProcessValid() override;
 
   bool can_read_file_result = true;
   bool can_read_file_system_file_result = true;
   bool can_commit_url_result = true;
-  bool is_process_valid_result = true;
 };
 
 }  // namespace storage