ChildProcessSecurityPolicy: Port FileAPIMessageFilter to use new checks

ChildProcessSecurityPolicy: Port FileAPIMessageFilter to use new checks

This CL: 
 * Closes the P1 security hole described in http://crbug.com/284792 by changing the message contents to contain Pepper file open flags instead of base::PlatformFileFlags and checking those in FileAPIMessageFilter.
 * Ports the rest of FileAPIMessageFilter to use new CPSP calls.
 * Ports one call in ResourceDispatcherHostImpl.
 * Makes base::PlatformFileFlags-based methods private in CPSP.

Refactoring document / plans here:
https://docs.google.com/a/google.com/document/d/1QGkGWuwgSuaRqovz4wyb0upqPKDVsgYOFKt44E7gmOE/edit?usp=sharing 

BUG=262142, 284792
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223399

[tommycli, 2013-09-04 21:56:54]

kinuko: Need review on content/browser.
tsepez: Need security review.
jam: Need the rest.

[Tom Sepez, 2013-09-04 22:22:44]

LGTM from a security perspective, but some other issues.

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:299: if
(!FileSystemURLIsValid(context_, url)) {
Seems like a shame to repeat this same block of code so often. Can it be
consolidated somewhere?

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:210:
ChildProcessSecurityPolicyImpl* security_policy_;
Why do we need to cache this rather than calling CPSPI::GetInstance()?

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:57: bool pp_read
= !!(pp_open_flags & PP_FILEOPENFLAG_READ);
Seems a shame to have this same logic here and above. It would be nice if one of
these called into the other or some common routines.

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.h (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.h:20: // Helper
method that returns whether or not the child process is allowed to
Uh ... this and the above look like functions rather than methods.

https://codereview.chromium.org/23760004/diff/34001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:224: if
(!ppapi::PepperFileOpenFlagsToPlatformFileFlags(open_flags, NULL))
Is this called in other places? Can it be replaced by something like
ValidatePepperFileOpenFlags(open_flags) ?

[tommycli, 2013-09-04 23:21:26]

tsepez: Agreed with a lot of your issues with SLOC bloat / repetition. I didn't
really have any good solutions to them without re-introducing some kind of enum
interface to CPSP, which seemed... un-typesafe and lame.

If there already exists a typesafe enum library in chromium that could work...

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:299: if
(!FileSystemURLIsValid(context_, url)) {
On 2013/09/04 22:22:45, Tom Sepez wrote:
> Seems like a shame to repeat this same block of code so often. Can it be
> consolidated somewhere?

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?

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:210:
ChildProcessSecurityPolicyImpl* security_policy_;
On 2013/09/04 22:22:45, Tom Sepez wrote:
> Why do we need to cache this rather than calling CPSPI::GetInstance()?

No reason other than to try to contain the SLOC bloat.

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:57: bool pp_read
= !!(pp_open_flags & PP_FILEOPENFLAG_READ);
On 2013/09/04 22:22:45, Tom Sepez wrote:
> Seems a shame to have this same logic here and above. It would be nice if one
of
> these called into the other or some common routines.

I agree it's a shame. The only obvious way I saw to combine these a bit would be
to do something like:

// (Create|Write != CreateWrite) See crbug.com/263150
struct PermissionsSet {
  bool can_read;
  bool can_write;
  bool can_create;
  bool can_create_write;
}

PermissionSet GetPermissionSetForFile(...);
PermissionSet GetPermissionSetForFileSystemFile(...);

But that didn't seem to save any lines of code.

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.h (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.h:20: // Helper
method that returns whether or not the child process is allowed to
On 2013/09/04 22:22:45, Tom Sepez wrote:
> Uh ... this and the above look like functions rather than methods.

Done.

https://codereview.chromium.org/23760004/diff/34001/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:224: if
(!ppapi::PepperFileOpenFlagsToPlatformFileFlags(open_flags, NULL))
On 2013/09/04 22:22:45, Tom Sepez wrote:
> Is this called in other places? Can it be replaced by something like
> ValidatePepperFileOpenFlags(open_flags) ?

Yes, called in FileAPIMessageFilter.

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

Looks good, but (as was discussed) having duplicated logics concerns me.
FileSystemURLIsValid sequence in FAPMF is simple and probably ok as is, but
CanOpenWithPepperFlags one has much more code and looks error-prone.

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:299: if
(!FileSystemURLIsValid(context_, url)) {
On 2013/09/04 23:21:27, tommycli wrote:
> On 2013/09/04 22:22:45, Tom Sepez wrote:
> > Seems like a shame to repeat this same block of code so often. Can it be
> > consolidated somewhere?
> 
> 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?

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;

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:533: DLOG(ERROR) << "Open file
request with invalid pp_open_flags ignored.";
NOTREACHED() ?

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:57: bool pp_read
= !!(pp_open_flags & PP_FILEOPENFLAG_READ);
On 2013/09/04 23:21:27, tommycli wrote:
> On 2013/09/04 22:22:45, Tom Sepez wrote:
> > Seems a shame to have this same logic here and above. It would be nice if
one
> of
> > these called into the other or some common routines.
> 
> I agree it's a shame. The only obvious way I saw to combine these a bit would
be
> to do something like:
> 
> // (Create|Write != CreateWrite) See crbug.com/263150
> struct PermissionsSet {
>   bool can_read;
>   bool can_write;
>   bool can_create;
>   bool can_create_write;
> }
> 
> PermissionSet GetPermissionSetForFile(...);
> PermissionSet GetPermissionSetForFileSystemFile(...);
> 
> But that didn't seem to save any lines of code.

Could we make the common logic a template?  I think something like following
code would work:

namespace {

template <typename CanRead, typename CanWrite,
          typename CanCreate, typename CanCreateWrite,
          typename FileID>
bool CanOpenFileWithPepperFlags(CanRead can_read,
                                CanWrite can_write,
                                CanCreate can_create,
                                CanCreateWrite can_create_write,
                                int pp_open_flags,
                                int child_id,
                                const FileID& file) {
  ChildProcessSecurityPolicyImpl* policy = ...
  bool pp_read = !!(...);
  ...

  if (pp_read && !(policy->*can_read)(child_id, file))
    return false;
  if (pp_write && !(policy->*can_write)(child_id, file))
    return false;
  ...
}

}  // namespace

bool CanOpenWithPepperFlags(int pp_open_flags, int child_id, 
                            const base::FilePath& file) { 
  return CanOpenFileWithPepperFlags(
      &ChildProcessSecurityPolicyImpl::CanReadFile,
      &ChildProcessSecurityPolicyImpl::CanWriteFile,
      &ChildProcessSecurityPolicyImpl::CanCreateFile,
      &ChildProcessSecurityPolicyImpl::CanCreateWriteFile,
      pp_open_flags, child_id, file);
}
  
bool CanOpenFileSystemURLWithPepperFlags(int pp_open_flags, int child_id,
                                         const fileapi::FileSystemURL& url) {
  return CanOpenFileWithPepperFlags(
      &ChildProcessSecurityPolicyImpl::CanReadFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanWriteFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanCreateFileSystemFile,
      &ChildProcessSecurityPolicyImpl::CanCreateWriteFileSystemFile,
      pp_open_flags, child_id, url);
}

Other than SLOC bloat I'm afraid having duplicated logic makes the code
error-prone.  (We likely tend to change only one method and forget the other)

[tommycli, 2013-09-06 01:41:42]

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:299: if
(!FileSystemURLIsValid(context_, url)) {
On 2013/09/05 03:49:43, kinuko wrote:
> On 2013/09/04 23:21:27, tommycli wrote:
> > On 2013/09/04 22:22:45, Tom Sepez wrote:
> > > Seems like a shame to repeat this same block of code so often. Can it be
> > > consolidated somewhere?
> > 
> > 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?
> 
> 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;

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

https://codereview.chromium.org/23760004/diff/34001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:533: DLOG(ERROR) << "Open file
request with invalid pp_open_flags ignored.";
On 2013/09/05 03:49:43, kinuko wrote:
> NOTREACHED() ?

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

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/34001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:57: bool pp_read
= !!(pp_open_flags & PP_FILEOPENFLAG_READ);
On 2013/09/05 03:49:43, kinuko wrote:
> On 2013/09/04 23:21:27, tommycli wrote:
> > On 2013/09/04 22:22:45, Tom Sepez wrote:
> > > Seems a shame to have this same logic here and above. It would be nice if
> one
> > of
> > > these called into the other or some common routines.
> > 
> > I agree it's a shame. The only obvious way I saw to combine these a bit
would
> be
> > to do something like:
> > 
> > // (Create|Write != CreateWrite) See crbug.com/263150
> > struct PermissionsSet {
> >   bool can_read;
> >   bool can_write;
> >   bool can_create;
> >   bool can_create_write;
> > }
> > 
> > PermissionSet GetPermissionSetForFile(...);
> > PermissionSet GetPermissionSetForFileSystemFile(...);
> > 
> > But that didn't seem to save any lines of code.
> 
> Could we make the common logic a template?  I think something like following
> code would work:
> 
> namespace {
> 
> template <typename CanRead, typename CanWrite,
>           typename CanCreate, typename CanCreateWrite,
>           typename FileID>
> bool CanOpenFileWithPepperFlags(CanRead can_read,
>                                 CanWrite can_write,
>                                 CanCreate can_create,
>                                 CanCreateWrite can_create_write,
>                                 int pp_open_flags,
>                                 int child_id,
>                                 const FileID& file) {
>   ChildProcessSecurityPolicyImpl* policy = ...
>   bool pp_read = !!(...);
>   ...
> 
>   if (pp_read && !(policy->*can_read)(child_id, file))
>     return false;
>   if (pp_write && !(policy->*can_write)(child_id, file))
>     return false;
>   ...
> }
> 
> }  // namespace
> 
> bool CanOpenWithPepperFlags(int pp_open_flags, int child_id, 
>                             const base::FilePath& file) { 
>   return CanOpenFileWithPepperFlags(
>       &ChildProcessSecurityPolicyImpl::CanReadFile,
>       &ChildProcessSecurityPolicyImpl::CanWriteFile,
>       &ChildProcessSecurityPolicyImpl::CanCreateFile,
>       &ChildProcessSecurityPolicyImpl::CanCreateWriteFile,
>       pp_open_flags, child_id, file);
> }
>   
> bool CanOpenFileSystemURLWithPepperFlags(int pp_open_flags, int child_id,
>                                          const fileapi::FileSystemURL& url) {
>   return CanOpenFileWithPepperFlags(
>       &ChildProcessSecurityPolicyImpl::CanReadFileSystemFile,
>       &ChildProcessSecurityPolicyImpl::CanWriteFileSystemFile,
>       &ChildProcessSecurityPolicyImpl::CanCreateFileSystemFile,
>       &ChildProcessSecurityPolicyImpl::CanCreateWriteFileSystemFile,
>       pp_open_flags, child_id, url);
> }
> 
> Other than SLOC bloat I'm afraid having duplicated logic makes the code
> error-prone.  (We likely tend to change only one method and forget the other)

Done. I had no idea you could do this with templates. Great idea!

[kinuko, 2013-09-06 02:28:55]

lgtm with some nits

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:502: NOTREACHED();
nit: just

NOTREACHED() << "Open file request with invalid pp_open_flags ignored.";

would work. (Btw NOTREACHED() doesn't crash in official release build)

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:198: bool
ValidateFileSystemURL(int request_id, const fileapi::FileSystemURL& url);
Can you add a method comment saying this sends DidFail message (in addition to
returning false) if the given url is not valid?

https://codereview.chromium.org/23760004/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:11: #define
CALL_MEMBER_FN(ptrToObject, ptrToMember) ((ptrToObject)->*(ptrToMember))
Hmm... do we need this indirection?  It is in general discouraged to use macros
when we can live without it.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...

[tommycli, 2013-09-07 00:28:22]

jam: ready for OWNER review

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.cc (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.cc:502: NOTREACHED();
On 2013/09/06 02:28:55, kinuko wrote:
> nit: just
> 
> NOTREACHED() << "Open file request with invalid pp_open_flags ignored.";
> 
> would work. (Btw NOTREACHED() doesn't crash in official release build)

Done.

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
File content/browser/fileapi/fileapi_message_filter.h (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/fileapi/f...
content/browser/fileapi/fileapi_message_filter.h:198: bool
ValidateFileSystemURL(int request_id, const fileapi::FileSystemURL& url);
On 2013/09/06 02:28:55, kinuko wrote:
> Can you add a method comment saying this sends DidFail message (in addition to
> returning false) if the given url is not valid?

Done.

https://codereview.chromium.org/23760004/diff/53001/content/browser/renderer_...
File content/browser/renderer_host/pepper/pepper_security_helper.cc (right):

https://codereview.chromium.org/23760004/diff/53001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:11: #define
CALL_MEMBER_FN(ptrToObject, ptrToMember) ((ptrToObject)->*(ptrToMember))
On 2013/09/06 02:28:55, kinuko wrote:
> Hmm... do we need this indirection?  It is in general discouraged to use
macros
> when we can live without it.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...

Done.

[jam, 2013-09-09 14:59:16]

On 2013/09/07 00:28:22, tommycli wrote:
> jam: ready for OWNER review

content/child and content/renderer lgtm

[commit-bot: I haz the power, 2013-09-09 20:51:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/23760004/78001

[commit-bot: I haz the power, 2013-09-09 20:51:26]

Failed to apply patch for content/browser/fileapi/fileapi_message_filter.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/fileapi/fileapi_message_filter.cc
  Hunk #1 succeeded at 41 (offset 1 line).
  Hunk #2 succeeded at 75 (offset 2 lines).
  Hunk #3 succeeded at 94 (offset 2 lines).
  Hunk #4 succeeded at 181 (offset 3 lines).
  Hunk #5 succeeded at 262 (offset 14 lines).
  Hunk #6 succeeded at 284 (offset 14 lines).
  Hunk #7 succeeded at 306 (offset 15 lines).
  Hunk #8 succeeded at 323 (offset 15 lines).
  Hunk #9 succeeded at 340 (offset 15 lines).
  Hunk #10 succeeded at 363 (offset 15 lines).
  Hunk #11 succeeded at 386 (offset 15 lines).
  Hunk #12 succeeded at 423 (offset 25 lines).
  Hunk #13 succeeded at 443 (offset 28 lines).
  Hunk #14 succeeded at 464 (offset 28 lines).
  Hunk #15 succeeded at 496 (offset 28 lines).
  Hunk #16 succeeded at 522 (offset 28 lines).
  Hunk #17 succeeded at 587 (offset 28 lines).
  Hunk #18 FAILED at 588.
  Hunk #19 succeeded at 915 (offset 55 lines).
  Hunk #20 succeeded at 947 (offset 55 lines).
  1 out of 20 hunks FAILED -- saving rejects to file
content/browser/fileapi/fileapi_message_filter.cc.rej

Patch:       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..0d1b2bd548c13a0bb8b65cc0a5875d887ef1848c
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,17 @@ 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 (!ValidateFileSystemURL(request_id, src_url) ||
+      !ValidateFileSystemURL(request_id, dest_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 +270,16 @@ 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 (!ValidateFileSystemURL(request_id, src_url) ||
+      !ValidateFileSystemURL(request_id, dest_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 +291,12 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanWriteFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -292,10 +308,12 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -307,10 +325,12 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanCreateFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -328,10 +348,12 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -349,10 +371,12 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanReadFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -374,9 +398,11 @@ 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 (!ValidateFileSystemURL(request_id, url))
+    return;
+  if (!security_policy_->CanWriteFileSystemFile(process_id_, url)) {
+    Send(new FileSystemMsg_DidFail(request_id,
+                                   base::PLATFORM_FILE_ERROR_SECURITY));
     return;
   }
 
@@ -389,10 +415,12 @@ void FileAPIMessageFilter::OnTruncate(
     int request_id,
     const GURL& path,
     int64 length) {
-  base::PlatformFileError error;
   FileSystemURL url(context_->CrackURL(path));
-  if …
(message too large)

[commit-bot: I haz the power, 2013-09-10 02:46:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/23760004/78001

[commit-bot: I haz the power, 2013-09-10 02:46:55]

Failed to apply patch for content/browser/child_process_security_policy_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/child_process_security_policy_impl.cc
  Hunk #1 FAILED at 38.
  Hunk #2 succeeded at 51 (offset 1 line).
  1 out of 2 hunks FAILED -- saving rejects to file
content/browser/child_process_security_policy_impl.cc.rej

Patch:       content/browser/child_process_security_policy_impl.cc
Index: content/browser/child_process_security_policy_impl.cc
diff --git a/content/browser/child_process_security_policy_impl.cc
b/content/browser/child_process_security_policy_impl.cc
index
91a74935c53185aa4e7db44c77cd41e9142f0e13..dbda8685d807275b0925a17d650978e6d4fafe8a
100644
--- a/content/browser/child_process_security_policy_impl.cc
+++ b/content/browser/child_process_security_policy_impl.cc
@@ -38,6 +38,7 @@ const int kReadFilePermissions =
 const int kWriteFilePermissions =
     base::PLATFORM_FILE_OPEN |
     base::PLATFORM_FILE_WRITE |
+    base::PLATFORM_FILE_APPEND |
     base::PLATFORM_FILE_EXCLUSIVE_WRITE |
     base::PLATFORM_FILE_ASYNC |
     base::PLATFORM_FILE_WRITE_ATTRIBUTES;
@@ -50,7 +51,7 @@ const int kEnumerateDirectoryPermissions =
     base::PLATFORM_FILE_ENUMERATE;
 
 // TODO(tommycli): These flag sets need some work to make more obvious.
-// Why for instance, does Create|Write != Create|Write? http://crbug.com/263150
+// Why for instance, does Create|Write != CreateWrite? http://crbug.com/263150
 const int kCreateReadWriteFilePermissions =
     kReadFilePermissions |
     kWriteFilePermissions |

[commit-bot: I haz the power, 2013-09-16 16:11:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/23760004/87001

[commit-bot: I haz the power, 2013-09-16 16:20:03]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-09-16 19:23:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/23760004/87001

[commit-bot: I haz the power, 2013-09-16 20:30:51]

Change committed as 223399