Update the child process security policy to use explicit permission grants.

Update the child process security policy to use explicit permission grants.

This replaces the method taking generic permissions with specific
interface methods. This makes using particular platform file flags an
implementation detail, as well as restricting the kinds of permissions
bundles client code can request to particular use bundles under the
control of the security policy.

R=vandebo@chromium.org
BUG=None

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

[vandebo (ex-Chrome), 2013-06-27 22:35:17]

LGTM

https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:851:
content::ChildProcessSecurityPolicy::GetInstance()->GrantReadFile(
read is a subset of readwrite, so we really only need to do one or the other
here.  How about:

if (action->CanWrite()) {
  grant read write
} else if (action->CanRead()) {
  grant read
}

?

https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:1054: GrantReadFile(id, *file);
nit: I think this will wrap at "id,"

https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
content/public/browser/render_view_host.h:212: enum FileSelectionPermissions {
style guide says this should go above any methods.  Considering the number of
methods here, I'm not going to push too hard if you really think it should stay
here.

[Greg Billock, 2013-06-28 18:40:33]

+tsepez for security review

https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
(right):

https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:851:
content::ChildProcessSecurityPolicy::GetInstance()->GrantReadFile(
I saw this, but decided to preserve the calls since we'll likely move to
specific security-policy permissions that might not be direct subsets.

On 2013/06/27 22:35:17, vandebo wrote:
> read is a subset of readwrite, so we really only need to do one or the other
> here.  How about:
> 
> if (action->CanWrite()) {
>   grant read write
> } else if (action->CanRead()) {
>   grant read
> }
> 
> ?

https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
File chrome/browser/sessions/session_restore.cc (right):

https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
chrome/browser/sessions/session_restore.cc:1054: GrantReadFile(id, *file);
On 2013/06/27 22:35:17, vandebo wrote:
> nit: I think this will wrap at "id,"

Done.

https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
content/public/browser/render_view_host.h:212: enum FileSelectionPermissions {
Yeah, that was what I was thinking. I can move it if we want to.

On 2013/06/27 22:35:17, vandebo wrote:
> style guide says this should go above any methods.  Considering the number of
> methods here, I'm not going to push too hard if you really think it should
stay
> here.

[Greg Billock, 2013-07-09 18:24:26]

On 2013/06/28 18:40:33, Greg Billock wrote:
> +tsepez for security review
> 
>
https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
> File chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc
> (right):
> 
>
https://codereview.chromium.org/18129002/diff/1/chrome/browser/chromeos/exten...
> chrome/browser/chromeos/extensions/file_manager/file_handler_util.cc:851:
> content::ChildProcessSecurityPolicy::GetInstance()->GrantReadFile(
> I saw this, but decided to preserve the calls since we'll likely move to
> specific security-policy permissions that might not be direct subsets.
> 
> On 2013/06/27 22:35:17, vandebo wrote:
> > read is a subset of readwrite, so we really only need to do one or the other
> > here.  How about:
> > 
> > if (action->CanWrite()) {
> >   grant read write
> > } else if (action->CanRead()) {
> >   grant read
> > }
> > 
> > ?
> 
>
https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
> File chrome/browser/sessions/session_restore.cc (right):
> 
>
https://codereview.chromium.org/18129002/diff/1/chrome/browser/sessions/sessi...
> chrome/browser/sessions/session_restore.cc:1054: GrantReadFile(id, *file);
> On 2013/06/27 22:35:17, vandebo wrote:
> > nit: I think this will wrap at "id,"
> 
> Done.
> 
>
https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
> File content/public/browser/render_view_host.h (right):
> 
>
https://codereview.chromium.org/18129002/diff/1/content/public/browser/render...
> content/public/browser/render_view_host.h:212: enum FileSelectionPermissions {
> Yeah, that was what I was thinking. I can move it if we want to.
> 
> On 2013/06/27 22:35:17, vandebo wrote:
> > style guide says this should go above any methods.  Considering the number
of
> > methods here, I'm not going to push too hard if you really think it should
> stay
> > here.

Ping tsepez. Are you around this week?

[Tom Sepez, 2013-07-09 18:44:29]

> Ping tsepez. Are you around this week?

Looking ...

[Tom Sepez, 2013-07-09 18:52:50]

LGTM with nits.

https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:845: if (permissions ==
RenderViewHost::FILE_PERMISSION_WRITE) {
nit: else if

https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
content/public/browser/child_process_security_policy.h:53: // This permission
grants creation, read, and full write access to a file,
nit: maybe this should be called GrantCreateReadWriteFile.

[Greg Billock, 2013-07-09 21:09:50]

https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_view_host_impl.cc:845: if (permissions ==
RenderViewHost::FILE_PERMISSION_WRITE) {
On 2013/07/09 18:52:50, Tom Sepez wrote:
> nit: else if

Done.

https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
File content/public/browser/child_process_security_policy.h (right):

https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
content/public/browser/child_process_security_policy.h:53: // This permission
grants creation, read, and full write access to a file,
On 2013/07/09 18:52:50, Tom Sepez wrote:
> nit: maybe this should be called GrantCreateReadWriteFile.

Done.

[Greg Billock, 2013-07-09 21:15:56]

On 2013/07/09 21:09:50, Greg Billock wrote:
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_view_host_impl.cc (right):
> 
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_view_host_impl.cc:845: if (permissions ==
> RenderViewHost::FILE_PERMISSION_WRITE) {
> On 2013/07/09 18:52:50, Tom Sepez wrote:
> > nit: else if
> 
> Done.
> 
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> File content/public/browser/child_process_security_policy.h (right):
> 
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> content/public/browser/child_process_security_policy.h:53: // This permission
> grants creation, read, and full write access to a file,
> On 2013/07/09 18:52:50, Tom Sepez wrote:
> > nit: maybe this should be called GrantCreateReadWriteFile.
> 
> Done.

Adding owners:

jam for content API and renderer_host.
satorux for chromeos file manager extension
bbudge for file_select_helper
sky for sessions

The intention is that the requested permissions remain identical to the current
set. Specific platform file flags are made an implementation detail of the
security policy.

[bbudge, 2013-07-09 21:50:50]

On 2013/07/09 21:15:56, Greg Billock wrote:
> On 2013/07/09 21:09:50, Greg Billock wrote:
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> > content/browser/renderer_host/render_view_host_impl.cc:845: if (permissions
==
> > RenderViewHost::FILE_PERMISSION_WRITE) {
> > On 2013/07/09 18:52:50, Tom Sepez wrote:
> > > nit: else if
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> > File content/public/browser/child_process_security_policy.h (right):
> > 
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> > content/public/browser/child_process_security_policy.h:53: // This
permission
> > grants creation, read, and full write access to a file,
> > On 2013/07/09 18:52:50, Tom Sepez wrote:
> > > nit: maybe this should be called GrantCreateReadWriteFile.
> > 
> > Done.
> 
> Adding owners:
> 
> jam for content API and renderer_host.
> satorux for chromeos file manager extension
> bbudge for file_select_helper
> sky for sessions
> 
> The intention is that the requested permissions remain identical to the
current
> set. Specific platform file flags are made an implementation detail of the
> security policy.

Hi Greg, I am not an OWNER for chrome/browser/file_select_helper.cc.

[sky, 2013-07-09 21:53:20]

LGTM

[Greg Billock, 2013-07-09 22:51:00]

On 2013/07/09 21:50:50, bbudge1 wrote:
> On 2013/07/09 21:15:56, Greg Billock wrote:
> > On 2013/07/09 21:09:50, Greg Billock wrote:
> > >
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> > > File content/browser/renderer_host/render_view_host_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/browser/renderer_...
> > > content/browser/renderer_host/render_view_host_impl.cc:845: if
(permissions
> ==
> > > RenderViewHost::FILE_PERMISSION_WRITE) {
> > > On 2013/07/09 18:52:50, Tom Sepez wrote:
> > > > nit: else if
> > > 
> > > Done.
> > > 
> > >
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> > > File content/public/browser/child_process_security_policy.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/18129002/diff/11001/content/public/browser/ch...
> > > content/public/browser/child_process_security_policy.h:53: // This
> permission
> > > grants creation, read, and full write access to a file,
> > > On 2013/07/09 18:52:50, Tom Sepez wrote:
> > > > nit: maybe this should be called GrantCreateReadWriteFile.
> > > 
> > > Done.
> > 
> > Adding owners:
> > 
> > jam for content API and renderer_host.
> > satorux for chromeos file manager extension
> > bbudge for file_select_helper
> > sky for sessions
> > 
> > The intention is that the requested permissions remain identical to the
> current
> > set. Specific platform file flags are made an implementation detail of the
> > security policy.
> 
> Hi Greg, I am not an OWNER for chrome/browser/file_select_helper.cc.

Sorry. It looked in blame like you'd know the most about the permissions there.
I think other reviewers are more OWNERS-ish. If I read wrong I'll look about
more.

[bbudge, 2013-07-09 23:07:05]

I see that I did modify this file, probably while doing some File Chooser work.
It LGTM.

[jam, 2013-07-10 01:14:16]

I don't see the reason for this. I'm apprehensive about having yet another place
that holds flags about what permission to open the files. it seems that we could
use the definition from base and have documentation/dchecks/early returns to
verify that only PLATFORM_FILE_READ or PLATFORM_FILE_WRITE are passed in.

[satorux1, 2013-07-10 06:34:09]

chrome/browser/chromeos/extensions/file_manager LGTM. The new code looks simpler
in this directory.

[Greg Billock, 2013-07-10 16:07:21]

On 2013/07/10 01:14:16, jam wrote:
> I don't see the reason for this. I'm apprehensive about having yet another
place
> that holds flags about what permission to open the files. it seems that we
could
> use the definition from base and have documentation/dchecks/early returns to
> verify that only PLATFORM_FILE_READ or PLATFORM_FILE_WRITE are passed in.

I don't understand. This reduces the number of place that file permission flags
are managed to one -- the internals of the security policy itself. And since the
permission flags themselves are now an implementation-only detail, we could even
get rid of them and replace them with security-specific constants that govern
capability accesses directly, if we want to.

Is that what you're thinking about in a restriction to PLATFORM_FILE_READ and
PLATFORM_FILE_WRITE? I don't think we'd want to re-use those constants; I think
specific grant-type enumeration in the API gives us more flexibility to tighten
security controls. It makes it clear who wants which permissions, rather than
having to reason about the flags and their access permissions separately.

chromium-dev discussion:
https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/2cGLolxsOs4

[jam, 2013-07-10 17:16:28]

On 2013/07/10 16:07:21, Greg Billock wrote:
> On 2013/07/10 01:14:16, jam wrote:
> > I don't see the reason for this. I'm apprehensive about having yet another
> place
> > that holds flags about what permission to open the files. it seems that we
> could
> > use the definition from base and have documentation/dchecks/early returns to
> > verify that only PLATFORM_FILE_READ or PLATFORM_FILE_WRITE are passed in.
> 
> I don't understand. This reduces the number of place that file permission
flags
> are managed to one -- the internals of the security policy itself. And since
the
> permission flags themselves are now an implementation-only detail, we could
even
> get rid of them and replace them with security-specific constants that govern
> capability accesses directly, if we want to.
> 
> Is that what you're thinking about in a restriction to PLATFORM_FILE_READ and
> PLATFORM_FILE_WRITE? I don't think we'd want to re-use those constants; I
think
> specific grant-type enumeration in the API gives us more flexibility to
tighten
> security controls. It makes it clear who wants which permissions, rather than
> having to reason about the flags and their access permissions separately.
> 
> chromium-dev discussion:
>
https://groups.google.com/a/chromium.org/forum/#%21topic/chromium-dev/2cGLolx...

i see, thanks for the background.

i'm ok with the changes to CPSP

however the changes to RVH mean that we're creating yet another enum of file
permissions. following the code paths that call this, it looks like
chrome/browser/file_select_helper.cc picks the existing file permission flags
based on the content::FileChooserParams::mode value that was passed in to
WebContentsDelegate::RunFileChooser. So instead you can make
RenderViewHost::FilesSelectedInChooser take a content::FileChooserParams::mode

[Greg Billock, 2013-07-10 18:38:57]

On 2013/07/10 17:16:28, jam wrote:
> On 2013/07/10 16:07:21, Greg Billock wrote:
> > On 2013/07/10 01:14:16, jam wrote:
> > > I don't see the reason for this. I'm apprehensive about having yet another
> > place
> > > that holds flags about what permission to open the files. it seems that we
> > could
> > > use the definition from base and have documentation/dchecks/early returns
to
> > > verify that only PLATFORM_FILE_READ or PLATFORM_FILE_WRITE are passed in.
> > 
> > I don't understand. This reduces the number of place that file permission
> flags
> > are managed to one -- the internals of the security policy itself. And since
> the
> > permission flags themselves are now an implementation-only detail, we could
> even
> > get rid of them and replace them with security-specific constants that
govern
> > capability accesses directly, if we want to.
> > 
> > Is that what you're thinking about in a restriction to PLATFORM_FILE_READ
and
> > PLATFORM_FILE_WRITE? I don't think we'd want to re-use those constants; I
> think
> > specific grant-type enumeration in the API gives us more flexibility to
> tighten
> > security controls. It makes it clear who wants which permissions, rather
than
> > having to reason about the flags and their access permissions separately.
> > 
> > chromium-dev discussion:
> >
>
https://groups.google.com/a/chromium.org/forum/#%2521topic/chromium-dev/2cGLo...
> 
> i see, thanks for the background.
> 
> i'm ok with the changes to CPSP
> 
> however the changes to RVH mean that we're creating yet another enum of file
> permissions. following the code paths that call this, it looks like
> chrome/browser/file_select_helper.cc picks the existing file permission flags
> based on the content::FileChooserParams::mode value that was passed in to
> WebContentsDelegate::RunFileChooser. So instead you can make
> RenderViewHost::FilesSelectedInChooser take a content::FileChooserParams::mode

OK, yes. I think re-using FileChooserParams::mode is better. I'll plumb that
through. (I remember thinking about that option, perhaps I'll soon rediscover
why it seemed daunting. :-))

[Greg Billock, 2013-07-10 19:18:06]

On 2013/07/10 18:38:57, Greg Billock wrote:
> On 2013/07/10 17:16:28, jam wrote:
> > On 2013/07/10 16:07:21, Greg Billock wrote:
> > > On 2013/07/10 01:14:16, jam wrote:
> > > > I don't see the reason for this. I'm apprehensive about having yet
another
> > > place
> > > > that holds flags about what permission to open the files. it seems that
we
> > > could
> > > > use the definition from base and have documentation/dchecks/early
returns
> to
> > > > verify that only PLATFORM_FILE_READ or PLATFORM_FILE_WRITE are passed
in.
> > > 
> > > I don't understand. This reduces the number of place that file permission
> > flags
> > > are managed to one -- the internals of the security policy itself. And
since
> > the
> > > permission flags themselves are now an implementation-only detail, we
could
> > even
> > > get rid of them and replace them with security-specific constants that
> govern
> > > capability accesses directly, if we want to.
> > > 
> > > Is that what you're thinking about in a restriction to PLATFORM_FILE_READ
> and
> > > PLATFORM_FILE_WRITE? I don't think we'd want to re-use those constants; I
> > think
> > > specific grant-type enumeration in the API gives us more flexibility to
> > tighten
> > > security controls. It makes it clear who wants which permissions, rather
> than
> > > having to reason about the flags and their access permissions separately.
> > > 
> > > chromium-dev discussion:
> > >
> >
>
https://groups.google.com/a/chromium.org/forum/#%252521topic/chromium-dev/2cG...
> > 
> > i see, thanks for the background.
> > 
> > i'm ok with the changes to CPSP
> > 
> > however the changes to RVH mean that we're creating yet another enum of file
> > permissions. following the code paths that call this, it looks like
> > chrome/browser/file_select_helper.cc picks the existing file permission
flags
> > based on the content::FileChooserParams::mode value that was passed in to
> > WebContentsDelegate::RunFileChooser. So instead you can make
> > RenderViewHost::FilesSelectedInChooser take a
content::FileChooserParams::mode
> 
> OK, yes. I think re-using FileChooserParams::mode is better. I'll plumb that
> through. (I remember thinking about that option, perhaps I'll soon rediscover
> why it seemed daunting. :-))

Not too bad. Have another look jam and bbudge?

[jam, 2013-07-10 19:52:41]

lgtm

[bbudge, 2013-07-10 20:02:42]

Still LGTM

[commit-bot: I haz the power, 2013-07-10 20:25:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/18129002/50001

[commit-bot: I haz the power, 2013-07-11 04:37:27]

Change committed as 211018