ChildProcessSecurityPolicy: Port FileAPIMessageFilter to use new checks

content/browser/fileapi/fileapi_message_filter.cc

Index: content/browser/fileapi/fileapi_message_filter.cc
diff --git a/content/browser/fileapi/fileapi_message_filter.cc b/content/browser/fileapi/fileapi_message_filter.cc
index b4b5aefc866fdaa0d599b4f093b8f8cf462f43fe..b796e1b166e192af1b35b5b4bcca58dc901d3c83 100644
--- a/content/browser/fileapi/fileapi_message_filter.cc
+++ b/content/browser/fileapi/fileapi_message_filter.cc
@@ -40,6 +40,11 @@
 #include "webkit/common/fileapi/file_system_types.h"
 #include "webkit/common/fileapi/file_system_util.h"
 
+#if defined(ENABLE_PLUGINS)
+#include "content/browser/renderer_host/pepper/pepper_security_helper.h"
+#include "ppapi/shared_impl/file_type_conversion.h"
+#endif  // defined(ENABLE_PLUGINS)
+
 using fileapi::FileSystemFileUtil;
 using fileapi::FileSystemBackend;
 using fileapi::FileSystemOperation;
@@ -68,6 +73,7 @@ FileAPIMessageFilter::FileAPIMessageFilter(
     StreamContext* stream_context)
     : process_id_(process_id),
       context_(file_system_context),
+      security_policy_(ChildProcessSecurityPolicyImpl::GetInstance()),
       request_context_getter_(request_context_getter),
       request_context_(NULL),
       blob_storage_context_(blob_storage_context),
@@ -86,6 +92,7 @@ FileAPIMessageFilter::FileAPIMessageFilter(
     StreamContext* stream_context)
     : process_id_(process_id),
       context_(file_system_context),
