Pepper Message Filters: Port to use explicit permission grants in ChildProcessSecurityPolicy.

Pepper Message Filters: Port to use explicit permission grants in ChildProcessSecurityPolicy.

This ports the Pepper message filters to use explicit permission grants instead of the base::PlatformFile bitmasks which are now deprecated. In the case that we have to deal with Pepper open flags, we now directly translate Pepper open flags to a series of explicit permissions checks instead of converting to base::PlatformFile bitmasks.

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

[jam, 2013-07-22 23:46:41]

https://codereview.chromium.org/19723010/diff/9001/content/public/browser/chi...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/19723010/diff/9001/content/public/browser/chi...
content/public/browser/child_process_security_policy.h:71: bool
HasPermissionsForPepperMode(int child_id, const base::FilePath& file,
nit: since this is used only by content, put it on
ChildProcessSecurityPolicyImpl and not the interface

[tommycli, 2013-07-23 22:21:35]

jochen: Need review of content/browser/renderer_host
dmichael: Need review of content/renderer/pepper
tsepez: General security review.

Thanks!

https://codereview.chromium.org/19723010/diff/9001/content/public/browser/chi...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/19723010/diff/9001/content/public/browser/chi...
content/public/browser/child_process_security_policy.h:71: bool
HasPermissionsForPepperMode(int child_id, const base::FilePath& file,
On 2013/07/22 23:46:41, jam wrote:
> nit: since this is used only by content, put it on
> ChildProcessSecurityPolicyImpl and not the interface

Whoops this was just leftover. This has been put into PepperSecurityHelper
class.

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

https://codereview.chromium.org/19723010/diff/13001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:12: bool
CanOpenWithPepperFlags(int pp_open_flags, int child_id,
security: Needs some scrutiny. See ppapi/shared_impl/file_type_conversion.cc for
PepperFileOpenFlagsToPlatformFileFlags as a reference.

[Tom Sepez, 2013-07-23 22:30:01]

Security stuff LGTM.

[dmichael (off chromium), 2013-07-24 17:02:23]

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

https://codereview.chromium.org/19723010/diff/20001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.cc:33: return false;
I don't think we can do this change until APPEND is supported... we added
support for append recently to our FileIO API:
https://code.google.com/p/chromium/issues/detail?id=242383

Have you run browser_tests?

[dmichael (off chromium), 2013-07-24 17:28:26]

+teravest

[teravest, 2013-07-24 19:50:00]

On 2013/07/24 17:28:26, dmichael wrote:
> +teravest

I have the same concern with regard to append support. Otherwise this looks
fine.

[tommycli, 2013-07-24 21:00:12]

dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy before this
change also always denies requests to APPEND.

Please see file_type_conversion.cc and the flags at the top of
child_process_security_policy_impl.cc.

PPAPITest.FileIO passes. I will run the whole suite of browser tests to confirm.

[teravest, 2013-07-24 21:04:52]

That's strange; there's an explicit test of PP_FILEOPENFLAG_APPEND in
TestFileIO::TestReadWriteSetLength().

On Wed, Jul 24, 2013 at 3:00 PM,  <tommycli@chromium.org> wrote:
> dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy before
> this
> change also always denies requests to APPEND.
>
> Please see file_type_conversion.cc and the flags at the top of
> child_process_security_policy_impl.cc.
>
> PPAPITest.FileIO passes. I will run the whole suite of browser tests to
> confirm.
>
>
> https://codereview.chromium.org/19723010/

[teravest, 2013-07-24 21:56:07]

In the FileIO test, I see that permission check passing when the
append flag is used.

I added the following line in FileAPIMessageFilter::OnOpenFile():
  fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d "
                  "HasPermissions: %s\n",
          file_flags,
          HasPermissionsForFile(url, open_permissions, &error) ?
"true" : "false");

And I get this output:
FileAPIMessageFilter::OnOpenFile, file_flags: 16488 HasPermissions: true
FileAPIMessageFilter::OnOpenFile, file_flags: 164 HasPermissions: true

16488:
  PLATFORM_FILE_CREATE_ALWAYS
  PLATFORM_FILE_READ
  PLATFORM_FILE_WRITE
  PLATFORM_FILE_WRITE_ATTRIBUTES

164:
  PLATFORM_FILE_OPEN_ALWAYS
  PLATFORM_FILE_READ
  PLATFORM_FILE_APPEND

These correspond to file_io and file_io2 in
TestFileIO::TestReadWriteSetLength().

Do we grant all file permissions in the browser when running tests? I
read through the files you mentioned, and can't see how appends are
being permitted either.


On Wed, Jul 24, 2013 at 3:04 PM, Justin TerAvest <teravest@chromium.org> wrote:
> That's strange; there's an explicit test of PP_FILEOPENFLAG_APPEND in
> TestFileIO::TestReadWriteSetLength().
>
> On Wed, Jul 24, 2013 at 3:00 PM,  <tommycli@chromium.org> wrote:
>> dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy before
>> this
>> change also always denies requests to APPEND.
>>
>> Please see file_type_conversion.cc and the flags at the top of
>> child_process_security_policy_impl.cc.
>>
>> PPAPITest.FileIO passes. I will run the whole suite of browser tests to
>> confirm.
>>
>>
>> https://codereview.chromium.org/19723010/

[tommycli, 2013-07-24 22:21:03]

How bizzare! Thanks for confirming the issue. I will investigate and get back to
you.

On 2013/07/24 21:56:07, teravest wrote:
> In the FileIO test, I see that permission check passing when the
> append flag is used.
> 
> I added the following line in FileAPIMessageFilter::OnOpenFile():
>   fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d "
>                   "HasPermissions: %s\n",
>           file_flags,
>           HasPermissionsForFile(url, open_permissions, &error) ?
> "true" : "false");
> 
> And I get this output:
> FileAPIMessageFilter::OnOpenFile, file_flags: 16488 HasPermissions: true
> FileAPIMessageFilter::OnOpenFile, file_flags: 164 HasPermissions: true
> 
> 16488:
>   PLATFORM_FILE_CREATE_ALWAYS
>   PLATFORM_FILE_READ
>   PLATFORM_FILE_WRITE
>   PLATFORM_FILE_WRITE_ATTRIBUTES
> 
> 164:
>   PLATFORM_FILE_OPEN_ALWAYS
>   PLATFORM_FILE_READ
>   PLATFORM_FILE_APPEND
> 
> These correspond to file_io and file_io2 in
> TestFileIO::TestReadWriteSetLength().
> 
> Do we grant all file permissions in the browser when running tests? I
> read through the files you mentioned, and can't see how appends are
> being permitted either.
> 
> 
> On Wed, Jul 24, 2013 at 3:04 PM, Justin TerAvest
<mailto:teravest@chromium.org> wrote:
> > That's strange; there's an explicit test of PP_FILEOPENFLAG_APPEND in
> > TestFileIO::TestReadWriteSetLength().
> >
> > On Wed, Jul 24, 2013 at 3:00 PM,  <mailto:tommycli@chromium.org> wrote:
> >> dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy before
> >> this
> >> change also always denies requests to APPEND.
> >>
> >> Please see file_type_conversion.cc and the flags at the top of
> >> child_process_security_policy_impl.cc.
> >>
> >> PPAPITest.FileIO passes. I will run the whole suite of browser tests to
> >> confirm.
> >>
> >>
> >> https://codereview.chromium.org/19723010/

[tommycli, 2013-07-25 19:49:20]

teravest: I have the answer to the mystery.

I modified your print statement a little bit:
  fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d, "
                  "open_permissions: %d, HasPermissions: %s\n",
                  file_flags,
                  open_permissions,
                  HasPermissionsForFile(url, open_permissions, &error) ?
                      "true" : "false");

