Added a new process mitigation to harden process token IL policy.

Added a new process mitigation to harden process token IL policy.

This adds a new process mitigation policy to harden the current process
token's integrity level policy. What this actually means is the token's
IL policy in its SACL is modified to add no-read-up and no-execute-up
which is not the default. This prevents a lower privilege process from
opening the token object with rights such as duplicate and impersonation
which could be used to circumvent sandbox restrictions and elevate
privileges. While the policy is only enabled on the browser process by
making it a general mitigation policy it could be applied to all process
levels such as the GPU process to provide a similar effect.

BUG=440692
Committed: https://crrev.com/19669894e93f9279e860d9fff6a54f6cd042acd7
Cr-Commit-Position: refs/heads/master@{#313099}

[forshaw, 2014-12-17 17:59:03]

forshaw@chromium.org changed reviewers:
+ agl@chromium.org, cpu@chromium.org, jschuh@chromium.org

[forshaw, 2014-12-17 17:59:03]

Please can you review this CL.

[cpu_(ooo_6.6-7.5), 2014-12-17 22:35:38]

first question. This has the chance to break something subtle, justin submarine
style. Do we want a flag to disable this? 

Or do we start with a flag to enable and (we) run a few days with it enabled?

[forshaw, 2014-12-17 22:39:05]

Well it shouldn't affect anything in theory. You shouldn't be able to access the
token from a process unless you're already at the same IL level. This is really
a defence in depth against things like the NtUserGetClipboardAccessToken kind of
"features". But that's not to say there isn't potential issues. I'm sure I can
add it as an option in case the actual process of setting the policy causes
issues on some platforms.

[cpu_(ooo_6.6-7.5), 2014-12-18 21:05:36]

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_miti...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_miti...
sandbox/win/src/process_mitigations.cc:63: if (version >=
base::win::VERSION_VISTA &&
if you don't mind start at win7. we have very poor coverage for vista and the
fallout is usually directly to me.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
File sandbox/win/src/restricted_token_utils.cc (right):

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:346: if (base::win::GetVersion() <
base::win::VERSION_VISTA)
seven

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:355: last_error = ::GetLastError();
Mixing :: style for calling windows apis. Looks at the this file and settle to
the most common style.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:381: {
381 bracket in the previous line?

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:404: base::win::ScopedHandle
token(token_handle);
isn't there a base/ helper for doing 399 to 404 ?

[forshaw, 2014-12-19 08:30:00]

If you can just have another quick look it would be appreciated.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_miti...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/process_miti...
sandbox/win/src/process_mitigations.cc:63: if (version >=
base::win::VERSION_VISTA &&
On 2014/12/18 21:05:36, cpu wrote:
> if you don't mind start at win7. we have very poor coverage for vista and the
> fallout is usually directly to me.

Done.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
File sandbox/win/src/restricted_token_utils.cc (right):

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:346: if (base::win::GetVersion() <
base::win::VERSION_VISTA)
On 2014/12/18 21:05:36, cpu wrote:
> seven

Done.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:355: last_error = ::GetLastError();
On 2014/12/18 21:05:36, cpu wrote:
> Mixing :: style for calling windows apis. Looks at the this file and settle to
> the most common style.

Done.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:381: {
On 2014/12/18 21:05:36, cpu wrote:
> 381 bracket in the previous line?

Done.

https://codereview.chromium.org/810083002/diff/1/sandbox/win/src/restricted_t...
sandbox/win/src/restricted_token_utils.cc:404: base::win::ScopedHandle
token(token_handle);
On 2014/12/18 21:05:36, cpu wrote:
> isn't there a base/ helper for doing 399 to 404 ?

Not that I could see in code search. The only users of OpenProcessToken then
used to to get the user SID or the token IL and never returned the token
directly.

[jschuh, 2014-12-20 01:01:49]

Sorry, didn't realize anyone was waiting on me. lgtm

[Will Harris, 2014-12-20 01:07:23]

On 2014/12/20 01:01:49, jschuh wrote:
> Sorry, didn't realize anyone was waiting on me. lgtm

might be worth someone testing this on Citrix ICA, that seems to cause us the
most bizarre "justin submarine style" issues related to sandboxes.

[cpu_(ooo_6.6-7.5), 2014-12-20 01:10:05]

lgtm with two nits.


"best submarine is justin's submarine"

https://codereview.chromium.org/810083002/diff/20001/sandbox/win/src/restrict...
File sandbox/win/src/restricted_token_utils.h (right):

https://codereview.chromium.org/810083002/diff/20001/sandbox/win/src/restrict...
sandbox/win/src/restricted_token_utils.h:85: // Hardens the integrity level
policy on a token. This is only valid on Vista
win7 in the comment.

https://codereview.chromium.org/810083002/diff/20001/sandbox/win/src/restrict...
sandbox/win/src/restricted_token_utils.h:92: // valid on Vista and above.
Specifically it sets the policy to block read
win7

[jschuh, 2015-01-22 04:33:10]

I just noticed this is still sitting. Any reason it didn't land?

[Will Harris, 2015-01-22 04:53:31]

my gift is some free try jobs.

[forshaw, 2015-01-22 06:46:35]

I think it was just there's one file which needs owner approval from agl and I
didn't chase that up. I was going to split out the CL so that the sandbox and
small change in the browser could be landed independently but I also didn't get
around to that.

[jschuh, 2015-01-22 19:55:07]

It's just a flag change in that file. And I'm happy to nag agl@ to rubberstamp
it. (agl@ this is my first volley of nagging.)

[Will Harris, 2015-01-26 18:12:39]

On 2015/01/22 19:55:07, jschuh wrote:
> It's just a flag change in that file. And I'm happy to nag agl@ to rubberstamp
> it. (agl@ this is my first volley of nagging.)

Second nag!

[agl, 2015-01-26 18:24:06]

LGTM

Sorry, if I haven't responded within 1 business day then I've missed the email
and probably need to be poked directly (i.e. not on the codereview thread.)

[forshaw, 2015-01-26 18:27:30]

The CQ bit was checked by forshaw@chromium.org

[forshaw, 2015-01-26 18:27:41]

The CQ bit was unchecked by forshaw@chromium.org

[forshaw, 2015-01-26 18:27:47]

The CQ bit was checked by forshaw@chromium.org

[commit-bot: I haz the power, 2015-01-26 18:28:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/810083002/40001

[commit-bot: I haz the power, 2015-01-26 19:28:01]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-01-26 19:29:13]

Patchset 3 (id:??) landed as
https://crrev.com/19669894e93f9279e860d9fff6a54f6cd042acd7
Cr-Commit-Position: refs/heads/master@{#313099}