+      security_policy_(ChildProcessSecurityPolicyImpl::GetInstance()),
       request_context_(request_context),
       blob_storage_context_(blob_storage_context),
       stream_context_(stream_context) {
@@ -171,7 +178,9 @@ bool FileAPIMessageFilter::OnMessageReceived(
     IPC_MESSAGE_HANDLER(FileSystemHostMsg_Truncate, OnTruncate)
     IPC_MESSAGE_HANDLER(FileSystemHostMsg_TouchFile, OnTouchFile)
     IPC_MESSAGE_HANDLER(FileSystemHostMsg_CancelWrite, OnCancel)
-    IPC_MESSAGE_HANDLER(FileSystemHostMsg_OpenFile, OnOpenFile)
+#if defined(ENABLE_PLUGINS)
+    IPC_MESSAGE_HANDLER(FileSystemHostMsg_OpenPepperFile, OnOpenPepperFile)
+#endif  // defined(ENABLE_PLUGINS)
     IPC_MESSAGE_HANDLER(FileSystemHostMsg_NotifyCloseFile, OnNotifyCloseFile)
     IPC_MESSAGE_HANDLER(FileSystemHostMsg_CreateSnapshotFile,
                         OnCreateSnapshotFile)
@@ -239,15 +248,19 @@ void FileAPIMessageFilter::OnDeleteFileSystem(
 void FileAPIMessageFilter::OnMove(
     int request_id, const GURL& src_path, const GURL& dest_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL src_url(context_->CrackURL(src_path));
   FileSystemURL dest_url(context_->CrackURL(dest_path));
-  const int src_permissions =
-      fileapi::kReadFilePermissions | fileapi::kWriteFilePermissions;
-  if (!HasPermissionsForFile(src_url, src_permissions, &error) ||
-      !HasPermissionsForFile(
-          dest_url, fileapi::kCreateFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, src_url) ||
+      !FileSystemURLIsValid(context_, dest_url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, src_url) ||
+      !security_policy_->CanWriteFileSystemFile(process_id_, src_url) ||
+      !security_policy_->CanCreateFileSystemFile(process_id_, dest_url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -259,13 +272,18 @@ void FileAPIMessageFilter::OnMove(
 void FileAPIMessageFilter::OnCopy(
     int request_id, const GURL& src_path, const GURL& dest_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL src_url(context_->CrackURL(src_path));
   FileSystemURL dest_url(context_->CrackURL(dest_path));
-  if (!HasPermissionsForFile(src_url, fileapi::kReadFilePermissions, &error) ||
-      !HasPermissionsForFile(
-          dest_url, fileapi::kCreateFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, src_url) ||
+      !FileSystemURLIsValid(context_, dest_url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, src_url) ||
+      !security_policy_->CanCreateFileSystemFile(process_id_, dest_url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -277,10 +295,15 @@ void FileAPIMessageFilter::OnCopy(
 void FileAPIMessageFilter::OnRemove(
     int request_id, const GURL& path, bool recursive) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {

[Tom Sepez, 2013/09/04 22:22:45]

Seems like a shame to repeat this same block of code so often. Can it be
consolidated somewhere?

[tommycli, 2013/09/04 23:21:27]

Is is definitely repetitive. There were two ways I saw that it could be
consolidated.

1. Make CPSP operate off of enums again, which sacrificed type safety.
2. Make three helper functions for CanRead, CanWrite, and CanCreate,
respectively, but they didn't seem to save much code, since many methods call
more than one of these.

Neither seemed all that attractive. Do you have any insight into this?

[kinuko, 2013/09/05 03:49:43]

One way to reduce SLOC would be to add a helper method which also sends DidFail
message when necessary. Not cleanest, but would work:

// This also sends DidFail message when the given url is invalid.
bool FAPMF::ValidateFileSystemURL(const FileSystemURL& url) {
  if (FileSystemURLIsValid(url))
    return true;
  Send(new FileSystemMsg_DidFail(...));
  return false;
}

Then each call site would be:

if (!ValidateFileSystemURL(url))
  return;

[tommycli, 2013/09/06 01:41:43]

Done. Slightly scary to have a subroutine send the message on the caller's
behalf, but it does save a lot of SLOC. Thanks!

+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanWriteFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -292,10 +315,15 @@ void FileAPIMessageFilter::OnRemove(
 void FileAPIMessageFilter::OnReadMetadata(
     int request_id, const GURL& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -307,10 +335,15 @@ void FileAPIMessageFilter::OnCreate(
     int request_id, const GURL& path, bool exclusive,
     bool is_directory, bool recursive) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kCreateFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanCreateFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -328,10 +361,15 @@ void FileAPIMessageFilter::OnCreate(
 void FileAPIMessageFilter::OnExists(
     int request_id, const GURL& path, bool is_directory) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -349,10 +387,15 @@ void FileAPIMessageFilter::OnExists(
 void FileAPIMessageFilter::OnReadDirectory(
     int request_id, const GURL& path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -374,9 +417,14 @@ void FileAPIMessageFilter::OnWrite(
   }
 
   FileSystemURL url(context_->CrackURL(path));
-  base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanWriteFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -389,10 +437,15 @@ void FileAPIMessageFilter::OnTruncate(
     int request_id,
     const GURL& path,
     int64 length) {
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, fileapi::kWriteFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanWriteFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -408,9 +461,14 @@ void FileAPIMessageFilter::OnTouchFile(
     const base::Time& last_modified_time) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   FileSystemURL url(context_->CrackURL(path));
-  base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, fileapi::kCreateFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanCreateFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -438,15 +496,20 @@ void FileAPIMessageFilter::OnCancel(
   }
 }
 
-void FileAPIMessageFilter::OnOpenFile(
-    int request_id, const GURL& path, int file_flags) {
+#if defined(ENABLE_PLUGINS)
+void FileAPIMessageFilter::OnOpenPepperFile(
+    int request_id, const GURL& path, int pp_open_flags) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  base::PlatformFileError error;
-  const int open_permissions = base::PLATFORM_FILE_OPEN |
-                               (file_flags & fileapi::kOpenFilePermissions);
+
   FileSystemURL url(context_->CrackURL(path));
-  if (!HasPermissionsForFile(url, open_permissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(
+        request_id, base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!CanOpenFileSystemURLWithPepperFlags(pp_open_flags, process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(
+        request_id, base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -463,11 +526,20 @@ void FileAPIMessageFilter::OnOpenFile(
     quota_policy = quota::kQuotaLimitTypeLimited;
   }
 
+  int platform_file_flags = 0;
+  if (!ppapi::PepperFileOpenFlagsToPlatformFileFlags(pp_open_flags,
+                                                     &platform_file_flags)) {
+    // |pp_open_flags| should have already been checked in PepperFileIOHost.
+    DLOG(ERROR) << "Open file request with invalid pp_open_flags ignored.";

[kinuko, 2013/09/05 03:49:43]

NOTREACHED() ?

[tommycli, 2013/09/06 01:41:43]

Done. Not sure if the process should crash over an invalid IPC, but it doesn't
seem like it should ever happen in practice.

+    return;
+  }
+
   operations_[request_id] = operation_runner()->OpenFile(
-      url, file_flags, PeerHandle(),
+      url, platform_file_flags, PeerHandle(),
       base::Bind(&FileAPIMessageFilter::DidOpenFile, this, request_id,
                  quota_policy));
 }
+#endif  // defined(ENABLE_PLUGINS)
 
 void FileAPIMessageFilter::OnNotifyCloseFile(int file_open_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
@@ -520,9 +592,14 @@ void FileAPIMessageFilter::OnCreateSnapshotFile(
   // Make sure if this file can be read by the renderer as this is
   // called when the renderer is about to create a new File object
   // (for reading the file).
-  base::PlatformFileError error;
-  if (!HasPermissionsForFile(url, fileapi::kReadFilePermissions, &error)) {
-    Send(new FileSystemMsg_DidFail(request_id, error));
+  if (!FileSystemURLIsValid(context_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_INVALID_URL));
+    return;
+  }
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -547,17 +624,15 @@ void FileAPIMessageFilter::OnAppendBlobDataItemToBlob(
     const GURL& url, const BlobData::Item& item) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   if (item.type() == BlobData::Item::TYPE_FILE_FILESYSTEM) {
-    base::PlatformFileError error;
     FileSystemURL filesystem_url(context_->CrackURL(item.url()));
-    if (!HasPermissionsForFile(filesystem_url,
-                               fileapi::kReadFilePermissions, &error)) {
+    if (!FileSystemURLIsValid(context_, filesystem_url) ||
+        !security_policy_->CanReadFileSystemFile(process_id_, filesystem_url)) {
       OnRemoveBlob(url);
       return;
     }
   }
   if (item.type() == BlobData::Item::TYPE_FILE &&
-      !ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
-          process_id_, item.path())) {
+      !security_policy_->CanReadFile(process_id_, item.path())) {
     OnRemoveBlob(url);
     return;
   }
@@ -819,16 +894,14 @@ void FileAPIMessageFilter::DidCreateSnapshot(
 
   scoped_refptr<webkit_blob::ShareableFileReference> file_ref =
       webkit_blob::ShareableFileReference::Get(platform_path);
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
-          process_id_, platform_path)) {
+  if (!security_policy_->CanReadFile(process_id_, platform_path)) {
     // Give per-file read permission to the snapshot file if it hasn't it yet.
     // In order for the renderer to be able to read the file via File object,
     // it must be granted per-file read permission for the file's platform
     // path. By now, it has already been verified that the renderer has
     // sufficient permissions to read the file, so giving per-file permission
     // here must be safe.
-    ChildProcessSecurityPolicyImpl::GetInstance()->GrantReadFile(
-        process_id_, platform_path);
+    security_policy_->GrantReadFile(process_id_, platform_path);
 
     // Revoke all permissions for the file when the last ref of the file
     // is dropped.
@@ -853,12 +926,6 @@ void FileAPIMessageFilter::DidCreateSnapshot(
                request_id, info, platform_path));
 }
 
-bool FileAPIMessageFilter::HasPermissionsForFile(
-    const FileSystemURL& url, int permissions, base::PlatformFileError* error) {
-  return CheckFileSystemPermissionsForProcess(context_, process_id_, url,
-                                              permissions, error);
-}
-
 scoped_refptr<Stream> FileAPIMessageFilter::GetStreamForURL(const GURL& url) {
   return stream_context_->registry()->GetStream(url);
 }