And output:
FileAPIMessageFilter::OnOpenFile, file_flags: 164, open_permissions: 5,
HasPermissions: true

Pertinent line of code in FileAPIMessageFilter::OnOpenFile:
  const int open_permissions = base::PLATFORM_FILE_OPEN |
                               (file_flags & fileapi::kOpenFilePermissions);
...
  HasPermissionsForFile(url, open_permissions, &error)

The permissions passed in are bitwise ANDed with a set of permissions that don't
contain APPEND, so an APPEND request can pass. So APPEND is supported
almost-by-accident due to the way FileAPIMessageFilter::OnOpenFile is
constructed.

This CL won't break that. Any APPEND ops that were previously supported will
still be supported. Any calls to ChildProcessSecurityPolicy directly that
contain the APPEND flag didn't pass before, and won't pass after this CL either.

Here's some additional info: After this CL lands, my next plan is to tackle
porting over FileAPIMessageFilter and FileSystemDispatcher. The only user of the
FileAPIMessageFilter::OnOPenFile is Pepper. So I'm likely going to be changing
OnOpenFile to a more specific OnOpenPepperFile and passing in pp_open_flags
directly instead of converting to PlatformFile flags. If you want
ChildProcessSecurityPolicy to support APPEND "for reals", that's the opportunity
to get it fixed.



