Add sandbox support for Windows process mitigations

Add sandbox support for Windows process mitigations 

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

[jschuh, 2012-06-30 23:49:50]

For this pass I don't think there's any need to add the attributes vector. So, I
just set up the version-based handling for the attributes and set the process
mitigation stuff.

[cpu_(ooo_6.6-7.5), 2012-07-02 21:42:32]

http://codereview.chromium.org/10690058/diff/11003/sandbox/src/target_process.cc
File sandbox/src/target_process.cc (right):

http://codereview.chromium.org/10690058/diff/11003/sandbox/src/target_process...
sandbox/src/target_process.cc:72: startup_info_.StartupInfo.cb =
sizeof(STARTUPINFOEX);
sizeof(startup_info_)

http://codereview.chromium.org/10690058/diff/11003/sandbox/src/target_process...
sandbox/src/target_process.cc:119: PVOID value,
PVOID --> void*

Interestingly we also mix SIZE_T and size_t in this file. I'll let that go :)

http://codereview.chromium.org/10690058/diff/11003/sandbox/src/target_process...
sandbox/src/target_process.cc:257: if (base::win::GetVersion() >=
base::win::VERSION_WIN7) {
space after line 256

http://codereview.chromium.org/10690058/diff/11003/sandbox/src/target_process...
sandbox/src/target_process.cc:261:
PROCESS_CREATION_MITIGATION_POLICY_DEP_ATL_THUNK_ENABLE |
I think these flags need to be passed as an argument. We can harcode them one
level up for the time being.

For example dep_atl_thunk_enable weakens the protection and it is not necessary
for renderers, which should not be using ATL window thunking.

Check also sandbox::SetCurrentProcessDEP in startup_helper_win.cc
we should unify with that as well.

[jschuh, 2012-09-05 20:55:28]

Have at it. :)

[cpu_(ooo_6.6-7.5), 2012-09-06 19:46:15]

more to come, but you can start mulling these ones.

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/broker_ser...
File sandbox/win/src/broker_services.cc (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:354: #endif
this would be clearer to me if instead you have some repetition. Liek

#ifdef _WIN64
.. all 64n part here
#else
.. all 32b part here
#endif

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.h (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:14: // Standard Windows mitigations
So I like the idea of having our own flags but that means that we need to do
some brief explaining on what they do in deference with all the 3rd parties that
use this.

For example mitigation_heap_terminate is not very descriptive by itself

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:34: const uint64 MITIGATION_DLL_SEARCH    
               = 0x00000001ULL << 32;
dll_search_order ? or restricted_dll_search ?

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:36: 
nuke extra space

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy.h (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:172: 
The flags are our flags but the comment  can take the user to the msdn ones by
googling PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY. 

But more generally, the mitigation flags need to be here or in a file that is
more 'interfacy' than process_mitigations.h. So what I propose is either:

1) move the flags here
2) create a mitigations header like security_level.h

[jschuh, 2012-09-07 01:14:22]

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/broker_ser...
File sandbox/win/src/broker_services.cc (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:354: #endif
On 2012/09/06 19:46:15, cpu wrote:
> this would be clearer to me if instead you have some repetition. Liek
> 
> #ifdef _WIN64
> .. all 64n part here
> #else
> .. all 32b part here
> #endif

Done.

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.h (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:14: // Standard Windows mitigations
On 2012/09/06 19:46:15, cpu wrote:
> So I like the idea of having our own flags but that means that we need to do
> some brief explaining on what they do in deference with all the 3rd parties
that
> use this.
> 
> For example mitigation_heap_terminate is not very descriptive by itself

Done.

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:34: const uint64 MITIGATION_DLL_SEARCH    
               = 0x00000001ULL << 32;
On 2012/09/06 19:46:15, cpu wrote:
> dll_search_order ? or restricted_dll_search ?

Done.

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:36: 
On 2012/09/06 19:46:15, cpu wrote:
> nuke extra space

Done.

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy.h (right):

http://codereview.chromium.org/10690058/diff/40051/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:172: 
On 2012/09/06 19:46:15, cpu wrote:
> The flags are our flags but the comment  can take the user to the msdn ones by
> googling PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY. 
> 
> But more generally, the mitigation flags need to be here or in a file that is
> more 'interfacy' than process_mitigations.h. So what I propose is either:
> 
> 1) move the flags here
> 2) create a mitigations header like security_level.h

Done.

[cpu_(ooo_6.6-7.5), 2012-09-07 19:22:55]

lgtm

http://codereview.chromium.org/10690058/diff/45032/content/app/startup_helper...
File content/app/startup_helper_win.cc (right):

http://codereview.chromium.org/10690058/diff/45032/content/app/startup_helper...
content/app/startup_helper_win.cc:43: // Ensure the proper mitigations are
enforced for the broker process.
consider using browser instead of broker when in chrome land.

http://codereview.chromium.org/10690058/diff/45032/content/common/sandbox_pol...
File content/common/sandbox_policy.cc (right):

http://codereview.chromium.org/10690058/diff/45032/content/common/sandbox_pol...
content/common/sandbox_policy.cc:736: // TODO(jschuh): Make NaCl work with DEP
and SEHOP
period

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/sandbox_win.gypi
File sandbox/win/sandbox_win.gypi (right):

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/sandbox_win.gy...
sandbox/win/sandbox_win.gypi:243: 'src/process_mitigations_test.cc',
tabby tab tab tab :)

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.cc (right):

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:34: HMODULE module =
::GetModuleHandleW(L"kernel32.dll");
use the Ansi version. This one of the rare cases where the ansi version is the
true one.

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:101: 
line 52 can move down here?

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:102: // This is all we can do in Win7 and
below
period

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:106: // Load the Win8 API on demand.
remove comment line 106

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:231: // Windows XP SP2
use IsXPSP2OrLater()? or maybe the comment is wrong.

