Enable DEP earlier on Vista and below

Enable DEP earlier on Vista and below

We can't enable DEP at launch prior to Win7, but we can queue an APC to enable immediately after the loader finishes.

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

[jschuh, 2012-10-03 02:06:10]

Rick, Carlos said he was okay with this if I convinced you of it. What do you
think?

[rvargas (doing something else), 2012-10-03 02:42:37]

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.cc (left):

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:255: MITIGATION_BOTTOM_UP_ASLR |
Are you losing these?

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

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:286: // policy immediately after then
loader finishes.
nit: the loader

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:289: if
(!::QueueUserAPC(ApplyMitigationsCallback, target->MainThread(),
I'm not fully convinced that this is a good idea, given that the process has not
started yet, and it is going to wake up through an APC. Yes, queuing another APC
_should_ not interfere with anything, but still... there's no guarantee that the
kernel won't use two APCs by itself (and that this one won't be in the middle).

On the other hand, what are we really gaining? We are applying DEP early on in
the process lifetime in any case, before any untrusted code can run... is your
concern our code, or the initialization code of all DLLs that we depend on?

[jschuh, 2012-10-03 03:00:06]

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
File sandbox/win/src/process_mitigations.cc (left):

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:255: MITIGATION_BOTTOM_UP_ASLR |
On 2012/10/03 02:42:37, rvargas wrote:
> Are you losing these?

No. The DEP flags don't need to be passed due to this patch, and the bottom-up
ASLR flags were a mistake.

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

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:286: // policy immediately after then
loader finishes.
On 2012/10/03 02:42:37, rvargas wrote:
> nit: the loader

Done.

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:289: if
(!::QueueUserAPC(ApplyMitigationsCallback, target->MainThread(),
On 2012/10/03 02:42:37, rvargas wrote:
> I'm not fully convinced that this is a good idea, given that the process has
not
> started yet, and it is going to wake up through an APC. Yes, queuing another
APC
> _should_ not interfere with anything, but still... there's no guarantee that
the
> kernel won't use two APCs by itself (and that this one won't be in the
middle).
> 
> On the other hand, what are we really gaining? We are applying DEP early on in
> the process lifetime in any case, before any untrusted code can run... is your
> concern our code, or the initialization code of all DLLs that we depend on?

That's pretty much it. I want to do it before we explicitly load any DLLs. And
after lockdown it doesn't really matter, because we won't be loading anything
anyway.

[rvargas (doing something else), 2012-10-03 22:23:57]

If you guys think that this is really worth it, I'm ok with it... so far I'm not
that convinced (especially given the APC)

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

http://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_mi...
sandbox/win/src/process_mitigations.cc:289: if
(!::QueueUserAPC(ApplyMitigationsCallback, target->MainThread(),
On 2012/10/03 03:00:06, Justin Schuh wrote:
> On 2012/10/03 02:42:37, rvargas wrote:
> > I'm not fully convinced that this is a good idea, given that the process has
> not
> > started yet, and it is going to wake up through an APC. Yes, queuing another
> APC
> > _should_ not interfere with anything, but still... there's no guarantee that
> the
> > kernel won't use two APCs by itself (and that this one won't be in the
> middle).
> > 
> > On the other hand, what are we really gaining? We are applying DEP early on
in
> > the process lifetime in any case, before any untrusted code can run... is
your
> > concern our code, or the initialization code of all DLLs that we depend on?
> 
> That's pretty much it. I want to do it before we explicitly load any DLLs. And
> after lockdown it doesn't really matter, because we won't be loading anything
> anyway.

But the bulk of the benefit is by protecting pages created after initialization,
right? I mean, the code executed at dll init should be minimal.

[rvargas (doing something else), 2012-10-11 02:00:11]

lgtm

[rvargas (doing something else), 2012-10-11 23:04:44]

https://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_m...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/10944015/diff/12001/sandbox/win/src/process_m...
sandbox/win/src/process_mitigations.cc:289: if
(!::QueueUserAPC(ApplyMitigationsCallback, target->MainThread(),
btw, this breaks with the SANDBOX_EXPORTS mode, but to be fair I wouldn't be
surprised if that's broken already.

[cpu_(ooo_6.6-7.5), 2012-10-16 22:37:17]

lgtm

[commit-bot: I haz the power, 2012-10-16 22:37:42]

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

[commit-bot: I haz the power, 2012-10-17 00:37:53]

Change committed as 162293