Replace security state workaround in CanAccessDataForOrigin()

- Replace workaround with code that is more strict about enforcing
  security policy during child process shutdown. The old code would
  always allow data access for IDs not in the security_state_ map. The
  new code adds a pending map so we can deal with UI/IO thread races
  during child process removal AND rejects any unknown IDs.

- Fixed a test that depended on the old behavior where unknown IDs
  always allowed access.

Bug: 898281, 600441, 915203
Change-Id: I26ca1e48536672b05d2310d8a17be47d5b6ef5c7
Reviewed-on: https://chromium-review.googlesource.com/c/1382855
Commit-Queue: Aaron Colwell <acolwell@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617937}
diff --git a/content/browser/child_process_security_policy_impl.cc b/content/browser/child_process_security_policy_impl.cc
index c3c9689..41d7b99 100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -441,8 +441,26 @@
 }
 
 void ChildProcessSecurityPolicyImpl::Remove(int child_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   base::AutoLock lock(lock_);
+
+  auto state = security_state_.find(child_id);
+  if (state == security_state_.end())
+    return;
+
+  pending_remove_state_[child_id] = std::move(state->second);
   security_state_.erase(child_id);
+
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO},
+      base::BindOnce(&ChildProcessSecurityPolicyImpl::RemovePendingIDOnIOThread,
+                     base::Unretained(this), child_id));
+}
+
+void ChildProcessSecurityPolicyImpl::RemovePendingIDOnIOThread(int child_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  base::AutoLock lock(lock_);
+  pending_remove_state_.erase(child_id);
 }
 
 void ChildProcessSecurityPolicyImpl::RegisterWebSafeScheme(
@@ -1151,21 +1169,20 @@
       SiteInstanceImpl::DetermineProcessLockURL(nullptr, url);
 
   base::AutoLock lock(lock_);
-  auto state = security_state_.find(child_id);
-  if (state == security_state_.end()) {
-    // TODO(nick): Returning true instead of false here is a temporary
-    // workaround for https://crbug.com/600441
-    return true;
-  }
-  bool can_access =
-      state->second->CanAccessDataForOrigin(expected_process_lock);
+  SecurityState* security_state = GetSecurityState(child_id);
+
+  bool can_access = security_state && security_state->CanAccessDataForOrigin(
+                                          expected_process_lock);
   if (!can_access) {
     // Returning false here will result in a renderer kill.  Set some crash
     // keys that will help understand the circumstances of that kill.
     base::debug::SetCrashKeyString(bad_message::GetRequestedSiteURLKey(),
                                    expected_process_lock.spec());
+
     base::debug::SetCrashKeyString(bad_message::GetKilledProcessOriginLockKey(),
-                                   state->second->origin_lock().spec());
+                                   security_state
+                                       ? security_state->origin_lock().spec()
+                                       : "(child id not found)");
 
     static auto* requested_origin_key = base::debug::AllocateCrashKeyString(
         "requested_origin", base::debug::CrashKeySize::Size64);
@@ -1367,4 +1384,22 @@
     isolated_origins_.erase(key);
 }
 
+ChildProcessSecurityPolicyImpl::SecurityState*
+ChildProcessSecurityPolicyImpl::GetSecurityState(int child_id) {
+  auto itr = security_state_.find(child_id);
+  if (itr != security_state_.end())
+    return itr->second.get();
+
+  if (BrowserThread::CurrentlyOn(BrowserThread::IO)) {
+    // Checking to see if |child_id| is in the pending removal map since this
+    // may be a call that was already on the IO thread task queue when the
+    // Remove() call occurred on the UI thread.
+    itr = pending_remove_state_.find(child_id);
+    if (itr != pending_remove_state_.end())
+      return itr->second.get();
+  }
+
+  return nullptr;
+}
+
 }  // namespace content
diff --git a/content/browser/child_process_security_policy_impl.h b/content/browser/child_process_security_policy_impl.h
index 82d5dd4..271a5685 100644
--- a/content/browser/child_process_security_policy_impl.h
+++ b/content/browser/child_process_security_policy_impl.h
@@ -162,6 +162,10 @@
 
   // Upon destruction, child processess should unregister themselves by caling
   // this method exactly once.
+  //
+  // Note: IO thread is expected to keep pre-Remove() permissions until
+  // RemovePendingIDOnIOThread() runs on the IO thread. The UI thread is
+  // expected to have no permissions after Remove() returns.
   void Remove(int child_id);
 
   // Whenever the browser processes commands the child process to commit a URL,
@@ -367,6 +371,14 @@
       const std::string& filesystem_id,
       int permission);
 
