Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(75)

Issue 1835003003: [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. (Closed)

Created:
4 years, 9 months ago by penny
Modified:
4 years, 6 months ago
CC:
chromium-reviews, jschuh, rickyz+watch_chromium.org, wfh+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) "MITIGATION_EXTENSION_POINT_DISABLE emergency off finch" (https://codereview.chromium.org/1836523004/) 3) "New NT registry API" (https://codereview.chromium.org/1841573002) 4) "Early browser security support" (https://codereview.chromium.org/1656453002) 5) "Turn on MITIGATION_EXTENSION_POINT_DISABLE" (https://codereview.chromium.org/1854323002) Added support for this mitigation on child processes. Not turning on in this CL - will add in a tiny follow-up CL that is easy to revert if necessary. 6 out of 7 of the tests added to sbox_integration_tests (ProcessMitigationsTest.CheckWin8ExtensionPoint*) are DISABLED and should be run manually (will not auto run on bots). The following extension points are blocked by this policy: o AppInit DLLs o Winsock Layered Service Providers (LSPs) o Global Windows Hooks (not thread-targeted hooks) o Legacy Input Method Editors (IMEs) - note Chrome supports IMEs via extension (https://developer.chrome.com/extensions/input_ime). TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/c06d6fb1850d6217d35a5cccb1abccd6db0e7a2a Cr-Commit-Position: refs/heads/master@{#400422}

Patch Set 1 #

Patch Set 2 : GN fix. #

Patch Set 3 : Adding 2 new (missing) files. #

Patch Set 4 : Tiny cleanup - strange link error only on win8_chromium_ng try bot. #

Total comments: 1

Patch Set 5 : GN fix (thanks Bruce) #

Total comments: 49

Patch Set 6 : Code review fixes, part 1. #

Total comments: 2

Patch Set 7 : Code review fixes part 2. #

Total comments: 20

Patch Set 8 : Code review fixes, part 3 (robertshield). #

Total comments: 6

Patch Set 9 : Code review fixes, part 4 (wfh). #

Patch Set 10 : Final fixes and nits. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+758 lines, -53 lines) Patch
M sandbox/win/BUILD.gn View 1 2 3 4 5 6 7 8 2 chunks +20 lines, -0 lines 0 comments Download
M sandbox/win/sandbox_win.gypi View 1 2 3 4 5 6 7 8 3 chunks +28 lines, -0 lines 0 comments Download
M sandbox/win/src/process_mitigations.cc View 1 2 3 4 5 7 8 3 chunks +4 lines, -4 lines 0 comments Download
M sandbox/win/src/process_mitigations_test.cc View 1 2 3 4 5 6 7 8 9 17 chunks +505 lines, -46 lines 0 comments Download
M sandbox/win/src/security_level.h View 1 chunk +7 lines, -3 lines 0 comments Download
A sandbox/win/tests/integration_tests/hooking_dll.cc View 1 2 3 4 5 6 7 8 9 1 chunk +64 lines, -0 lines 0 comments Download
A sandbox/win/tests/integration_tests/hooking_win_proc.cc View 1 2 3 4 5 6 7 8 1 chunk +87 lines, -0 lines 0 comments Download
A sandbox/win/tests/integration_tests/integration_tests_common.h View 1 2 3 4 5 6 7 8 9 1 chunk +43 lines, -0 lines 0 comments Download

Messages