http://codereview.chromium.org/10690058/diff/45032/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:288: // These flags must be set after
startup.
comment seems wrong, also the function instead of 'Can' would rather be 'Should'
or 'Must' ?

[jschuh, 2012-09-07 20:23:14]

https://chromiumcodereview.appspot.com/10690058/diff/45032/content/app/startu...
File content/app/startup_helper_win.cc (right):

https://chromiumcodereview.appspot.com/10690058/diff/45032/content/app/startu...
content/app/startup_helper_win.cc:43: // Ensure the proper mitigations are
enforced for the broker process.
On 2012/09/07 19:22:55, cpu wrote:
> consider using browser instead of broker when in chrome land.

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/content/common/san...
File content/common/sandbox_policy.cc (right):

https://chromiumcodereview.appspot.com/10690058/diff/45032/content/common/san...
content/common/sandbox_policy.cc:736: // TODO(jschuh): Make NaCl work with DEP
and SEHOP
On 2012/09/07 19:22:55, cpu wrote:
> period

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/sandbo...
File sandbox/win/sandbox_win.gypi (right):

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/sandbo...
sandbox/win/sandbox_win.gypi:243: 'src/process_mitigations_test.cc',
On 2012/09/07 19:22:55, cpu wrote:
> tabby tab tab tab :)

