Fix CPSPI::Handle duplication after Remove().

This change simply allows a valid Handle to be duplicated after
ChildProcessSecurityPolicyImpl::Remove() has been called for the process
associated with the Handle. Handles are intended to keep security state
alive beyond the process lifetime and some Handles are duplicated so
they can be passed down into lower level code. This change fixes a bug
where Duplicate() would generate invalid Handle objects if it was
called after CPSPI::Remove() was called. These calls can happen because
of Mojo message queuing and no ordering constraints between different
Mojo interfaces.

Bug: 977169, 1086306
Change-Id: Icacaf06b049ba88ae7152f5c7307312d73134824
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2226156Reviewed-by: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Auto-Submit: Aaron Colwell <acolwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#775272}

content/browser/child_process_security_policy_impl.cc

@@ -166,10 +166,11 @@ void LogCanAccessDataForOriginCrashKeys(
 ChildProcessSecurityPolicyImpl::Handle::Handle()
     : child_id_(ChildProcessHost::kInvalidUniqueID) {}
 
-ChildProcessSecurityPolicyImpl::Handle::Handle(int child_id)
+ChildProcessSecurityPolicyImpl::Handle::Handle(int child_id,
+                                               bool duplicating_handle)
     : child_id_(child_id) {
   auto* policy = ChildProcessSecurityPolicyImpl::GetInstance();
-  if (!policy->AddProcessReference(child_id_))
+  if (!policy->AddProcessReference(child_id_, duplicating_handle))
     child_id_ = ChildProcessHost::kInvalidUniqueID;
 }
 
@@ -180,7 +181,7 @@ ChildProcessSecurityPolicyImpl::Handle::Handle(Handle&& rhs)
 
 ChildProcessSecurityPolicyImpl::Handle
 ChildProcessSecurityPolicyImpl::Handle::Duplicate() {
-  return Handle(child_id_);
+  return Handle(child_id_, /* duplicating_handle */ true);
 }
 
 ChildProcessSecurityPolicyImpl::Handle::~Handle() {
@@ -664,6 +665,7 @@ void ChildProcessSecurityPolicyImpl::Add(int child_id,
                                          BrowserContext* browser_context) {
   DCHECK(browser_context);
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_NE(child_id, ChildProcessHost::kInvalidUniqueID);
   base::AutoLock lock(lock_);
   if (security_state_.find(child_id) != security_state_.end()) {
     NOTREACHED() << "Add child process at most once.";
@@ -671,11 +673,12 @@ void ChildProcessSecurityPolicyImpl::Add(int child_id,
   }
 
   security_state_[child_id] = std::make_unique<SecurityState>(browser_context);
-  CHECK(AddProcessReferenceLocked(child_id));
+  CHECK(AddProcessReferenceLocked(child_id, /* duplicating_handle */ false));
 }
 
 void ChildProcessSecurityPolicyImpl::Remove(int child_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  DCHECK_NE(child_id, ChildProcessHost::kInvalidUniqueID);
   base::AutoLock lock(lock_);
 
   auto state = security_state_.find(child_id);
@@ -2191,7 +2194,7 @@ void ChildProcessSecurityPolicyImpl::LogKilledProcessOriginLock(int child_id) {
 
 ChildProcessSecurityPolicyImpl::Handle
 ChildProcessSecurityPolicyImpl::CreateHandle(int child_id) {
-  return Handle(child_id);
+  return Handle(child_id, /* duplicating_handle */ false);
 }
 
 // static
@@ -2221,17 +2224,36 @@ ChildProcessSecurityPolicyImpl::ScopedOriginIsolationOptInRequest::
   instance->scoped_isolation_request_origin_ = base::nullopt;
 }
 
-bool ChildProcessSecurityPolicyImpl::AddProcessReference(int child_id) {
+bool ChildProcessSecurityPolicyImpl::AddProcessReference(
+    int child_id,
+    bool duplicating_handle) {
   base::AutoLock lock(lock_);
-  return AddProcessReferenceLocked(child_id);
+  return AddProcessReferenceLocked(child_id, duplicating_handle);
 }
 
-bool ChildProcessSecurityPolicyImpl::AddProcessReferenceLocked(int child_id) {
-  // Make sure that we aren't trying to add references after the process has
-  // been destroyed.
-  if (security_state_.find(child_id) == security_state_.end())
+bool ChildProcessSecurityPolicyImpl::AddProcessReferenceLocked(
+    int child_id,
+    bool duplicating_handle) {
+  if (child_id == ChildProcessHost::kInvalidUniqueID)
     return false;
 
+  // Check to see if the SecurityState has been removed from |security_state_|
+  // via a Remove() call. This corresponds to the process being destroyed.
+  if (security_state_.find(child_id) == security_state_.end()) {
+    if (!duplicating_handle) {
+      // Do not allow Handles to be created after the process has been
+      // destroyed, unless they are being duplicated.
+      return false;
+    }
+
+    // The process has been destroyed but we are allowing an existing Handle
+    // to be duplicated. Verify that the process reference count is available
+    // and indicates another Handle has a reference.
+    auto itr = process_reference_counts_.find(child_id);
+    CHECK(itr != process_reference_counts_.end());
+    CHECK_GT(itr->second, 0);
+  }
+
   ++process_reference_counts_[child_id];
   return true;
 }

content/browser/child_process_security_policy_impl.h

@@ -111,7 +111,14 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
 
    private:
     friend class ChildProcessSecurityPolicyImpl;
-    explicit Handle(int child_id);
+    // |child_id| - The ID of the process that this Handle is being created
+    // for, or ChildProcessHost::kInvalidUniqueID if an invalid handle is being
+    // created.
+    // |duplicating_handle| - True if the handle is being created by a
+    // Duplicate() call. Otherwise false. This is used to trigger special
+    // behavior for handle duplication that is not allowed for Handles created
+    // by other means.
+    Handle(int child_id, bool duplicating_handle);
 
     // The ID of the child process that this handle is associated with or
     // ChildProcessHost::kInvalidUniqueID if the handle is no longer valid.
@@ -659,8 +666,9 @@ class CONTENT_EXPORT ChildProcessSecurityPolicyImpl
                           IsolatedOriginSource source,
                           BrowserContext* browser_context = nullptr);
 
-  bool AddProcessReference(int child_id);
-  bool AddProcessReferenceLocked(int child_id) EXCLUSIVE_LOCKS_REQUIRED(lock_);
+  bool AddProcessReference(int child_id, bool duplicating_handle);
+  bool AddProcessReferenceLocked(int child_id, bool duplicating_handle)
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
   void RemoveProcessReference(int child_id);
   void RemoveProcessReferenceLocked(int child_id)
       EXCLUSIVE_LOCKS_REQUIRED(lock_);

content/browser/child_process_security_policy_unittest.cc

@@ -1337,6 +1337,45 @@ TEST_F(ChildProcessSecurityPolicyTest, HandleExtendsSecurityStateLifetime) {
   EXPECT_FALSE(ui_after_handle_invalidation);
 }
 
+TEST_F(ChildProcessSecurityPolicyTest, HandleDuplicate) {
+  ChildProcessSecurityPolicyImpl* p =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  GURL url("file:///etc/passwd");
+
+  p->Add(kRendererID, browser_context());
+  LockProcessIfNeeded(kRendererID, browser_context(), url);
+
+  auto handle = p->CreateHandle(kRendererID);
+
+  EXPECT_TRUE(handle.CanAccessDataForOrigin(url));
+
+  // Verify that a valid duplicate can be created and allows access.
+  auto duplicate_handle = handle.Duplicate();
+  EXPECT_TRUE(duplicate_handle.is_valid());
+  EXPECT_TRUE(duplicate_handle.CanAccessDataForOrigin(url));
+
+  p->Remove(kRendererID);
+
+  // Verify that both handles still work even after Remove() has been called.
+  EXPECT_TRUE(handle.CanAccessDataForOrigin(url));
+  EXPECT_TRUE(duplicate_handle.CanAccessDataForOrigin(url));
+
+  // Verify that a new duplicate can be created after Remove().
+  auto duplicate_handle2 = handle.Duplicate();
+  EXPECT_TRUE(duplicate_handle2.is_valid());
+  EXPECT_TRUE(duplicate_handle2.CanAccessDataForOrigin(url));
+
+  // Verify that a new valid Handle cannot be created after Remove().
+  EXPECT_FALSE(p->CreateHandle(kRendererID).is_valid());
+
+  // Invalidate the original Handle and verify that the duplicates still work.
+  handle = ChildProcessSecurityPolicyImpl::Handle();
+  EXPECT_FALSE(handle.CanAccessDataForOrigin(url));
+  EXPECT_TRUE(duplicate_handle.CanAccessDataForOrigin(url));
+  EXPECT_TRUE(duplicate_handle2.CanAccessDataForOrigin(url));
+}
+
 TEST_F(ChildProcessSecurityPolicyTest, CanAccessDataForOrigin_URL) {
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();