On 2013/07/24 22:21:03, tommycli wrote:
> How bizzare! Thanks for confirming the issue. I will investigate and get back
to
> you.
> 
> On 2013/07/24 21:56:07, teravest wrote:
> > In the FileIO test, I see that permission check passing when the
> > append flag is used.
> > 
> > I added the following line in FileAPIMessageFilter::OnOpenFile():
> >   fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d "
> >                   "HasPermissions: %s\n",
> >           file_flags,
> >           HasPermissionsForFile(url, open_permissions, &error) ?
> > "true" : "false");
> > 
> > And I get this output:
> > FileAPIMessageFilter::OnOpenFile, file_flags: 16488 HasPermissions: true
> > FileAPIMessageFilter::OnOpenFile, file_flags: 164 HasPermissions: true
> > 
> > 16488:
> >   PLATFORM_FILE_CREATE_ALWAYS
> >   PLATFORM_FILE_READ
> >   PLATFORM_FILE_WRITE
> >   PLATFORM_FILE_WRITE_ATTRIBUTES
> > 
> > 164:
> >   PLATFORM_FILE_OPEN_ALWAYS
> >   PLATFORM_FILE_READ
> >   PLATFORM_FILE_APPEND
> > 
> > These correspond to file_io and file_io2 in
> > TestFileIO::TestReadWriteSetLength().
> > 
> > Do we grant all file permissions in the browser when running tests? I
> > read through the files you mentioned, and can't see how appends are
> > being permitted either.
> > 
> > 
> > On Wed, Jul 24, 2013 at 3:04 PM, Justin TerAvest
> <mailto:teravest@chromium.org> wrote:
> > > That's strange; there's an explicit test of PP_FILEOPENFLAG_APPEND in
> > > TestFileIO::TestReadWriteSetLength().
> > >
> > > On Wed, Jul 24, 2013 at 3:00 PM,  <mailto:tommycli@chromium.org> wrote:
> > >> dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy before
> > >> this
> > >> change also always denies requests to APPEND.
> > >>
> > >> Please see file_type_conversion.cc and the flags at the top of
> > >> child_process_security_policy_impl.cc.
> > >>
> > >> PPAPITest.FileIO passes. I will run the whole suite of browser tests to
> > >> confirm.
> > >>
> > >>
> > >> https://codereview.chromium.org/19723010/

[teravest, 2013-07-25 19:52:28]

Thanks for the investigation!
Your explanation (and plan) sound good to me.

