Added a policy option to restrict the default DACL for tokens.

Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
Committed: https://crrev.com/cfcbe0af3076ba9c23d65bbc92d63e74c7188759
Cr-Commit-Position: refs/heads/master@{#384522}

[forshaw, 2016-03-22 16:04:16]

forshaw@chromium.org changed reviewers:
+ jschuh@chromium.org, wfh@chromium.org

[forshaw, 2016-03-22 16:04:16]

wfh@ can you PTAL.

[forshaw, 2016-03-23 13:49:24]

Description was changed from

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
==========

to

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
==========

[forshaw, 2016-03-23 13:49:25]

forshaw@chromium.org changed reviewers:
- jschuh@chromium.org

[Will Harris, 2016-03-27 01:20:05]

thanks for doing this

I'd quite like to see an sbox_integration_test where you verify that two
sandboxed processes can't open each other's process token... even better if it's
testing the same configuration we see between GPU and renderer. Even better if
it verifies if you don't have this new mode enabled, you can successfully open
the process.

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/acl.h
File sandbox/win/src/acl.h (right):

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/acl.h#newco...
sandbox/win/src/acl.h:33: bool AddSidToDefaultDacl(HANDLE token,
are there really that many users of AddSidToDefaultDacl that they can't just
always use the four parameter function?

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/restricted_...
File sandbox/win/src/restricted_token.cc (right):

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/restricted_...
sandbox/win/src/restricted_token.cc:169: return ::GetLastError();
nit brackets for multiline if statement

[forshaw, 2016-03-28 14:31:06]

Fixed nits and added an integration test. PTAL.

[forshaw, 2016-03-28 16:40:31]

Really really should be done now :-) PTAL.

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/acl.h
File sandbox/win/src/acl.h (right):

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/acl.h#newco...
sandbox/win/src/acl.h:33: bool AddSidToDefaultDacl(HANDLE token,
On 2016/03/27 01:20:05, Will Harris wrote:
> are there really that many users of AddSidToDefaultDacl that they can't just
> always use the four parameter function?

Done.

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/restricted_...
File sandbox/win/src/restricted_token.cc (right):

https://codereview.chromium.org/1821193002/diff/1/sandbox/win/src/restricted_...
sandbox/win/src/restricted_token.cc:169: return ::GetLastError();
On 2016/03/27 01:20:05, Will Harris wrote:
> nit brackets for multiline if statement

Done.

[Will Harris, 2016-03-30 20:02:24]

This looks good. I like your test.

my only comment would be to land the code and the tests but not to enable it yet
in content/common/sandbox_win.cc then it becomes easier to revert if anything
breaks.

[forshaw, 2016-03-31 09:54:49]

On 2016/03/30 20:02:24, Will Harris wrote:
> This looks good. I like your test.
> 
> my only comment would be to land the code and the tests but not to enable it
yet
> in content/common/sandbox_win.cc then it becomes easier to revert if anything
> breaks.

I've broken out the change to sandbox_win into a separate CL and I've also added
a few more integration tests. PTAL.

[Will Harris, 2016-04-01 03:47:33]

lgtm. thanks for this.

[forshaw, 2016-04-01 07:11:45]

The CQ bit was checked by forshaw@chromium.org

[commit-bot: I haz the power, 2016-04-01 07:12:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1821193002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1821193002/100001

[commit-bot: I haz the power, 2016-04-01 08:45:27]

Description was changed from

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
==========

to

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
==========

[commit-bot: I haz the power, 2016-04-01 08:45:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-01 08:46:59]

Description was changed from

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862
==========

to

==========
Added a policy option to restrict the default DACL for tokens.
This patch modified the way the default DACL is calculated for new restricted
tokens to remove certain rights. This blocks processes being able to open
other processes at the same or lower security level.

BUG=596862

Committed: https://crrev.com/cfcbe0af3076ba9c23d65bbc92d63e74c7188759
Cr-Commit-Position: refs/heads/master@{#384522}
==========

[commit-bot: I haz the power, 2016-04-01 08:47:01]

Patchset 6 (id:??) landed as
https://crrev.com/cfcbe0af3076ba9c23d65bbc92d63e74c7188759
Cr-Commit-Position: refs/heads/master@{#384522}