ChildProcessSecurityPolicy: Deprecate bitmask-based permissions checks for files.

ChildProcessSecurityPolicy: Deprecate bitmask-based permissions checks for files.

HasPermissionsForFile and HasPermissionsForFilesystemFile is currently used as general bitmask-based permissions querying functions for files. This change deprecates those functions and adds some additional explicit grants and grant-checking methods instead.

The larger goal is to deprecate all usage of PlatformFile bitmasks in ChildProcessSecurityPolicy in favor of explicitly granted permissions. This is to improve security and allow for a permissions set different than PlatformFile.

See https://chromiumcodereview.appspot.com/18129002.

Original post by vandebo:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/2cGLolxsOs4/Ga8eF7iEejkJ

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

[tommycli, 2013-07-17 21:44:02]

vandebo: If this seems reasonable, I will start on migrating the
PepperFileRefHost classes to use these...

[vandebo (ex-Chrome), 2013-07-17 21:54:27]

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.cc:435: void
ChildProcessSecurityPolicyImpl::GrantWriteFile(
Are these currently used?  I thought Greg's change already handled all the
granting of permissions.

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
File content/browser/child_process_security_policy_impl.h (right):

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.h:55: virtual bool
CanReadFile(int child_id, const base::FilePath& file) OVERRIDE;
File stuff is probably more generally interesting than file system first, so
leave it first.

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.h:140: // Deprecated: Use
CanReadFile, etc. methods instead.
I presume these will go away by the time you're done?

[tommycli, 2013-07-17 22:31:10]

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.cc:435: void
ChildProcessSecurityPolicyImpl::GrantWriteFile(
On 2013/07/17 21:54:27, vandebo wrote:
> Are these currently used?  I thought Greg's change already handled all the
> granting of permissions.

Done: Whoops, guess I got carried away by symmetry.

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
File content/browser/child_process_security_policy_impl.h (right):

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.h:55: virtual bool
CanReadFile(int child_id, const base::FilePath& file) OVERRIDE;
On 2013/07/17 21:54:27, vandebo wrote:
> File stuff is probably more generally interesting than file system first, so
> leave it first.

Done.

https://chromiumcodereview.appspot.com/19599006/diff/9001/content/browser/chi...
content/browser/child_process_security_policy_impl.h:140: // Deprecated: Use
CanReadFile, etc. methods instead.
On 2013/07/17 21:54:27, vandebo wrote:
> I presume these will go away by the time you're done?

Yes these will go away... after a long time.

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
content/browser/child_process_security_policy_impl.cc:65: const int
kCreateWriteFilePermissions =
Here I added EXCLUSIVE_WRITE to the list. Otherwise, GrantCreateWrite would not
make CanWrite true..., which seems counterintuitive and violated my unit test.

Does this seem legit to you?

[vandebo (ex-Chrome), 2013-07-18 15:16:59]

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
File content/browser/child_process_security_policy_impl.cc (right):

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
content/browser/child_process_security_policy_impl.cc:65: const int
kCreateWriteFilePermissions =
On 2013/07/17 22:31:11, tommycli wrote:
> Here I added EXCLUSIVE_WRITE to the list. Otherwise, GrantCreateWrite would
not
> make CanWrite true..., which seems counterintuitive and violated my unit test.
> 
> Does this seem legit to you?

Maybe... Please make sure the the security reviewer considers this question.

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
File content/browser/child_process_security_policy_unittest.cc (right):

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/browser/ch...
content/browser/child_process_security_policy_unittest.cc:97:
PermissionsSet(bool can_read, bool can_write, bool can_create,
A list of bools is almost always a bad plan.  How about a set of methods that
are chainable?

const PermissionSet& EnableRead();
const PermissionSet& EnableWrite();
etc ?

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/public/bro...
File content/public/browser/child_process_security_policy.h (left):

https://chromiumcodereview.appspot.com/19599006/diff/13001/content/public/bro...
content/public/browser/child_process_security_policy.h:43: // Before servicing a
child process's request to upload a file to the web, the
Seems like the explicit guidance in these comments should be preserved.

[tommycli, 2013-07-18 15:56:47]

https://codereview.chromium.org/19599006/diff/13001/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/19599006/diff/13001/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:97:
PermissionsSet(bool can_read, bool can_write, bool can_create,
On 2013/07/18 15:16:59, vandebo wrote:
> A list of bools is almost always a bad plan.  How about a set of methods that
> are chainable?
> 
> const PermissionSet& EnableRead();
> const PermissionSet& EnableWrite();
> etc ?

Done.

https://codereview.chromium.org/19599006/diff/13001/content/public/browser/ch...
File content/public/browser/child_process_security_policy.h (left):

https://codereview.chromium.org/19599006/diff/13001/content/public/browser/ch...
content/public/browser/child_process_security_policy.h:43: // Before servicing a
child process's request to upload a file to the web, the
On 2013/07/18 15:16:59, vandebo wrote:
> Seems like the explicit guidance in these comments should be preserved.

Done.

[vandebo (ex-Chrome), 2013-07-18 16:15:57]

https://codereview.chromium.org/19599006/diff/18001/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/19599006/diff/18001/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:121:
can_create_read_write = true;
Should there only be three bools with this one setting all of them?

[tommycli, 2013-07-18 16:23:45]

https://codereview.chromium.org/19599006/diff/18001/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/19599006/diff/18001/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:121:
can_create_read_write = true;
On 2013/07/18 16:15:58, vandebo wrote:
> Should there only be three bools with this one setting all of them?

Unfortunately, CreateReadWrite is almost a misnomer.

Create|Read|Write != CreateReadWrite. Check out the top of
child_process_security_policy_impl.cc for the specific flags...

Should that above relationship hold? It probably should, or we should rename it
a bit... Thoughts?

[Tom Sepez, 2013-07-19 18:17:24]

Can we get the changes to the callers of these methods added to the CL?  It's a
bit of a drag to create methods that aren't actually used (yet).

[tommycli, 2013-07-19 18:24:36]

On 2013/07/19 18:17:24, Tom Sepez wrote:
> Can we get the changes to the callers of these methods added to the CL?  It's
a
> bit of a drag to create methods that aren't actually used (yet).

Could be done, but would make the CL huge. Here's two work in progress CLs that
part some of the callers over, but I'm not done finding all the callsites yet:
https://codereview.chromium.org/19770009/
https://codereview.chromium.org/19723010

[jam, 2013-07-22 16:21:10]

why not remove the deprecated methods in this change? They're only on the Impl
object, so there shouldn't be that many callers.

[tommycli, 2013-07-22 16:33:54]

On 2013/07/22 16:21:10, jam wrote:
> why not remove the deprecated methods in this change? They're only on the Impl
> object, so there shouldn't be that many callers.

There's three of four callsites that are fairly involved. I'm working through
them on these CLs:

https://codereview.chromium.org/19770009/
https://codereview.chromium.org/19723010/

Once all the callsites are ported over, I will make these methods private.

After that, we will change the internals to remove these methods entirely.

[tommycli, 2013-07-22 19:39:24]

vandebo/tsepez: May I have a review on this please? I'm comfortable with the set
of checks I've added. This CL is the basis of the two CLs I mentioned in the
previous message.

https://codereview.chromium.org/19599006/diff/37001/content/browser/child_pro...
File content/browser/child_process_security_policy_impl.cc (left):

https://codereview.chromium.org/19599006/diff/37001/content/browser/child_pro...
content/browser/child_process_security_policy_impl.cc:74: // need
EXCLUSIVE_WRITE in this mix?
security: I did in fact add EXCLUSIVE_WRITE here, because otherwise kCreateWrite
would not include kWrite, which made it impossible to port legacy bitmask-using
code over.

[Tom Sepez, 2013-07-22 19:44:38]

lgtm

[tommycli, 2013-07-22 20:12:38]

jam: May I have an OWNER review on
content/public/browser/child_process_security_policy.h ?

[vandebo (ex-Chrome), 2013-07-22 21:33:40]

https://codereview.chromium.org/19599006/diff/39001/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/19599006/diff/39001/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:99: : can_read(false),
I find this class strange because the grants are combinations of permissions and
this checks for the exact combination.  I think expanding it away is probably
less confusing in the long run.

[jam, 2013-07-22 23:47:58]

lgtm

[tommycli, 2013-07-23 15:30:22]

https://codereview.chromium.org/19599006/diff/39001/content/browser/child_pro...
File content/browser/child_process_security_policy_unittest.cc (right):

https://codereview.chromium.org/19599006/diff/39001/content/browser/child_pro...
content/browser/child_process_security_policy_unittest.cc:99: : can_read(false),
On 2013/07/22 21:33:41, vandebo wrote:
> I find this class strange because the grants are combinations of permissions
and
> this checks for the exact combination.  I think expanding it away is probably
> less confusing in the long run.

Done. I tried expanding the lines to do it the inline way. It's actually
shorter. I can't argue with that.

[commit-bot: I haz the power, 2013-07-23 15:55:24]

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

[commit-bot: I haz the power, 2013-07-23 18:14:57]

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

[commit-bot: I haz the power, 2013-07-23 18:57:11]

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

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

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=win_rel&nu...

[commit-bot: I haz the power, 2013-07-23 20:02:40]

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

[commit-bot: I haz the power, 2013-07-23 23:03:08]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2013-07-23 23:08:17]

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

[commit-bot: I haz the power, 2013-07-23 23:18:24]

Change committed as 213262