Sandbox policy and intercepts for the MITIGATION_WIN32K_DISABLE policy for renderer processes.

Sandbox policy and intercepts for the MITIGATION_WIN32K_DISABLE policy for renderer processes.

This policy when set will prevent the renderer process from making Win32K.sys calls via user32/gdi32 on
Windows 8 and beyond.

The following intercepts are needed for getting basic renderer functionality.
1. gdi32!GdiDllInitialize:
2. gdi32!GetStockObject.
3. user32!RegisterClassW.

The above functions are called during renderer process initialization. We intercept these APIS by
EAT patching the corresponding dlls and return fake success values from those.

The intercepts live in the process_mitigations_win32k_interception.cc/.h files. The rest of the changes
are plumbing with the sandbox policy framework.

While basic renderers work well now on Windows 8, pepper flash does not as it sends an IPC to the renderer
to creating the transport DIB. Justin is aware of this problem and thinks we can workaround this.

BUG=365160
Added gdi and user32 interceptors for the win32k lockdown project.

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

[jschuh, 2014-06-06 03:23:37]

Big picture comment: you don't need to add a new policy for this. All you're
doing is fixups to make the MITIGATION_WIN32K_DISABLE flag in the
SetProcessMitigations() work due to some unavoidable library loads, so the
policy is already being conveyed by the arguments supplied to
SetProcessMitigations().