+  // Removes |child_id| from |pending_remove_state_| on the IO thread.
+  void RemovePendingIDOnIOThread(int child_id);
+
+  // Gets the SecurityState object associated with |child_id|.
+  // Note: Returned object is only valid for the duration the caller holds
+  // |lock_|.
+  SecurityState* GetSecurityState(int child_id) EXCLUSIVE_LOCKS_REQUIRED(lock_);
+
   // You must acquire this lock before reading or writing any members of this
   // class.  You must not block while holding this lock.
   base::Lock lock_;
@@ -388,6 +400,13 @@
   // not escape this class.
   SecurityStateMap security_state_ GUARDED_BY(lock_);
 
+  // This map holds the SecurityState for a child process after Remove()
+  // is called on the UI thread and until RemovePendingIDOnIOThread() is
+  // called on the IO thread. This is necessary to provide consistent
+  // security decisions and avoid races between the UI & IO threads during
+  // child process shutdown.
+  SecurityStateMap pending_remove_state_ GUARDED_BY(lock_);
+
   FileSystemPermissionPolicyMap file_system_policy_map_ GUARDED_BY(lock_);
 
   // Tracks origins for which the entire origin should be treated as a site
diff --git a/content/browser/child_process_security_policy_unittest.cc b/content/browser/child_process_security_policy_unittest.cc
index 3101bc1..3d43ebc 100644
--- a/content/browser/child_process_security_policy_unittest.cc
+++ b/content/browser/child_process_security_policy_unittest.cc
@@ -7,6 +7,8 @@
 
 #include "base/files/file_path.h"
 #include "base/logging.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/mock_log.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/site_instance_impl.h"
@@ -59,7 +61,9 @@
 
 class ChildProcessSecurityPolicyTest : public testing::Test {
  public:
-  ChildProcessSecurityPolicyTest() : old_browser_client_(nullptr) {}
+  ChildProcessSecurityPolicyTest()
+      : thread_bundle_(TestBrowserThreadBundle::REAL_IO_THREAD),
+        old_browser_client_(nullptr) {}
 
   void SetUp() override {
     old_browser_client_ = SetBrowserClientForTesting(&test_browser_client_);
@@ -993,6 +997,85 @@
   EXPECT_FALSE(p->HasWebUIBindings(kRendererID));
 }
 
+TEST_F(ChildProcessSecurityPolicyTest, RemoveRace_CanAccessDataForOrigin) {
+  ChildProcessSecurityPolicyImpl* p =
+      ChildProcessSecurityPolicyImpl::GetInstance();
+
+  GURL url("file:///etc/passwd");
+
+  p->Add(kRendererID);
+
+  base::WaitableEvent ready_for_remove_event;
+  base::WaitableEvent remove_called_event;
+  base::WaitableEvent pending_remove_complete_event;
+
+  bool io_before_remove = false;
+  bool io_while_remove_pending = false;
+  bool io_after_remove_complete = false;
+  bool ui_before_remove = false;
+  bool ui_while_remove_pending = false;
+  bool ui_after_remove_complete = false;
+
+  // Post a task that will run on the IO thread before the task that
+  // Remove() will post to the IO thread.
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO}, base::BindLambdaForTesting([&]() {
+        // Capture state on the IO thread before Remove() is called.
+        io_before_remove = p->CanAccessDataForOrigin(kRendererID, url);
+
+        // Tell the UI thread we are ready for Remove() to be called.
+        ready_for_remove_event.Signal();
+
+        // Wait for Remove() to be called on the UI thread.
+        remove_called_event.Wait();
+
+        // Capture state after Remove() is called, but before its task on
+        // the IO thread runs.
+        io_while_remove_pending = p->CanAccessDataForOrigin(kRendererID, url);
+      }));
+
+  ready_for_remove_event.Wait();
+
+  ui_before_remove = p->CanAccessDataForOrigin(kRendererID, url);
+
+  p->Remove(kRendererID);
+
+  // Post a task to run after the task Remove() posted on the IO thread.
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::IO}, base::BindLambdaForTesting([&]() {
+        io_after_remove_complete = p->CanAccessDataForOrigin(kRendererID, url);
+
+        // Tell the UI thread that the task from Remove() has completed on the
+        // IO thread.
+        pending_remove_complete_event.Signal();
+      }));
+
+  // Capture state after Remove() has been called, but before its IO thread
+  // task has run. We know the IO thread task hasn't run yet because the
+  // task we posted before the Remove() call is waiting for us to signal
+  // |remove_called_event|.
+  ui_while_remove_pending = p->CanAccessDataForOrigin(kRendererID, url);
+
+  // Unblock the IO thread so the pending remove events can run.
+  remove_called_event.Signal();
+
+  pending_remove_complete_event.Wait();
+
+  ui_after_remove_complete = p->CanAccessDataForOrigin(kRendererID, url);
+
+  // Verify expected states at various parts of the removal.
+  // Note: IO thread is expected to keep pre-Remove() permissions until its
+  // remove task runs on the IO thread. The UI thread is expected to have no
+  // permissions after Remove() returns.
+  EXPECT_TRUE(io_before_remove);
+  EXPECT_TRUE(io_while_remove_pending);
+  EXPECT_FALSE(io_after_remove_complete);
+
+  EXPECT_TRUE(ui_before_remove);
+  EXPECT_FALSE(ui_while_remove_pending);
+  EXPECT_FALSE(ui_after_remove_complete);
+}
+
 // Test the granting of origin permissions, and their interactions with
 // granting scheme permissions.
 TEST_F(ChildProcessSecurityPolicyTest, OriginGranting) {
diff --git a/content/browser/dom_storage/session_storage_context_mojo_unittest.cc b/content/browser/dom_storage/session_storage_context_mojo_unittest.cc
index c701b61..ebf479c 100644
--- a/content/browser/dom_storage/session_storage_context_mojo_unittest.cc
+++ b/content/browser/dom_storage/session_storage_context_mojo_unittest.cc
@@ -17,6 +17,7 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/services/leveldb/public/cpp/util.h"
+#include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/dom_storage/session_storage_database.h"
 #include "content/browser/dom_storage/test/fake_leveldb_database_error_on_write.h"
 #include "content/browser/dom_storage/test/fake_leveldb_service.h"
@@ -62,9 +63,13 @@
     features_.InitAndEnableFeature(blink::features::kOnionSoupDOMStorage);
     mojo::core::SetDefaultProcessErrorCallback(base::BindRepeating(
         &SessionStorageContextMojoTest::OnBadMessage, base::Unretained(this)));
+
+    ChildProcessSecurityPolicyImpl::GetInstance()->Add(kTestProcessId);
   }
 
   void TearDown() override {
+    ChildProcessSecurityPolicyImpl::GetInstance()->Remove(kTestProcessId);
+
     mojo::core::SetDefaultProcessErrorCallback(
         mojo::core::ProcessErrorCallback());
   }