On Thu, Jul 25, 2013 at 1:49 PM,  <tommycli@chromium.org> wrote:
> teravest: I have the answer to the mystery.
>
> I modified your print statement a little bit:
>   fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d, "
>                   "open_permissions: %d, HasPermissions: %s\n",
>                   file_flags,
>                   open_permissions,
>
>                   HasPermissionsForFile(url, open_permissions, &error) ?
>                       "true" : "false");
>
> And output:
> FileAPIMessageFilter::OnOpenFile, file_flags: 164, open_permissions: 5,
> HasPermissions: true
>
> Pertinent line of code in FileAPIMessageFilter::OnOpenFile:
>   const int open_permissions = base::PLATFORM_FILE_OPEN |
>                                (file_flags & fileapi::kOpenFilePermissions);
> ...
>   HasPermissionsForFile(url, open_permissions, &error)
>
> The permissions passed in are bitwise ANDed with a set of permissions that
> don't
> contain APPEND, so an APPEND request can pass. So APPEND is supported
> almost-by-accident due to the way FileAPIMessageFilter::OnOpenFile is
> constructed.
>
> This CL won't break that. Any APPEND ops that were previously supported will
> still be supported. Any calls to ChildProcessSecurityPolicy directly that
> contain the APPEND flag didn't pass before, and won't pass after this CL
> either.
>
> Here's some additional info: After this CL lands, my next plan is to tackle
> porting over FileAPIMessageFilter and FileSystemDispatcher. The only user of
> the
> FileAPIMessageFilter::OnOPenFile is Pepper. So I'm likely going to be
> changing
> OnOpenFile to a more specific OnOpenPepperFile and passing in pp_open_flags
> directly instead of converting to PlatformFile flags. If you want
> ChildProcessSecurityPolicy to support APPEND "for reals", that's the
> opportunity
> to get it fixed.
>
>
>
>
> On 2013/07/24 22:21:03, tommycli wrote:
>>
>> How bizzare! Thanks for confirming the issue. I will investigate and get
>> back
>
> to
>>
>> you.
>
>
>> On 2013/07/24 21:56:07, teravest wrote:
>> > In the FileIO test, I see that permission check passing when the
>> > append flag is used.
>> >
>> > I added the following line in FileAPIMessageFilter::OnOpenFile():
>> >   fprintf(stderr, "FileAPIMessageFilter::OnOpenFile, file_flags: %d "
>> >                   "HasPermissions: %s\n",
>> >           file_flags,
>> >           HasPermissionsForFile(url, open_permissions, &error) ?
>> > "true" : "false");
>> >
>> > And I get this output:
>> > FileAPIMessageFilter::OnOpenFile, file_flags: 16488 HasPermissions: true
>> > FileAPIMessageFilter::OnOpenFile, file_flags: 164 HasPermissions: true
>> >
>> > 16488:
>> >   PLATFORM_FILE_CREATE_ALWAYS
>> >   PLATFORM_FILE_READ
>> >   PLATFORM_FILE_WRITE
>> >   PLATFORM_FILE_WRITE_ATTRIBUTES
>> >
>> > 164:
>> >   PLATFORM_FILE_OPEN_ALWAYS
>> >   PLATFORM_FILE_READ
>> >   PLATFORM_FILE_APPEND
>> >
>> > These correspond to file_io and file_io2 in
>> > TestFileIO::TestReadWriteSetLength().
>> >
>> > Do we grant all file permissions in the browser when running tests? I
>> > read through the files you mentioned, and can't see how appends are
>> > being permitted either.
>> >
>> >
>> > On Wed, Jul 24, 2013 at 3:04 PM, Justin TerAvest
>> <mailto:teravest@chromium.org> wrote:
>> > > That's strange; there's an explicit test of PP_FILEOPENFLAG_APPEND in
>> > > TestFileIO::TestReadWriteSetLength().
>> > >
>> > > On Wed, Jul 24, 2013 at 3:00 PM,  <mailto:tommycli@chromium.org>
>> > > wrote:
>> > >> dmichael/teravest: To my knowledge, the ChildProcessSecurityPolicy
>> > >> before
>> > >> this
>> > >> change also always denies requests to APPEND.
>> > >>
>> > >> Please see file_type_conversion.cc and the flags at the top of
>> > >> child_process_security_policy_impl.cc.
>> > >>
>> > >> PPAPITest.FileIO passes. I will run the whole suite of browser tests
>> > >> to
>> > >> confirm.
>> > >>
>> > >>
>> > >> https://codereview.chromium.org/19723010/
>
>
>
> https://codereview.chromium.org/19723010/

[teravest, 2013-07-25 20:15:37]

lgtm

[dmichael (off chromium), 2013-07-25 20:17:31]

lgtm for pepper OWNERS stuff

[tommycli, 2013-07-25 21:31:27]

jochen: may I have an OWNER review

[jochen (gone - plz use gerrit), 2013-07-26 15:00:59]

lgtm

[commit-bot: I haz the power, 2013-07-26 15:20:58]

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

[tommycli, 2013-07-26 15:40:23]

jam: May I get an OWNER review for content/content_browser.gypi ? Thanks.

[commit-bot: I haz the power, 2013-07-26 16:32:07]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tommycli, 2013-07-29 19:21:57]

On 2013/07/26 15:40:23, tommycli wrote:
> jam: May I get an OWNER review for content/content_browser.gypi ? Thanks.

jam: ping

[jam, 2013-07-30 16:07:12]