In fact, the policy you added doesn't even seem to do anything. So, just remove
it entirely. (I listed out the specific changes inline).

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
content/common/sandbox_win.cc:338: result =
policy->AddRule(sandbox::TargetPolicy::SUBSYS_WIN32K_LOCKDOWN,
Just remove this section. You're already setting the flag you need later on. So,
you shouldn't need to change this file at all.

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
content/common/sandbox_win.cc:620: mitigations |=
sandbox::MITIGATION_WIN32K_DISABLE;
You're already doing exactly what you need to enable this mitigation here.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/sandbox_win....
File sandbox/win/sandbox_win.gypi (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/sandbox_win....
sandbox/win/sandbox_win.gypi:81: 'src/process_mitigations_win32k_policy.cc',
Get rid of the two process_mitigations_win32k_policy.* files.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
Delete this file.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
Delete this file.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:30: SUBSYS_WIN32K_LOCKDOWN    // Win32K
Lockdown related policy.
Remove this SubSystem. You actually shouldn't need to modify this file at all.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: LOCKDOWN_WIN32K         // Locks down the
Win32K subsystem for the process.
Remove this Semantic value.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:25: #include
"sandbox/win/src/process_mitigations_win32k_policy.h"
Remove this include because you're removing the file.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:419: }
Remove this whole case.

[rvargas (doing something else), 2014-06-06 21:22:24]

I have vague recollections about discussions to have a more formal policy for
stuff conveyed by other means (like mitigation flags).

At a high level, we have two options:

1. Say that once a mitigation flag is set, we'll trigger the interceptions for
stuff _we_ currently need.

2. Assume that it is possible to set the mitigation flag without having to
implement interceptions (say, if the target code doesn't load user/gdi at all).
This means that setting interceptions can be done independently from setting a
mitigation.

I really can go either way. #1 is what chrome needs today, #2 is more general
purpose (which has always been a principle of the sandbox)

In any case, as I said in some comments, given that this is a policy-driven
interception I would like to keep it as consistent as possible with the rest of
the system.

Note that this is somewhat unique in the sense that all the non-ipc
interceptions up to this point are non policy driven and that's why they don't
follow the usual dispatcher pattern (they have a "manual" setup).

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.cc:251: //
-----------------------------------------------------------------------
nit: add one of this at line 271

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.h:169: // Interceptors for
GdiDllInitialize/GetStockObject/RegisterClassW.
The three methods are not related. Please add a comment for each one.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_dispatcher.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_dispatcher.cc:22: return true;
Shouldn't this be false?

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_dispatcher.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_dispatcher.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
nit: No (c) for new files

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_interception.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_interception.cc:16: HGDIOBJ WINAPI
TargetGetStockObject(GetStockObjectFunction orig_get_stock_object,
Args on the next line :(

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_interception.cc:18: return
reinterpret_cast<HGDIOBJ>(1);
Is this the final code? Can we return NULL?

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:5: #ifndef
SANDBOX_SRC_PROCESS_MITIGATIONS_WIN32K_POLICY_H__
only one underscore at the end.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:130: dispatcher = new
ProcessMitigationsWin32KDispatcher(this);
I guess this is the core of the matter.

I'd like to keep the consistency... so I want to have a formal dispatcher, even
if it doesn't do anything on the broker side.

One way or another, this is a policy driven interception (even if there is no
IPC), so we may do without a low level policy, but all the current
infrastructure assumes a dispatcher means low level policy, so the option would
be change that.

On the other hand, we could do policy look up on the target, say to decide if we
should fake success or failure, but that is most likely overkill for what we
need at this point (assuming we don't find out very soon that we need more
interceptions).

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:310: mitigations_ = flags;
If we don't want to have a separate SUBSYS_WIN32K_LOCKDOWN, we could also call
ProcessMitigationsWin32KLockdownPolicy::GenerateRules from here after checking
the right flag.

[ananta, 2014-06-06 23:57:36]

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
content/common/sandbox_win.cc:338: result =
policy->AddRule(sandbox::TargetPolicy::SUBSYS_WIN32K_LOCKDOWN,
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Just remove this section. You're already setting the flag you need later on.
So,
> you shouldn't need to change this file at all.

Leaving this as is as this is needed for the policy stuff to work correctly.

https://codereview.chromium.org/318603003/diff/20001/content/common/sandbox_w...
content/common/sandbox_win.cc:620: mitigations |=
sandbox::MITIGATION_WIN32K_DISABLE;
On 2014/06/06 03:23:36, Justin Schuh wrote:
> You're already doing exactly what you need to enable this mitigation here.

Explained above. Leaving the policy stuff as is.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/sandbox_win....
File sandbox/win/sandbox_win.gypi (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/sandbox_win....
sandbox/win/sandbox_win.gypi:81: 'src/process_mitigations_win32k_policy.cc',
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Get rid of the two process_mitigations_win32k_policy.* files.

Leaving them as is.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.cc:251: //
-----------------------------------------------------------------------
On 2014/06/06 21:22:23, rvargas wrote:
> nit: add one of this at line 271

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
File sandbox/win/src/interceptors_64.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/intercep...
sandbox/win/src/interceptors_64.h:169: // Interceptors for
GdiDllInitialize/GetStockObject/RegisterClassW.
On 2014/06/06 21:22:23, rvargas wrote:
> The three methods are not related. Please add a comment for each one.

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_dispatcher.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_dispatcher.cc:22: return true;
On 2014/06/06 21:22:24, rvargas wrote:
> Shouldn't this be false?

No. If we return false the PolicyBase::SetupAllInterceptions function aborts and
causes the policies to not be applied fully
causing the process launch to fail.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_dispatcher.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_dispatcher.h:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/06/06 21:22:24, rvargas wrote:
> nit: No (c) for new files

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_interception.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_interception.cc:16: HGDIOBJ WINAPI
TargetGetStockObject(GetStockObjectFunction orig_get_stock_object,
On 2014/06/06 21:22:24, rvargas wrote:
> Args on the next line :(

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_interception.cc:18: return
reinterpret_cast<HGDIOBJ>(1);
On 2014/06/06 21:22:24, rvargas wrote:
> Is this the final code? Can we return NULL?

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:1: // Copyright (c) 2014
The Chromium Authors. All rights reserved.
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Delete this file.

Leaving this as is.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:1: // Copyright (c) 2014 The
Chromium Authors. All rights reserved.
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Delete this file.

Thought about this some more. Not sure if adding the virtual function as we
discussed is any cleaner. Leaving this as is.
While this is some more boilerplate code, it maintains the structure of the
existing code which is already extremely complex.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:5: #ifndef
SANDBOX_SRC_PROCESS_MITIGATIONS_WIN32K_POLICY_H__
On 2014/06/06 21:22:24, rvargas wrote:
> only one underscore at the end.

Done.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:30: SUBSYS_WIN32K_LOCKDOWN    // Win32K
Lockdown related policy.
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Remove this SubSystem. You actually shouldn't need to modify this file at all.

Leaving this on the same lines as the policy stuff.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: LOCKDOWN_WIN32K         // Locks down the
Win32K subsystem for the process.
On 2014/06/06 03:23:36, Justin Schuh wrote:
> Remove this Semantic value.
Ditto

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:25: #include
"sandbox/win/src/process_mitigations_win32k_policy.h"
On 2014/06/06 03:23:37, Justin Schuh wrote:
> Remove this include because you're removing the file.

Ditto

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:130: dispatcher = new
ProcessMitigationsWin32KDispatcher(this);
On 2014/06/06 21:22:24, rvargas wrote:
> I guess this is the core of the matter.
> 
> I'd like to keep the consistency... so I want to have a formal dispatcher,
even
> if it doesn't do anything on the broker side.
> 
> One way or another, this is a policy driven interception (even if there is no
> IPC), so we may do without a low level policy, but all the current
> infrastructure assumes a dispatcher means low level policy, so the option
would
> be change that.
> 
> On the other hand, we could do policy look up on the target, say to decide if
we
> should fake success or failure, but that is most likely overkill for what we
> need at this point (assuming we don't find out very soon that we need more
> interceptions).

Leaving this as is. Sort of agree that maintaining existing structure is better.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:310: mitigations_ = flags;
On 2014/06/06 21:22:24, rvargas wrote:
> If we don't want to have a separate SUBSYS_WIN32K_LOCKDOWN, we could also call
> ProcessMitigationsWin32KLockdownPolicy::GenerateRules from here after checking
> the right flag.

Not doing that. Leaving the SUBSYS_WIN32K_LOCKDOWN code as is. This is a touch
cleaner.

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:419: }
On 2014/06/06 03:23:37, Justin Schuh wrote:
> Remove this whole case.

Leaving this as is.

[rvargas (doing something else), 2014-06-09 20:05:57]

How about unit tests?

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_dispatcher.cc (right):

https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_dispatcher.cc:22: return true;
On 2014/06/06 23:57:35, ananta wrote:
> On 2014/06/06 21:22:24, rvargas wrote:
> > Shouldn't this be false?
> 
> No. If we return false the PolicyBase::SetupAllInterceptions function aborts
and
> causes the policies to not be applied fully
> causing the process launch to fail.
> 

But that's what we want, no? If by the time this policy is being applied
MITIGATION_WIN32K_DISABLE is not set, it means that there is a bug in the
policies so we want that failure to be as visible as possible.

https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: LOCKDOWN_WIN32K         // Locks down the
Win32K subsystem for the process.
nit: This seems a little weird. The subsystem is SUBSYS_WIN32K_LOCKDOWN because
it relates to things we may have to do when locking down win32k. But this
section is about what a rule would do when enabled: like allow read only access
to a file.

If what the rule does is "lockdown_win32k" then this is 100% redundant with the
mitigation flag.

So I think that we either have a name that reflects that we are enabling random
interception based helpers (and leave what to intercept and what to do for each
interception as implementation-defined), or use more than one "semantic" for
specific stuff that a consumer may decide to do. Note that all values here are
enablers of something that is disabled by some other mean. So in a sense, the
mitigation flag is the one that "locks down" stuff, and what we need here is
something that allow user32 to be loaded (for example).

[ananta, 2014-06-10 00:30:51]

On 2014/06/09 20:05:57, rvargas wrote:
> How about unit tests?
> 
>
https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
> File sandbox/win/src/process_mitigations_win32k_dispatcher.cc (right):
> 
>
https://codereview.chromium.org/318603003/diff/20001/sandbox/win/src/process_...
> sandbox/win/src/process_mitigations_win32k_dispatcher.cc:22: return true;
> On 2014/06/06 23:57:35, ananta wrote:
> > On 2014/06/06 21:22:24, rvargas wrote:
> > > Shouldn't this be false?
> > 
> > No. If we return false the PolicyBase::SetupAllInterceptions function aborts
> and
> > causes the policies to not be applied fully
> > causing the process launch to fail.
> > 
> 
> But that's what we want, no? If by the time this policy is being applied
> MITIGATION_WIN32K_DISABLE is not set, it means that there is a bug in the
> policies so we want that failure to be as visible as possible.
> 
>
https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
> File sandbox/win/src/sandbox_policy.h (right):
> 
>
https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
> sandbox/win/src/sandbox_policy.h:57: LOCKDOWN_WIN32K         // Locks down the
> Win32K subsystem for the process.
> nit: This seems a little weird. The subsystem is SUBSYS_WIN32K_LOCKDOWN
because
> it relates to things we may have to do when locking down win32k. But this
> section is about what a rule would do when enabled: like allow read only
access
> to a file.
> 
> If what the rule does is "lockdown_win32k" then this is 100% redundant with
the
> mitigation flag.
> 
> So I think that we either have a name that reflects that we are enabling
random
> interception based helpers (and leave what to intercept and what to do for
each
> interception as implementation-defined), or use more than one "semantic" for
> specific stuff that a consumer may decide to do. Note that all values here are
> enablers of something that is disabled by some other mean. So in a sense, the
> mitigation flag is the one that "locks down" stuff, and what we need here is
> something that allow user32 to be loaded (for example).

Added the following sbox_integration_tests.
1. CheckWin8Win32KLockdownSuccess/Failure. These tests set the mitigation with
and without the policy
   for faking user32/gdi32 initialization and succeed and fail accordingly.
2. Removed the code from the CheckWin8 sbox test which was also checking whether
the win32k lockdown mitigation
   is set. Moved this to the new CheckWin8Lockdown test function.


   and succeede

[ananta, 2014-06-10 00:32:10]

https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/40001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: LOCKDOWN_WIN32K         // Locks down the
Win32K subsystem for the process.
On 2014/06/09 20:05:57, rvargas wrote:
> nit: This seems a little weird. The subsystem is SUBSYS_WIN32K_LOCKDOWN
because
> it relates to things we may have to do when locking down win32k. But this
> section is about what a rule would do when enabled: like allow read only
access
> to a file.
> 
> If what the rule does is "lockdown_win32k" then this is 100% redundant with
the
> mitigation flag.
> 
> So I think that we either have a name that reflects that we are enabling
random
> interception based helpers (and leave what to intercept and what to do for
each
> interception as implementation-defined), or use more than one "semantic" for
> specific stuff that a consumer may decide to do. Note that all values here are
> enablers of something that is disabled by some other mean. So in a sense, the
> mitigation flag is the one that "locks down" stuff, and what we need here is
> something that allow user32 to be loaded (for example).

Renamed this to FAKE_USER32_GDI32_INIT

[rvargas (doing something else), 2014-06-10 03:11:18]

Looks good

https://codereview.chromium.org/318603003/diff/60001/content/common/sandbox_w...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/318603003/diff/60001/content/common/sandbox_w...
content/common/sandbox_win.cc:614: L"FakeUserGdiInit") != sandbox::SBOX_ALL_OK)
{
Does it work passing a NULL string?

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (left):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:106: if
(!CheckWin8Win32CallPolicy())
Why is this not failing? Do we have win8 bots?

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:214: // the target process causes
the launch to fail in process initialization.
Nit: add "The test process itself links against user/gdi"

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:222:
EXPECT_EQ(policy->SetProcessMitigations(MITIGATION_WIN32K_DISABLE),
SBOX_ALL_OK);
80 columns

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:236:
EXPECT_EQ(policy->SetProcessMitigations(MITIGATION_WIN32K_DISABLE),
SBOX_ALL_OK);
80 cols

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:239: L"FakeUserGdiInit"),
sandbox::SBOX_ALL_OK);
NULL str?

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.cc (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:10: const wchar_t* name,
nit: indent 4 spaces

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:13: 
nit: remove this line

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.h (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:12: #include
"sandbox/win/src/security_level.h"
It doesn't look like this is needed by this header.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: FAKE_USER32_GDI32_INIT // Fakes user32 and
gdi32 initialization. This is
nit: can we get rid of the 32s? as in FAKE_USER_GDI_INIT

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:58: // used to get the sandboxed process to
launch if
How about:
"Fakes user32 and gdi32 initialization. This can be used to allow the DLLs to
load even if the process cannot access that subsystem."

[ananta, 2014-06-10 21:48:07]

https://codereview.chromium.org/318603003/diff/60001/content/common/sandbox_w...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/318603003/diff/60001/content/common/sandbox_w...
content/common/sandbox_win.cc:614: L"FakeUserGdiInit") != sandbox::SBOX_ALL_OK)
{
On 2014/06/10 03:11:17, rvargas wrote:
> Does it work passing a NULL string?

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (left):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:106: if
(!CheckWin8Win32CallPolicy())
On 2014/06/10 03:11:17, rvargas wrote:
> Why is this not failing? Do we have win8 bots?

Sadly no. This regressed in my previous patch to add the win32k lockdown
mitigation based on a command line.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:214: // the target process causes
the launch to fail in process initialization.
On 2014/06/10 03:11:17, rvargas wrote:
> Nit: add "The test process itself links against user/gdi"

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:222:
EXPECT_EQ(policy->SetProcessMitigations(MITIGATION_WIN32K_DISABLE),
SBOX_ALL_OK);
On 2014/06/10 03:11:17, rvargas wrote:
> 80 columns

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:236:
EXPECT_EQ(policy->SetProcessMitigations(MITIGATION_WIN32K_DISABLE),
SBOX_ALL_OK);
On 2014/06/10 03:11:17, rvargas wrote:
> 80 cols

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:239: L"FakeUserGdiInit"),
sandbox::SBOX_ALL_OK);
On 2014/06/10 03:11:17, rvargas wrote:
> NULL str?

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.cc (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:10: const wchar_t* name,
On 2014/06/10 03:11:18, rvargas wrote:
> nit: indent 4 spaces

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.cc:13: 
On 2014/06/10 03:11:18, rvargas wrote:
> nit: remove this line

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_win32k_policy.h (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_win32k_policy.h:12: #include
"sandbox/win/src/security_level.h"
On 2014/06/10 03:11:18, rvargas wrote:
> It doesn't look like this is needed by this header.

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:57: FAKE_USER32_GDI32_INIT // Fakes user32 and
gdi32 initialization. This is
On 2014/06/10 03:11:18, rvargas wrote:
> nit: can we get rid of the 32s? as in FAKE_USER_GDI_INIT

Done.

https://codereview.chromium.org/318603003/diff/60001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:58: // used to get the sandboxed process to
launch if
On 2014/06/10 03:11:18, rvargas wrote:
> How about:
> "Fakes user32 and gdi32 initialization. This can be used to allow the DLLs to
> load even if the process cannot access that subsystem." 

Done.

[rvargas (doing something else), 2014-06-10 23:03:08]

LGTM after final nits.

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:240: SBOX_ALL_OK);
nit: indent under first arg (policy->)

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:243: NULL), sandbox::SBOX_ALL_OK);
nit: move NULL to the previous line, and the result to indent under policy->

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:58: // be used to allow the DLL's to load and
initialize
nit: no ' in DLLs

[ananta, 2014-06-10 23:15:57]

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:240: SBOX_ALL_OK);
On 2014/06/10 23:03:08, rvargas wrote:
> nit: indent under first arg (policy->)

Done.

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/process_...
sandbox/win/src/process_mitigations_test.cc:243: NULL), sandbox::SBOX_ALL_OK);
On 2014/06/10 23:03:08, rvargas wrote:
> nit: move NULL to the previous line, and the result to indent under policy->

Done.

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy.h (right):

https://codereview.chromium.org/318603003/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy.h:58: // be used to allow the DLL's to load and
initialize
On 2014/06/10 23:03:08, rvargas wrote:
> nit: no ' in DLLs

Done.

[ananta, 2014-06-11 02:07:17]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-06-11 02:08:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/318603003/100001

[ananta, 2014-06-11 02:42:20]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-06-11 02:44:16]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/318603003/120001

[ananta, 2014-06-11 03:05:56]

The CQ bit was unchecked by ananta@chromium.org

[ananta, 2014-06-11 03:07:06]

+jam for content\common owners stamp

[jschuh, 2014-06-11 06:31:17]

On 2014/06/11 03:07:06, ananta wrote:
> +jam for content\common owners stamp

I can lgtm that.

[jschuh, 2014-06-11 06:31:26]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-06-11 06:34:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/318603003/140001

[commit-bot: I haz the power, 2014-06-11 09:09:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[commit-bot: I haz the power, 2014-06-11 09:31:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-11 09:31:36]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/8...)

[jschuh, 2014-06-11 14:52:53]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-06-11 14:53:30]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/318603003/140001

[commit-bot: I haz the power, 2014-06-11 15:28:02]

Change committed as 276407