blob_storage_controller.cc assert from this bug was caused by the fact that worker thread actually run in a different renderer process from the main page JS thread. chrome.fileBrowserPrivate.* methods grant access to files through ChildProcessSecurityPolicy class, but such file permissions would end up associated with renderer process of the main thread only. When worker tries to register blobs representing such files, ChildProcessSecurityPolicy check would reject it since its client process id has nothing to do with main renderer's id.

This CL adds mapping between worker and main renderer processes and uses that to ensure that worker thread renderer process file permissions are "inherited" from its main JS thread renderer child process.

BUG=chromium-os:14680
TEST=added extra tests to ChildProcessSecurityPolicyTest.FilePermissions
Review URL: http://codereview.chromium.org/6893145

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@83754 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 83 insertions, 13 deletions

diff --git a/content/browser/child_process_security_policy.cc b/content/browser/child_process_security_policy.cc
index cda7a06..eac07c4 100644
--- a/content/browser/child_process_security_policy.cc
+++ b/content/browser/child_process_security_policy.cc
@@ -157,12 +157,14 @@ ChildProcessSecurityPolicy* ChildProcessSecurityPolicy::GetInstance() {
 
 void ChildProcessSecurityPolicy::Add(int child_id) {
   base::AutoLock lock(lock_);
-  if (security_state_.count(child_id) != 0) {
-    NOTREACHED() << "Add child process at most once.";
-    return;
-  }
+  AddChild(child_id);
+}
 
-  security_state_[child_id] = new SecurityState();
+void ChildProcessSecurityPolicy::AddWorker(int child_id,
+                                           int main_render_process_id) {
+  base::AutoLock lock(lock_);
+  AddChild(child_id);
+  worker_map_[child_id] = main_render_process_id;
 }
 
 void ChildProcessSecurityPolicy::Remove(int child_id) {
@@ -172,6 +174,7 @@ void ChildProcessSecurityPolicy::Remove(int child_id) {
 
   delete security_state_[child_id];
   security_state_.erase(child_id);
+  worker_map_.erase(child_id);
 }
 
 void ChildProcessSecurityPolicy::RegisterWebSafeScheme(
@@ -405,12 +408,18 @@ bool ChildProcessSecurityPolicy::CanReadDirectory(int child_id,
 bool ChildProcessSecurityPolicy::HasPermissionsForFile(
     int child_id, const FilePath& file, int permissions) {
   base::AutoLock lock(lock_);
-
-  SecurityStateMap::iterator state = security_state_.find(child_id);
-  if (state == security_state_.end())
-    return false;
-
-  return state->second->HasPermissionsForFile(file, permissions);
+  bool result = ChildProcessHasPermissionsForFile(child_id, file, permissions);
+  if (!result) {
+    // If this is a worker thread that has no access to a given file,
+    // let's check that its renderer process has access to that file instead.
+    WorkerToMainProcessMap::iterator iter = worker_map_.find(child_id);
+    if (iter != worker_map_.end() && iter->second != 0) {
+      result = ChildProcessHasPermissionsForFile(iter->second,
+                                                 file,
+                                                 permissions);
+    }
+  }
+  return result;
 }
 
 bool ChildProcessSecurityPolicy::HasWebUIBindings(int child_id) {
@@ -442,3 +451,20 @@ bool ChildProcessSecurityPolicy::CanReadRawCookies(int child_id) {
 
   return state->second->can_read_raw_cookies();
 }
+
+void ChildProcessSecurityPolicy::AddChild(int child_id) {
+  if (security_state_.count(child_id) != 0) {
+    NOTREACHED() << "Add child process at most once.";
+    return;
+  }
+
+  security_state_[child_id] = new SecurityState();
+}
+
+bool ChildProcessSecurityPolicy::ChildProcessHasPermissionsForFile(
+    int child_id, const FilePath& file, int permissions) {
+  SecurityStateMap::iterator state = security_state_.find(child_id);
+  if (state == security_state_.end())
+    return false;
+  return state->second->HasPermissionsForFile(file, permissions);
+}
diff --git a/content/browser/child_process_security_policy.h b/content/browser/child_process_security_policy.h
index da8e924..2edb81c 100644
--- a/content/browser/child_process_security_policy.h
+++ b/content/browser/child_process_security_policy.h
@@ -66,6 +66,12 @@ class ChildProcessSecurityPolicy {
   // this method exactly once.
   void Add(int child_id);
 
+  // Upon creation, worker thread child processes should register themselves by
+  // calling this this method exactly once. Workers that are not shared will
+  // inherit permissions from their parent renderer process identified with
+  // |main_render_process_id|.
+  void AddWorker(int worker_child_id, int main_render_process_id);
+
   // Upon destruction, child processess should unregister themselves by caling
   // this method exactly once.
   void Remove(int child_id);
@@ -151,12 +157,22 @@ class ChildProcessSecurityPolicy {
 
   typedef std::set<std::string> SchemeSet;
   typedef std::map<int, SecurityState*> SecurityStateMap;
+  typedef std::map<int, int> WorkerToMainProcessMap;
 
   // Obtain an instance of ChildProcessSecurityPolicy via GetInstance().
   ChildProcessSecurityPolicy();
   friend struct DefaultSingletonTraits<ChildProcessSecurityPolicy>;
 
-  // You must acquire this lock before reading or writing any members of this
+  // Adds child process during registration.
+  void AddChild(int child_id);
+
+  // Determines if certain permissions were granted for a file to given child
+  // process. |permissions| must be a bit-set of base::PlatformFileFlags.
+  bool ChildProcessHasPermissionsForFile(int child_id,
+                                         const FilePath& file,
+                                         int permissions);
+
+    // 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_;
 
@@ -180,6 +196,10 @@ class ChildProcessSecurityPolicy {
   // 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_;
+
   DISALLOW_COPY_AND_ASSIGN(ChildProcessSecurityPolicy);
 };
 
diff --git a/content/browser/child_process_security_policy_unittest.cc b/content/browser/child_process_security_policy_unittest.cc
index 53799b7..0ee30f8 100644
--- a/content/browser/child_process_security_policy_unittest.cc
+++ b/content/browser/child_process_security_policy_unittest.cc
@@ -27,6 +27,7 @@ class ChildProcessSecurityPolicyTest : public testing::Test {
 };
 
 static int kRendererID = 42;
+static int kWorkerRendererID = kRendererID + 1;
 
 TEST_F(ChildProcessSecurityPolicyTest, IsWebSafeSchemeTest) {
   ChildProcessSecurityPolicy* p = ChildProcessSecurityPolicy::GetInstance();
@@ -349,6 +350,28 @@ TEST_F(ChildProcessSecurityPolicyTest, FilePermissions) {
   EXPECT_FALSE(p->HasPermissionsForFile(kRendererID, file,
                                         base::PLATFORM_FILE_TEMPORARY));
   p->Remove(kRendererID);
+
+  // Grant file permissions for the file to main thread renderer process,
+  // make sure its worker thread renderer process inherits those.
+  p->Add(kRendererID);
+  p->GrantPermissionsForFile(kRendererID, file, base::PLATFORM_FILE_OPEN |
+                                                base::PLATFORM_FILE_READ);
+  EXPECT_TRUE(p->HasPermissionsForFile(kRendererID, file,
+                                       base::PLATFORM_FILE_OPEN |
+                                       base::PLATFORM_FILE_READ));
+  EXPECT_FALSE(p->HasPermissionsForFile(kRendererID, file,
+                                       base::PLATFORM_FILE_WRITE));
+  p->AddWorker(kWorkerRendererID, kRendererID);
+  EXPECT_TRUE(p->HasPermissionsForFile(kWorkerRendererID, file,
+                                       base::PLATFORM_FILE_OPEN |
+                                       base::PLATFORM_FILE_READ));
+  EXPECT_FALSE(p->HasPermissionsForFile(kWorkerRendererID, file,
+                                        base::PLATFORM_FILE_WRITE));
+  p->Remove(kRendererID);
+  EXPECT_FALSE(p->HasPermissionsForFile(kWorkerRendererID, file,
+                                        base::PLATFORM_FILE_OPEN |
+                                        base::PLATFORM_FILE_READ));
+  p->Remove(kWorkerRendererID);
 }
 
 TEST_F(ChildProcessSecurityPolicyTest, CanServiceWebUIBindings) {
diff --git a/content/browser/worker_host/worker_process_host.cc b/content/browser/worker_host/worker_process_host.cc
index 82ec1cf..27a45178 100644
--- a/content/browser/worker_host/worker_process_host.cc
+++ b/content/browser/worker_host/worker_process_host.cc
@@ -177,7 +177,8 @@ bool WorkerProcessHost::Init(int render_process_id) {
 #endif
       cmd_line);
 
-  ChildProcessSecurityPolicy::GetInstance()->Add(id());
+  ChildProcessSecurityPolicy::GetInstance()->AddWorker(
+      id(), render_process_id);
   if (!CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kDisableFileSystem)) {
       // Grant most file permissions to this worker.