lgtm, sorry for the delay i just saw this.

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

https://codereview.chromium.org/19723010/diff/59001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.h:9: #include
"content/browser/child_process_security_policy_impl.h"
nit: don't include the header here.

https://codereview.chromium.org/19723010/diff/59001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/19723010/diff/59001/content/common/view_messa...
content/common/view_messages.h:1901: 
nit: extra line

[tommycli, 2013-07-30 17:52:41]

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

https://codereview.chromium.org/19723010/diff/59001/content/browser/renderer_...
content/browser/renderer_host/pepper/pepper_security_helper.h:9: #include
"content/browser/child_process_security_policy_impl.h"
On 2013/07/30 16:07:12, jam wrote:
> nit: don't include the header here.

Done.

https://codereview.chromium.org/19723010/diff/59001/content/common/view_messa...
File content/common/view_messages.h (right):

https://codereview.chromium.org/19723010/diff/59001/content/common/view_messa...
content/common/view_messages.h:1901: 
On 2013/07/30 16:07:12, jam wrote:
> nit: extra line

Done.

[commit-bot: I haz the power, 2013-07-30 20:38:51]

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

[commit-bot: I haz the power, 2013-07-30 20:39:11]

Failed to apply patch for content/renderer/pepper/pepper_file_io_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/pepper/pepper_file_io_host.cc
  Hunk #1 succeeded at 194 (offset -2 lines).
  Hunk #2 succeeded at 227 (offset -2 lines).
  Hunk #3 FAILED at 239.
  1 out of 3 hunks FAILED -- saving rejects to file
content/renderer/pepper/pepper_file_io_host.cc.rej

Patch:       content/renderer/pepper/pepper_file_io_host.cc
Index: content/renderer/pepper/pepper_file_io_host.cc
diff --git a/content/renderer/pepper/pepper_file_io_host.cc
b/content/renderer/pepper/pepper_file_io_host.cc
index
c58b25fbdfde5a9401661df37d8ec8078b6a1362..0c85824ad6064384f4fba53d9ff233ed071b9673
100644
--- a/content/renderer/pepper/pepper_file_io_host.cc
+++ b/content/renderer/pepper/pepper_file_io_host.cc
@@ -196,10 +196,14 @@ int32_t PepperFileIOHost::OnHostMsgOpen(
   if (rv != PP_OK)
     return rv;
 
-  int flags = 0;
+  // TODO(tommycli): Eventually just pass the Pepper flags straight to the
+  // FileSystemDispatcher so it can handle doing the security check.
+  int platform_file_flags = 0;
   open_flags_ = open_flags;
-  if (!::ppapi::PepperFileOpenFlagsToPlatformFileFlags(open_flags, &flags))
+  if (!::ppapi::PepperFileOpenFlagsToPlatformFileFlags(open_flags,
+                                                       &platform_file_flags)) {
     return PP_ERROR_BADARGUMENT;
+  }
 
   EnterResourceNoLock<PPB_FileRef_API> enter(file_ref_resource, true);
   if (enter.failed())
@@ -225,7 +229,7 @@ int32_t PepperFileIOHost::OnHostMsgOpen(
         weak_factory_.GetWeakPtr(),
         context->MakeReplyMessageContext());
     file_system_dispatcher->OpenFile(
-      file_system_url_, flags,
+      file_system_url_, platform_file_flags,
       base::Bind(&DidOpenFileSystemURL, callback),
       base::Bind(&DidFailOpenFileSystemURL, callback));
   } else {
@@ -235,7 +239,7 @@ int32_t PepperFileIOHost::OnHostMsgOpen(
     if (file_system_type_ != PP_FILESYSTEMTYPE_EXTERNAL || !plugin_delegate)
       return PP_ERROR_FAILED;
     if (!plugin_delegate->AsyncOpenFile(
-            file_ref->GetSystemPath(), flags,
+            file_ref->GetSystemPath(), open_flags,
             base::Bind(&PepperFileIOHost::ExecutePlatformOpenFileCallback,
                        weak_factory_.GetWeakPtr(),
                        context->MakeReplyMessageContext())))

[commit-bot: I haz the power, 2013-07-30 22:03:57]

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

[commit-bot: I haz the power, 2013-07-31 05:46:51]

Change committed as 214581