Total messages: 50 (23 generated)
brucedawson
https://codereview.chromium.org/1835003003/diff/60001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/1835003003/diff/60001/sandbox/win/BUILD.gn#newcode238 sandbox/win/BUILD.gn:238: ldflags = [ "/SUBSYSTEM:WINDOWS" ] You need to replace ...
4 years, 8 months ago (2016-03-29 20:56:23 UTC) #4
penny
On 2016/03/29 20:56:23, brucedawson wrote: > https://codereview.chromium.org/1835003003/diff/60001/sandbox/win/BUILD.gn > File sandbox/win/BUILD.gn (right): > > https://codereview.chromium.org/1835003003/diff/60001/sandbox/win/BUILD.gn#newcode238 > ...
4 years, 8 months ago (2016-03-30 03:52:13 UTC) #7
dcheng
drive-by https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc#newcode10 sandbox/win/src/process_mitigations_test.cc:10: #include <thread> Btw, I hate to the bearer ...
4 years, 8 months ago (2016-04-03 23:30:01 UTC) #10
Will Harris
first pass in general, you should try and use scoping variables as much as possible ...
4 years, 8 months ago (2016-04-04 00:15:09 UTC) #11
Will Harris
https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc#newcode10 sandbox/win/src/process_mitigations_test.cc:10: #include <thread> On 2016/04/03 23:30:01, dcheng wrote: > Btw, ...
4 years, 8 months ago (2016-04-04 00:16:41 UTC) #12
penny
On 2016/04/04 00:16:41, Will Harris wrote: > https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc > File sandbox/win/src/process_mitigations_test.cc (right): > > https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process_mitigations_test.cc#newcode10 ...
4 years, 8 months ago (2016-04-04 03:49:25 UTC) #13
penny
Thanks for the first run Will! I've uploaded a suite of fixes. (Re-ran the manual ...
4 years, 8 months ago (2016-04-11 22:11:59 UTC) #16
Will Harris
Also as discussed use of 'we' 'I' etc etc. Also, there is a difference between ...
4 years, 8 months ago (2016-04-12 19:39:13 UTC) #17
penny
Thanks Will! I've searched out all instances of " I ", "we", "our", "your" in ...
4 years, 8 months ago (2016-04-13 22:52:27 UTC) #19
robertshield
This is really neat, some drive-by nits and suggestions https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/process_mitigations_test.cc File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/process_mitigations_test.cc#newcode139 sandbox/win/src/process_mitigations_test.cc:139: ...
4 years, 8 months ago (2016-04-14 03:57:29 UTC) #21
penny
Thank you very much for the drive-by Robert! Fixes are in. I re-ran the manual ...
4 years, 8 months ago (2016-04-24 18:50:49 UTC) #22
penny
On 2016/04/24 18:50:49, penny wrote: > Thank you very much for the drive-by Robert! > ...
4 years, 7 months ago (2016-05-11 21:36:12 UTC) #23
Will Harris
lgtm % small nits https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/integration_tests/hooking_win_proc.cc File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/integration_tests/hooking_win_proc.cc#newcode51 sandbox/win/tests/integration_tests/hooking_win_proc.cc:51: window_class.hbrBackground = (HBRUSH)(COLOR_WINDOW + 1); ...
4 years, 7 months ago (2016-05-18 09:02:30 UTC) #24
penny
I've pulled in latest trunk, and fixed the nits. Last try run now. https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/integration_tests/hooking_win_proc.cc File ...
4 years, 6 months ago (2016-06-14 21:45:10 UTC) #26
Will Harris
still lgtm from me % nittynit https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/integration_tests/integration_tests_common.h File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/integration_tests/integration_tests_common.h#newcode40 sandbox/win/tests/integration_tests/integration_tests_common.h:40: const DWORD g_hook_event_max_wait_ms ...
4 years, 6 months ago (2016-06-14 21:47:37 UTC) #27
penny
Hi Will, Thanks very much. I think this is the final update - please have ...
4 years, 6 months ago (2016-06-16 16:45:47 UTC) #31
Will Harris
lgtm. please make sure to run the manual tests one last time.
4 years, 6 months ago (2016-06-16 16:53:48 UTC) #32
penny
On 2016/06/16 16:53:48, Will Harris wrote: > lgtm. please make sure to run the manual ...
4 years, 6 months ago (2016-06-16 17:09:59 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
4 years, 6 months ago (2016-06-16 17:11:11 UTC) #35
commit-bot: I haz the power
Try jobs failed on following builders: win10_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/165)
4 years, 6 months ago (2016-06-16 19:23:26 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
4 years, 6 months ago (2016-06-16 19:30:32 UTC) #39
commit-bot: I haz the power
Try jobs failed on following builders: win10_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/169)
4 years, 6 months ago (2016-06-16 23:03:50 UTC) #41
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
4 years, 6 months ago (2016-06-17 02:02:52 UTC) #43
commit-bot: I haz the power
Try jobs failed on following builders: win10_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x64_rel_ng/builds/172)
4 years, 6 months ago (2016-06-17 06:00:11 UTC) #45
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
4 years, 6 months ago (2016-06-17 12:42:21 UTC) #47
commit-bot: I haz the power
Committed patchset #10 (id:260001)
4 years, 6 months ago (2016-06-17 13:45:33 UTC) #48
commit-bot: I haz the power
4 years, 6 months ago (2016-06-17 13:48:12 UTC) #50
Message was sent while issue was closed.
Patchset 10 (id:??) landed as
https://crrev.com/c06d6fb1850d6217d35a5cccb1abccd6db0e7a2a
Cr-Commit-Position: refs/heads/master@{#400422}

Powered by Google App Engine
This is Rietveld 408576698