Done (but somehow I'm gonna blame this on gcl).

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
File sandbox/win/src/process_mitigations.cc (right):

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
sandbox/win/src/process_mitigations.cc:34: HMODULE module =
::GetModuleHandleW(L"kernel32.dll");
On 2012/09/07 19:22:55, cpu wrote:
> use the Ansi version. This one of the rare cases where the ansi version is the
> true one.

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
sandbox/win/src/process_mitigations.cc:101: 
On 2012/09/07 19:22:55, cpu wrote:
> line 52 can move down here?

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
sandbox/win/src/process_mitigations.cc:102: // This is all we can do in Win7 and
below
On 2012/09/07 19:22:55, cpu wrote:
> period

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
sandbox/win/src/process_mitigations.cc:106: // Load the Win8 API on demand.
On 2012/09/07 19:22:55, cpu wrote:
> remove comment line 106

Done.

https://chromiumcodereview.appspot.com/10690058/diff/45032/sandbox/win/src/pr...
sandbox/win/src/process_mitigations.cc:231: // Windows XP SP2
On 2012/09/07 19:22:55, cpu wrote:
> use IsXPSP2OrLater()? or maybe the comment is wrong.

Done.

[rvargas (doing something else), 2012-09-08 02:23:32]

Mostly nits.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
File sandbox/win/src/broker_services.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:335: return SBOX_ERROR_GENERIC;
Use a specific error code (and below)

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:344: #ifndef _WIN64
#if defined(_WIN64)

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:346: if (base::win::GetVersion() <
base::win::VERSION_WIN8) {
Can we send all this logic somewhere else?

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/nt_interna...
File sandbox/win/src/nt_internals.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/nt_interna...
sandbox/win/src/nt_internals.h:7: #ifndef SANDBOX_SRC_NT_INTERNALS_H__
could you fix this?

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:37: if (!IsXPSP2OrLater())
nit: bail out before line 34.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:59: #ifndef _WIN64  // DEP is always
enabled on 64-bit.
nit: #if !defined

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:222: // Windows 8.
nit: redundant comment?

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:252: #ifndef _WIN64
ditto

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:15: uint64
GetPostStartupProcessMitigations(uint64 flags);
It is not clear, from reading this file, what are the arguments to these
functions.

It looks like you mean that uint64 is a sandbox native type for flags, and
DWORD64 is an OS flags arrangement, but that is not spelled anywhere... May I
suggest a typedef for the sandbox flags so that at least the caller knows what
should pass?... or adding comments about the expected arguments.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:15: uint64
GetPostStartupProcessMitigations(uint64 flags);
btw, make sure the order on the header matches the order on the cc.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:19: DWORD64
GetProcessMitigationPolicyFlags(uint64 flags);
This looks really weird... convert from uint64 to uint64 but using two names for
that. And sadly requires the inclusion of windows.h just for the definition of
DWORD64 (not that it really matters).

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations_test.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations_test.cc:99: if (!CheckWin8AslrPolicy())
Did you considered just writing independent tests? As an added bonus, failures
will be clear on the bots.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations_test.cc:139: #ifndef _WIN64  // DEP is
always enabled on 64-bit.
ditto

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:179: static const uint64 MITIGATION_DEP        
                     = 0x00000001;
make this an enum instead? (yes, the actual variable still have to be a uint)

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:179: static const uint64 MITIGATION_DEP        
                     = 0x00000001;
We have 12 flags and declare a 64 bit flag? why? There's still plenty of room
even if MS keeps adding flags.

(another benefit of an enum + typedef instead of hardcoding the type)

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:179: static const uint64 MITIGATION_DEP        
                     = 0x00000001;
Candidate for moving somewhere else (policy level?,yeah, not a level). This is a
lot of noise in the middle of an interface (in any case, not the right place)

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy_base.cc:57: 
remove

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy_base.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy_base.h:55: virtual ResultCode
SetProcessMitigations(uint64 flags) OVERRIDE;
There is a little something between calling this method SetProcesblah (and I
think the name is correct), and also calling the function that actually applies
the settings SetProcessBlah... Maybe change the other one to ApplyFoo?

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/target_ser...
File sandbox/win/src/target_services.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/target_ser...
sandbox/win/src/target_services.cc:65: 
remove

[Peter Kasting, 2012-09-09 02:59:45]

Drive-by: Please add a BUG= link and if possible a more descriptive... uh...
description.

[jschuh, 2012-09-10 23:58:48]

Okay, did a little bit of shuffling. Responses inline.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
File sandbox/win/src/broker_services.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:335: return SBOX_ERROR_GENERIC;
On 2012/09/08 02:23:32, rvargas wrote:
> Use a specific error code (and below)

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:344: #ifndef _WIN64
On 2012/09/08 02:23:32, rvargas wrote:
> #if defined(_WIN64)

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/broker_ser...
sandbox/win/src/broker_services.cc:346: if (base::win::GetVersion() <
base::win::VERSION_WIN8) {
On 2012/09/08 02:23:32, rvargas wrote:
> Can we send all this logic somewhere else?

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/nt_interna...
File sandbox/win/src/nt_internals.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/nt_interna...
sandbox/win/src/nt_internals.h:7: #ifndef SANDBOX_SRC_NT_INTERNALS_H__
On 2012/09/08 02:23:32, rvargas wrote:
> could you fix this?

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:37: if (!IsXPSP2OrLater())
On 2012/09/08 02:23:32, rvargas wrote:
> nit: bail out before line 34.

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:59: #ifndef _WIN64  // DEP is always
enabled on 64-bit.
On 2012/09/08 02:23:32, rvargas wrote:
> nit: #if !defined

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:222: // Windows 8.
On 2012/09/08 02:23:32, rvargas wrote:
> nit: redundant comment?

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:252: #ifndef _WIN64
On 2012/09/08 02:23:32, rvargas wrote:
> ditto

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:19: DWORD64
GetProcessMitigationPolicyFlags(uint64 flags);
On 2012/09/08 02:23:32, rvargas wrote:
> This looks really weird... convert from uint64 to uint64 but using two names
for
> that. And sadly requires the inclusion of windows.h just for the definition of
> DWORD64 (not that it really matters).

I need windows.h for the HANDLE regardless. However, the variables are clearer
now due to the typedef.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations_test.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations_test.cc:99: if (!CheckWin8AslrPolicy())
On 2012/09/08 02:23:32, rvargas wrote:
> Did you considered just writing independent tests? As an added bonus, failures
> will be clear on the bots.

Yeah, but given the ever-increasing flakiness of the bots I feel better about
spawning fewer processes if they're not needed.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations_test.cc:139: #ifndef _WIN64  // DEP is
always enabled on 64-bit.
On 2012/09/08 02:23:32, rvargas wrote:
> ditto

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy.h:179: static const uint64 MITIGATION_DEP        
                     = 0x00000001;
On 2012/09/08 02:23:32, rvargas wrote:
> make this an enum instead? (yes, the actual variable still have to be a uint)

I split custom flags and Windows mitigations on the 32-bit boundary (e.g.
MITIGATION_DLL_SEARCH_ORDER) because it seemed unintuitive to separate them, and
I anticipate crossing 32-bits quickly. However, They're now shoved into a
typedef.

I've moved the flag values and typdef into security_level.h, which made sense
because that's we're we've centralized the other mitigation stuff.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy_base.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy_base.cc:57: 
On 2012/09/08 02:23:32, rvargas wrote:
> remove

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
File sandbox/win/src/sandbox_policy_base.h (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/sandbox_po...
sandbox/win/src/sandbox_policy_base.h:55: virtual ResultCode
SetProcessMitigations(uint64 flags) OVERRIDE;
On 2012/09/08 02:23:32, rvargas wrote:
> There is a little something between calling this method SetProcesblah (and I
> think the name is correct), and also calling the function that actually
applies
> the settings SetProcessBlah... Maybe change the other one to ApplyFoo?

Done.

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/target_ser...
File sandbox/win/src/target_services.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/target_ser...
sandbox/win/src/target_services.cc:65: 
On 2012/09/08 02:23:32, rvargas wrote:
> remove

Done.

[jschuh, 2012-09-11 04:38:53]

@jam - If you want to take a look at the content changes please go ahead.
They're all mechanical, so I was planning to just TBR before committing
tomorrow.

[jam, 2012-09-11 15:43:23]

lgtm

[commit-bot: I haz the power, 2012-09-12 02:26:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/10690058/50042

[commit-bot: I haz the power, 2012-09-12 02:26:39]

Presubmit check for 10690058-50042 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  sandbox/win/src/nt_internals.h \
  sandbox/win/src/sandbox_types.h

Presubmit checks took 1.8s to calculate.

[commit-bot: I haz the power, 2012-09-13 00:13:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/10690058/39071

[commit-bot: I haz the power, 2012-09-13 00:13:54]

Can't process patch for file sandbox/win/src/process_mitigations.cc.
Diff seems corrupted.

[commit-bot: I haz the power, 2012-09-13 00:20:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jschuh@chromium.org/10690058/46057

[commit-bot: I haz the power, 2012-09-13 00:20:46]

Presubmit check for 10690058-46057 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  sandbox/win/src/nt_internals.h \
  sandbox/win/src/sandbox_types.h

Presubmit checks took 4.9s to calculate.

[rvargas (doing something else), 2012-09-13 19:15:26]

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations_test.cc (right):

http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations_test.cc:99: if (!CheckWin8AslrPolicy())
On 2012/09/10 23:58:48, Justin Schuh wrote:
> On 2012/09/08 02:23:32, rvargas wrote:
> > Did you considered just writing independent tests? As an added bonus,
failures
> > will be clear on the bots.
> 
> Yeah, but given the ever-increasing flakiness of the bots I feel better about
> spawning fewer processes if they're not needed.

That's the wrong way to look at it. The cause of flakiness should be addressed
by itself... and a single flake on this test will disable all test at the same
time :(

As a general rule, test should be as small and specific as reasonable.

http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.h (right):

http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.h:28: DWORD64* policy_flags, size_t* size);
This is a weird API because it asks for a specific size (DWORD64) and then
returns a size :(.

http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/security_l...
File sandbox/win/src/security_level.h (right):

http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/security_l...
sandbox/win/src/security_level.h:137: const MitigationFlags MITIGATION_DEP      
                       = 0x00000001;
I'm sorry to insist on this one, but please use an enum instead of a bunch of
consts. And don't jump to 64 bits until we actually need that (if we ever use
all lower bits).

[jschuh, 2012-09-13 20:27:25]

Rick and I talked in person and cleared up some confusion. He's good with
landing the API as is now, and finishing off the mitigation tightening work.
After that I'm going to refactor the API a bit to better hide the internal guts.

On 2012/09/13 19:15:26, rvargas wrote:
>
http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
> File sandbox/win/src/process_mitigations_test.cc (right):
> 
>
http://codereview.chromium.org/10690058/diff/41066/sandbox/win/src/process_mi...
> sandbox/win/src/process_mitigations_test.cc:99: if (!CheckWin8AslrPolicy())
> On 2012/09/10 23:58:48, Justin Schuh wrote:
> > On 2012/09/08 02:23:32, rvargas wrote:
> > > Did you considered just writing independent tests? As an added bonus,
> failures
> > > will be clear on the bots.
> > 
> > Yeah, but given the ever-increasing flakiness of the bots I feel better
about
> > spawning fewer processes if they're not needed.
> 
> That's the wrong way to look at it. The cause of flakiness should be addressed
> by itself... and a single flake on this test will disable all test at the same
> time :(
> 
> As a general rule, test should be as small and specific as reasonable.
> 
>
http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/process_mi...
> File sandbox/win/src/process_mitigations.h (right):
> 
>
http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/process_mi...
> sandbox/win/src/process_mitigations.h:28: DWORD64* policy_flags, size_t*
size);
> This is a weird API because it asks for a specific size (DWORD64) and then
> returns a size :(.
> 
>
http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/security_l...
> File sandbox/win/src/security_level.h (right):
> 
>
http://codereview.chromium.org/10690058/diff/49105/sandbox/win/src/security_l...
> sandbox/win/src/security_level.h:137: const MitigationFlags MITIGATION_DEP    
 
>                        = 0x00000001;
> I'm sorry to insist on this one, but please use an enum instead of a bunch of
> consts. And don't jump to 64 bits until we actually need that (if we ever use
> all lower bits).