Grant all renderers all permissions to the file system directory, and

update the permissions check when a AsyncOpenFile message is received.

BUG=none
TEST=none


Review URL: http://codereview.chromium.org/3522003

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

7 files changed, 57 insertions, 21 deletions

diff --git a/chrome/browser/child_process_security_policy.cc b/chrome/browser/child_process_security_policy.cc
index 5068e57..67b8a7c 100644
--- a/chrome/browser/child_process_security_policy.cc
+++ b/chrome/browser/child_process_security_policy.cc
@@ -14,7 +14,7 @@
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request.h"
 
-const int kReadFilePermissions =
+static const int kReadFilePermissions =
     base::PLATFORM_FILE_OPEN |
     base::PLATFORM_FILE_READ |
     base::PLATFORM_FILE_EXCLUSIVE_READ |
@@ -46,6 +46,11 @@ class ChildProcessSecurityPolicy::SecurityState {
     file_permissions_[file.StripTrailingSeparators()] |= permissions;
   }
 
+  // Revokes all permissions granted to a file.
+  void RevokeAllPermissionsForFile(const FilePath& file) {
+    file_permissions_.erase(file.StripTrailingSeparators());
+  }
+
   void GrantBindings(int bindings) {
     enabled_bindings_ |= bindings;
   }