diff --git a/content/browser/dom_storage/test/mojo_test_with_file_service.h b/content/browser/dom_storage/test/mojo_test_with_file_service.h
index 77d6eae..8ae2bbb 100644
--- a/content/browser/dom_storage/test/mojo_test_with_file_service.h
+++ b/content/browser/dom_storage/test/mojo_test_with_file_service.h
@@ -10,7 +10,7 @@
 #include "base/files/file_path.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
-#include "base/test/scoped_task_environment.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "services/file/file_service.h"
 #include "services/service_manager/public/cpp/test/test_connector_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -34,10 +34,10 @@
     return test_connector_factory_.GetDefaultConnector();
   }
 
-  void RunUntilIdle() { scoped_task_environment_.RunUntilIdle(); }
+  void RunUntilIdle() { thread_bundle_.RunUntilIdle(); }
 
  private:
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  TestBrowserThreadBundle thread_bundle_;
   service_manager::TestConnectorFactory test_connector_factory_;
   file::FileService file_service_;
   base::ScopedTempDir temp_path_;
diff --git a/content/browser/fileapi/browser_file_system_helper_unittest.cc b/content/browser/fileapi/browser_file_system_helper_unittest.cc
index 1b0cff4..67e528b5 100644
--- a/content/browser/fileapi/browser_file_system_helper_unittest.cc
+++ b/content/browser/fileapi/browser_file_system_helper_unittest.cc
@@ -13,6 +13,7 @@
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/fileapi/browser_file_system_helper.h"
 #include "content/public/common/drop_data.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/filename_util.h"
 #include "storage/browser/fileapi/external_mount_points.h"
 #include "storage/browser/fileapi/file_system_options.h"
@@ -33,6 +34,7 @@
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
+  TestBrowserThreadBundle thread_bundle;
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
   p->Add(kRendererID);
@@ -136,6 +138,7 @@
   base::ScopedTempDir temp_dir;
   ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
 
+  TestBrowserThreadBundle thread_bundle;
   ChildProcessSecurityPolicyImpl* p =
       ChildProcessSecurityPolicyImpl::GetInstance();
   p->Add(kRendererID);
diff --git a/content/browser/network_service_client_unittest.cc b/content/browser/network_service_client_unittest.cc
index 5d6e021..de2c0047 100644
--- a/content/browser/network_service_client_unittest.cc
+++ b/content/browser/network_service_client_unittest.cc
@@ -13,6 +13,7 @@
 #include "base/test/test_file_util.h"
 #include "build/build_config.h"
 #include "content/browser/child_process_security_policy_impl.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
@@ -76,7 +77,7 @@
   }
 
  protected:
-  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  TestBrowserThreadBundle scoped_task_environment_;
   network::mojom::NetworkServiceClientPtr client_ptr_;
   NetworkServiceClient client_;
   base::ScopedTempDir temp_dir_;