Add header comments documenting extra constraints for some sandbox mitigations: MITIGATION_RELOCATE…

Add header comments documenting extra constraints for some sandbox mitigations: MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in Debug builds, and MITIGATION_WIN32K_DISABLE often requires the FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/861e256723897757ff2023c0755524d0b7bbfb3e
Cr-Commit-Position: refs/heads/master@{#421410}

[Joe Mason, 2016-09-23 19:07:27]

Description was changed from

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=none
==========

to

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Joe Mason, 2016-09-23 19:09:34]

ptal

[Joe Mason, 2016-09-23 19:12:07]

joenotcharles@chromium.org changed reviewers:
+ wfh@chromium.org

[Joe Mason, 2016-09-23 19:12:08]

Actually adding a reviewer this time. PTAL.

[Will Harris, 2016-09-23 19:31:11]

https://codereview.chromium.org/2369563002/diff/20001/sandbox/win/src/securit...
File sandbox/win/src/security_level.h (right):

https://codereview.chromium.org/2369563002/diff/20001/sandbox/win/src/securit...
sandbox/win/src/security_level.h:163: // enabled after startup. Corresponds to
hmm if this is true and these can be set post startup for debug builds then I'd
like ProcessMitigationsTest.CheckWin8 to be updated to add this flag in debug
for SetDelayedProcessMitigations and verify this works.

[penny, 2016-09-23 21:54:53]

Description was changed from

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=none
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[Joe Mason, 2016-09-23 22:34:34]

https://codereview.chromium.org/2369563002/diff/20001/sandbox/win/src/securit...
File sandbox/win/src/security_level.h (right):

https://codereview.chromium.org/2369563002/diff/20001/sandbox/win/src/securit...
sandbox/win/src/security_level.h:163: // enabled after startup. Corresponds to
On 2016/09/23 19:31:10, Will Harris wrote:
> hmm if this is true and these can be set post startup for debug builds then
I'd
> like ProcessMitigationsTest.CheckWin8 to be updated to add this flag in debug
> for SetDelayedProcessMitigations and verify this works.

Done.

[Joe Mason, 2016-09-23 22:35:58]

The CQ bit was checked by joenotcharles@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-23 22:37:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-24 00:09:43]

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

[commit-bot: I haz the power, 2016-09-24 00:09:44]

Dry run: This issue passed the CQ dry run.

[Will Harris, 2016-09-26 18:31:38]

sorry for delay reviewing

https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:892: mitigations |=
kDebugDelayedMitigations;
do you really mean to add these mitigations twice, once on L884 and again here?

[Joe Mason, 2016-09-26 19:39:58]

https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:892: mitigations |=
kDebugDelayedMitigations;
On 2016/09/26 18:31:38, Will Harris wrote:
> do you really mean to add these mitigations twice, once on L884 and again
here?

That's NDEBUG, this is !NDEBUG. "DebugDelayedMitigations" are only delayed in
debug builds.

[Will Harris, 2016-09-27 21:21:34]

On 2016/09/26 19:39:58, Joe Mason wrote:
>
https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
> File sandbox/win/src/process_mitigations_test.cc (right):
> 
>
https://codereview.chromium.org/2369563002/diff/40001/sandbox/win/src/process...
> sandbox/win/src/process_mitigations_test.cc:892: mitigations |=
> kDebugDelayedMitigations;
> On 2016/09/26 18:31:38, Will Harris wrote:
> > do you really mean to add these mitigations twice, once on L884 and again
> here?
> 
> That's NDEBUG, this is !NDEBUG. "DebugDelayedMitigations" are only delayed in
> debug builds.

woops yes you're right :)

lgtm

[Joe Mason, 2016-09-27 23:35:21]

The CQ bit was checked by joenotcharles@chromium.org

[commit-bot: I haz the power, 2016-09-27 23:35:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 01:12:50]

Description was changed from

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2016-09-28 01:12:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-09-28 01:16:03]

Description was changed from

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Add header comments documenting extra constraints for some sandbox mitigations:
MITIGATION_RELOCATE_IMAGE and MITIGATION_RELOCATE_IMAGE_REQUIRED don't work in
Debug builds, and MITIGATION_WIN32K_DISABLE often requires the
FAKE_USER_GDI_INIT rule as well.

BUG=649827
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng

Committed: https://crrev.com/861e256723897757ff2023c0755524d0b7bbfb3e
Cr-Commit-Position: refs/heads/master@{#421410}
==========

[commit-bot: I haz the power, 2016-09-28 01:16:04]

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