@@ -97,7 +102,7 @@ class ChildProcessSecurityPolicy::SecurityState {
 
  private:
   typedef std::map<std::string, bool> SchemeMap;
-  typedef std::map<FilePath, int> FileMap; // bit-set of PlatformFileFlags
+  typedef std::map<FilePath, int> FileMap;  // bit-set of PlatformFileFlags
 
   // Maps URL schemes to whether permission has been granted or revoked:
   //   |true| means the scheme has been granted.
@@ -246,6 +251,17 @@ void ChildProcessSecurityPolicy::GrantPermissionsForFile(
   state->second->GrantPermissionsForFile(file, permissions);
 }
 
+void ChildProcessSecurityPolicy::RevokeAllPermissionsForFile(
+    int renderer_id, const FilePath& file) {
+  AutoLock lock(lock_);
+
+  SecurityStateMap::iterator state = security_state_.find(renderer_id);
+  if (state == security_state_.end())
+    return;
+
+  state->second->RevokeAllPermissionsForFile(file);
+}
+
 void ChildProcessSecurityPolicy::GrantScheme(int renderer_id,
                                              const std::string& scheme) {
   AutoLock lock(lock_);
diff --git a/chrome/browser/child_process_security_policy.h b/chrome/browser/child_process_security_policy.h
index 6ae4f30..fc8814a 100644
--- a/chrome/browser/child_process_security_policy.h
+++ b/chrome/browser/child_process_security_policy.h
@@ -77,6 +77,9 @@ class ChildProcessSecurityPolicy {
                                const FilePath& file,
                                int permissions);
 
+  // Revokes all permissions granted to the given file.
+  void RevokeAllPermissionsForFile(int renderer_id, const FilePath& file);
+
   // Grants the renderer process the capability to access URLs of the provided
   // scheme.
   void GrantScheme(int renderer_id, const std::string& scheme);
@@ -108,7 +111,7 @@ class ChildProcessSecurityPolicy {
   // capability to upload the requested file.
   bool CanReadFile(int renderer_id, const FilePath& file);
 
-  // Determins if certain permissions were granted for a file. |permissions|
+  // Determines if certain permissions were granted for a file. |permissions|
   // must be a bit-set of base::PlatformFileFlags.
   bool HasPermissionsForFile(int renderer_id,
                              const FilePath& file,
diff --git a/chrome/browser/child_process_security_policy_unittest.cc b/chrome/browser/child_process_security_policy_unittest.cc
index 949265d..40b249b 100644
--- a/chrome/browser/child_process_security_policy_unittest.cc
+++ b/chrome/browser/child_process_security_policy_unittest.cc
@@ -275,6 +275,15 @@ TEST_F(ChildProcessSecurityPolicyTest, FilePermissions) {
                                         base::PLATFORM_FILE_OPEN));
   EXPECT_TRUE(p->HasPermissionsForFile(kRendererID, file,
                                        base::PLATFORM_FILE_TEMPORARY));
+
+  // Revoke all permissions for the file (it should inherit its permissions
+  // from the directory again).
+  p->RevokeAllPermissionsForFile(kRendererID, file);
+  EXPECT_TRUE(p->HasPermissionsForFile(kRendererID, file,
+                                       base::PLATFORM_FILE_OPEN |
+                                       base::PLATFORM_FILE_READ));
+  EXPECT_FALSE(p->HasPermissionsForFile(kRendererID, file,
+                                        base::PLATFORM_FILE_TEMPORARY));
   p->Remove(kRendererID);
 }
 
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index 8196f14..b2bd5d9 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -20,6 +20,7 @@
 #include "base/field_trial.h"
 #include "base/histogram.h"
 #include "base/logging.h"
+#include "base/platform_file.h"
 #include "base/stl_util-inl.h"
 #include "base/string_util.h"
 #include "base/thread.h"
@@ -30,6 +31,7 @@
 #include "chrome/browser/extensions/extension_message_service.h"
 #include "chrome/browser/extensions/extensions_service.h"
 #include "chrome/browser/extensions/user_script_master.h"
+#include "chrome/browser/file_system/file_system_host_context.h"
 #include "chrome/browser/gpu_process_host.h"
 #include "chrome/browser/history/history.h"
 #include "chrome/browser/io_thread.h"
@@ -223,6 +225,25 @@ BrowserRenderProcessHost::BrowserRenderProcessHost(Profile* profile)
   WebCacheManager::GetInstance()->Add(id());
   ChildProcessSecurityPolicy::GetInstance()->Add(id());
 
+  // Grant most file permissions to this renderer.
+  // PLATFORM_FILE_TEMPORARY, PLATFORM_FILE_HIDDEN and
+  // PLATFORM_FILE_DELETE_ON_CLOSE are not granted, because no existing API
+  // requests them.
+  ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
+      id(), profile->GetPath().Append(
+          FileSystemHostContext::kFileSystemDirectory),
+      base::PLATFORM_FILE_OPEN |
+      base::PLATFORM_FILE_CREATE |
+      base::PLATFORM_FILE_OPEN_ALWAYS |
+      base::PLATFORM_FILE_CREATE_ALWAYS |
+      base::PLATFORM_FILE_READ |
+      base::PLATFORM_FILE_WRITE |
+      base::PLATFORM_FILE_EXCLUSIVE_READ |
+      base::PLATFORM_FILE_EXCLUSIVE_WRITE |
+      base::PLATFORM_FILE_ASYNC |
+      base::PLATFORM_FILE_TRUNCATE |
+      base::PLATFORM_FILE_WRITE_ATTRIBUTES);
+
   // Note: When we create the BrowserRenderProcessHost, it's technically
   //       backgrounded, because it has no visible listeners.  But the process
   //       doesn't actually exist yet, so we'll Background it later, after
diff --git a/chrome/browser/renderer_host/redirect_to_file_resource_handler.cc b/chrome/browser/renderer_host/redirect_to_file_resource_handler.cc
index 94a5c77..1843514 100644
--- a/chrome/browser/renderer_host/redirect_to_file_resource_handler.cc
+++ b/chrome/browser/renderer_host/redirect_to_file_resource_handler.cc
@@ -9,7 +9,6 @@
 #include "base/logging.h"
 #include "base/platform_file.h"
 #include "base/task.h"
-#include "chrome/browser/child_process_security_policy.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/common/resource_response.h"
 #include "net/base/file_stream.h"
diff --git a/chrome/browser/renderer_host/resource_dispatcher_host.cc b/chrome/browser/renderer_host/resource_dispatcher_host.cc
index 9126eb9..1296df1 100644
--- a/chrome/browser/renderer_host/resource_dispatcher_host.cc
+++ b/chrome/browser/renderer_host/resource_dispatcher_host.cc
@@ -591,8 +591,10 @@ void ResourceDispatcherHost::UnregisterDownloadedTempFile(
   DeletableFilesMap::iterator found = map.find(request_id);
   if (found == map.end())
     return;
+
+  ChildProcessSecurityPolicy::GetInstance()->RevokeAllPermissionsForFile(
+      receiver_id, found->second->path());
   map.erase(found);
-  // TODO(michaeln): Revoke access to this file.
 }
 
 bool ResourceDispatcherHost::Send(IPC::Message* message) {
diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc
index 2f42aab..c1f7d13 100644
--- a/chrome/browser/renderer_host/resource_message_filter.cc
+++ b/chrome/browser/renderer_host/resource_message_filter.cc
@@ -1722,22 +1722,8 @@ void ResourceMessageFilter::OnAsyncOpenFile(const IPC::Message& msg,
                                             int message_id) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  if (!ChildProcessSecurityPolicy::GetInstance()->CanReadFile(id(), path)) {
-    IPC::Message* reply = new ViewMsg_AsyncOpenFile_ACK(
-        msg.routing_id(), base::PLATFORM_FILE_ERROR_ACCESS_DENIED,
-        IPC::InvalidPlatformFileForTransit(), message_id);
-    Send(reply);
-    return;
-  }
-
-  // TODO(dumi): update this check once we have a security attribute
-  // that allows renderers to modify files.
-  int allowed_flags =
-      base::PLATFORM_FILE_OPEN |
-      base::PLATFORM_FILE_READ |
-      base::PLATFORM_FILE_EXCLUSIVE_READ |
-      base::PLATFORM_FILE_ASYNC;
-  if (flags & ~allowed_flags) {
+  if (!ChildProcessSecurityPolicy::GetInstance()->HasPermissionsForFile(
+          id(), path, flags)) {
     DLOG(ERROR) << "Bad flags in ViewMsgHost_AsyncOpenFile message: " << flags;
     BrowserRenderProcessHost::BadMessageTerminateProcess(
         ViewHostMsg_AsyncOpenFile::ID, handle());