Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1835003003:
[Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. (Closed)
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. #
Messages
Total messages: 50 (23 generated)
Description was changed from ========== [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) 3) 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. The tests added to sbox_integration_tests are MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) 3) 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. The tests added to sbox_integration_tests (ProcessMitigationsTest.CheckWin8ExtensionPoint*) are MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ==========
Description was changed from ========== [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) 3) 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. The tests added to sbox_integration_tests (ProcessMitigationsTest.CheckWin8ExtensionPoint*) are MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) 3) 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 MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ==========
brucedawson@chromium.org changed reviewers: + brucedawson@chromium.org
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#ne... sandbox/win/BUILD.gn:238: ldflags = [ "/SUBSYSTEM:WINDOWS" ] You need to replace the ldflags line with: configs -= [ "//build/config/win:console" ] configs += [ "//build/config/win:windowed" ] Your config list includes win:console by default so you have to remove that and add win:windowed instead of slamming in /SUBSYSTEM:WINDOWS. Welcome to gn! It is better once you learn it. See line 296/297 for another example in this file.
Description was changed from ========== [Windows Sandbox] MITIGATION_EXTENSION_POINT_DISABLE support for children. This CL is part of a chain of CLs: -> THIS 2) 3) 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 MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [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" () 5) "Turning on MITIGATION_EXTENSION_POINT_DISABLE" () 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 MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ==========
pennymac@chromium.org changed reviewers: + wfh@
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#ne... > sandbox/win/BUILD.gn:238: ldflags = [ "/SUBSYSTEM:WINDOWS" ] > You need to replace the ldflags line with: > > configs -= [ "//build/config/win:console" ] > configs += [ "//build/config/win:windowed" ] > > Your config list includes win:console by default so you have to remove that and > add win:windowed instead of slamming in /SUBSYSTEM:WINDOWS. Welcome to gn! It is > better once you learn it. > > See line 296/297 for another example in this file. Thanks Bruce! Ok, this CL is ready to go wfh@. (I had to change the MANUAL tests back to DISABLED, as sbox tests don't seem to respect the MANUAL label either. It tries to auto run them anyways.) So for now they're disabled so they don't run on bots. Note that this CL does not do the final "turn on". That'll be in the last CL of the sequence (so it's easy to revert if necessary). Many thanks!
Description was changed from ========== [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" () 5) "Turning on MITIGATION_EXTENSION_POINT_DISABLE" () 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 MANUAL (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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [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" () 5) "Turning on MITIGATION_EXTENSION_POINT_DISABLE" () 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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ==========
pennymac@chromium.org changed reviewers: + wfh@chromium.org - brucedawson@chromium.org, wfh@
drive-by https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:10: #include <thread> Btw, I hate to the bearer of bad news but both <chrono> and <thread> are currently banned: http://chromium-cpp.appspot.com/#library-blacklist Sorry =/
first pass in general, you should try and use scoping variables as much as possible e.g. avoid managing the lifetime of anything, if you can - e.g. ScopedHandle, ScopedNativeLibrary are good here, avoid use of new/delete unless there really isn't another option. I think some of the shared names can be in a .h file and shared, this would also allow you to use decltype if you wanted to. I'll let you get back with comments to my comments then I'll have another pass next week. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:228: shared_library("sbox_integration_test_hook_dll") { loadable_module https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations.cc:140: // Enable system call policies. wrong comment? https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:122: // Spawn our Windows process (with or without mitigation enabled). style nit, feel free to ignore, but prefer not to use "we" and "our" in chromium code but active sense. e.g. Spawn Windows process (with or without mitigation enabled). but I can't find a citation for this (I just remember being told it) so up to you to decide to ignore me or not. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:167: HMODULE dll = NULL; chromium style is to declare variables just before they are used. yes, it's counter to how I always did it before, but that's life. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:179: dll = ::LoadLibraryW(L"sbox_integration_test_hook_dll.dll"); prefer ScopedNativeLibrary here rather than LoadLibraryW https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:184: ::GetProcAddress(dll, "WasHookCalled")); you could #define WasHookCalled in a .h file for your DLL then use decltype here... but could be overkill. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:202: std::this_thread::sleep_for(std::chrono::milliseconds(500)); base::PlatformThread::Sleep() and base::TimeDelta https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:210: EXPECT_EQ((is_success_test ? TRUE : FALSE), WasHookCalled()); true and false not TRUE and FALSE https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:224: EXPECT_TRUE(::FreeLibrary(dll)); this can go if you are using ScopedNativeLibrary https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:257: wchar_t* hook_dll = L"sbox_integration_test_hook_dll.dll"; const https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:260: wchar_t path[MAX_PATH]; not sure exactly what is happening here, you are getting the current module name then changing it to the dll path? Can you explain what is being done here - I suspect it could be done using base::FilePath operations rather than having to manually parse the paths using wcsrchr and wcsncat and messing with the raw strings? https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:270: wchar_t* short_name = new wchar_t[length]; use a scoped_ptr with a base::FreeDeleter rather than a new or, since this is a test you could just declare this as a stack buffer wchar_t short_name[MAX_PATH]; but we typically try and avoid managing lifetime of memory manually. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:309: app_init_key.WriteValue(enabled_value_name, static_cast<DWORD>(1))); can you just use 1U and avoid the cast here and above? https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:312: HANDLE event = ::CreateEventW(NULL, FALSE, FALSE, winproc_event); ScopedHandle https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:668: DWORD mutex_max_wait_ms = 10 * 1000; TestTimeouts::action_timeout() for defaults https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... File sandbox/win/tests/integration_tests/hooking_dll.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_dll.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_dll.cc:9: BOOL hook_called = false; why is this linker-fu needed? https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:9: const wchar_t* class_name = L"myWindowClass"; if these are shared they should be in a .h file and included in both places. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:44: HANDLE event = ::OpenEventW(EVENT_MODIFY_STATE, FALSE, winproc_event); scoped, unless a reason not to wait to link base? https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:68: L"ChromeMitigationTests", WS_OVERLAPPEDWINDOW, nit: formatting?
https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:10: #include <thread> On 2016/04/03 23:30:01, dcheng wrote: > Btw, I hate to the bearer of bad news but both <chrono> and <thread> are > currently banned: http://chromium-cpp.appspot.com/#library-blacklist > > Sorry =/ yes I noticed that too, suggested using base::PlatformThread::Sleep :) what are you doing on a Sunday reviewing CLs?!
On 2016/04/04 00:16:41, Will Harris wrote: > https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... > File sandbox/win/src/process_mitigations_test.cc (right): > > https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... > sandbox/win/src/process_mitigations_test.cc:10: #include <thread> > On 2016/04/03 23:30:01, dcheng wrote: > > Btw, I hate to the bearer of bad news but both <chrono> and <thread> are > > currently banned: http://chromium-cpp.appspot.com/#library-blacklist > > > > Sorry =/ > > yes I noticed that too, suggested using base::PlatformThread::Sleep :) > > what are you doing on a Sunday reviewing CLs?! Indeed, dcheng@ driving by on a Sunday! :D Thanks to you both for reviews. Sheriffing right now, so I'll go through this tomorrow.
Description was changed from ========== [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" () 5) "Turning on MITIGATION_EXTENSION_POINT_DISABLE" () 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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ==========
Description was changed from ========== [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) TEST=Manually run against Win8.1 x64, Win10 x64, Win10 x86. BUG=557798 ========== to ========== [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 ==========
Thanks for the first run Will! I've uploaded a suite of fixes. (Re-ran the manual tests locally, and am running the try bots now.) https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:228: shared_library("sbox_integration_test_hook_dll") { On 2016/04/04 00:15:08, Will Harris wrote: > loadable_module Done. Do you know what the difference between the two is? Both seem to work for a DLL... https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations.cc:140: // Enable system call policies. On 2016/04/04 00:15:08, Will Harris wrote: > wrong comment? Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:10: #include <thread> On 2016/04/04 00:16:41, Will Harris wrote: > On 2016/04/03 23:30:01, dcheng wrote: > > Btw, I hate to the bearer of bad news but both <chrono> and <thread> are > > currently banned: http://chromium-cpp.appspot.com/#library-blacklist > > > > Sorry =/ > > yes I noticed that too, suggested using base::PlatformThread::Sleep :) > > what are you doing on a Sunday reviewing CLs?! Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:122: // Spawn our Windows process (with or without mitigation enabled). On 2016/04/04 00:15:08, Will Harris wrote: > style nit, feel free to ignore, but prefer not to use "we" and "our" in chromium > code but active sense. e.g. > > Spawn Windows process (with or without mitigation enabled). > > but I can't find a citation for this (I just remember being told it) so up to > you to decide to ignore me or not. Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:167: HMODULE dll = NULL; On 2016/04/04 00:15:09, Will Harris wrote: > chromium style is to declare variables just before they are used. yes, it's > counter to how I always did it before, but that's life. Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:179: dll = ::LoadLibraryW(L"sbox_integration_test_hook_dll.dll"); On 2016/04/04 00:15:08, Will Harris wrote: > prefer ScopedNativeLibrary here rather than LoadLibraryW Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:184: ::GetProcAddress(dll, "WasHookCalled")); On 2016/04/04 00:15:08, Will Harris wrote: > you could #define WasHookCalled in a .h file for your DLL then use decltype > here... but could be overkill. Acknowledged. I've created a new common.h file with literal strings and these function definitions. If it was only one function, it wouldn't be worth it, but with other shared names, etc. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:202: std::this_thread::sleep_for(std::chrono::milliseconds(500)); On 2016/04/04 00:15:09, Will Harris wrote: > base::PlatformThread::Sleep() and base::TimeDelta Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:210: EXPECT_EQ((is_success_test ? TRUE : FALSE), WasHookCalled()); On 2016/04/04 00:15:08, Will Harris wrote: > true and false not TRUE and FALSE Done. The function WasHookCalled() was actually defined with BOOL return type. But I've now changed the hook DLL function to return bool. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:224: EXPECT_TRUE(::FreeLibrary(dll)); On 2016/04/04 00:15:08, Will Harris wrote: > this can go if you are using ScopedNativeLibrary Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:257: wchar_t* hook_dll = L"sbox_integration_test_hook_dll.dll"; On 2016/04/04 00:15:08, Will Harris wrote: > const Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:260: wchar_t path[MAX_PATH]; On 2016/04/04 00:15:08, Will Harris wrote: > not sure exactly what is happening here, you are getting the current module name > then changing it to the dll path? > > Can you explain what is being done here - I suspect it could be done using > base::FilePath operations rather than having to manually parse the paths using > wcsrchr and wcsncat and messing with the raw strings? I actually don't think FilePath helps at all. I've added some more comments though, and am using a vector instead of a pointer that requires deleting later. I need to get the absolute path to the currently running module, as that's where the right version of the appinit DLL will be. Then I need to convert that full path into short-name format for registry compat. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:270: wchar_t* short_name = new wchar_t[length]; On 2016/04/04 00:15:08, Will Harris wrote: > use a scoped_ptr with a base::FreeDeleter rather than a new > > or, since this is a test you could just declare this as a stack buffer > > wchar_t short_name[MAX_PATH]; > > but we typically try and avoid managing lifetime of memory manually. Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:309: app_init_key.WriteValue(enabled_value_name, static_cast<DWORD>(1))); On 2016/04/04 00:15:09, Will Harris wrote: > can you just use 1U and avoid the cast here and above? So the EXPECT_EQ macro (and the function call) is actually why the cast is needed. If I don't cast the '0' value to DWORD, it complains that it can't tell which overload of the WriteValue() function is being used. Doesn't matter whether the 'U' is added or not. The '1' value can definitely be not cast, but I'd rather have it match the call above for consistency! https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:312: HANDLE event = ::CreateEventW(NULL, FALSE, FALSE, winproc_event); On 2016/04/04 00:15:09, Will Harris wrote: > ScopedHandle Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:668: DWORD mutex_max_wait_ms = 10 * 1000; On 2016/04/04 00:15:08, Will Harris wrote: > TestTimeouts::action_timeout() for defaults So TestTimeouts::action_timeout() returns a base::TimeDelta. But I'm interacting with ::WaitForSingleObject(), which takes a DWORD number of milliseconds to wait for the object. I'm not sure I can use our TimeDelta objects when interacting with a system API. I just need to pass in a number of milliseconds. (?) https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... File sandbox/win/tests/integration_tests/hooking_dll.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_dll.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/04 00:15:09, Will Harris wrote: > no (c) Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_dll.cc:9: BOOL hook_called = false; On 2016/04/04 00:15:09, Will Harris wrote: > why is this linker-fu needed? Fair question. I've added some detailed comments. This makes a segment that is shared/accessible by every instance of the DLL (mapped into many processes for hooks). When I check whether "WasHookCalled()" in the test process, I'm actually calling into the version of the DLL in the test process - but I need the information from the version of the DLL that's in the separate WinProc address! This allows me to do so. Hook DLLs generally require some variables to be in a section like this for management. I wanted to add a cool little image to help visualize this, but no way to insert images in these reviews. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/04/04 00:15:09, Will Harris wrote: > no (c) Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:9: const wchar_t* class_name = L"myWindowClass"; On 2016/04/04 00:15:09, Will Harris wrote: > if these are shared they should be in a .h file and included in both places. Done. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:44: HANDLE event = ::OpenEventW(EVENT_MODIFY_STATE, FALSE, winproc_event); On 2016/04/04 00:15:09, Will Harris wrote: > scoped, unless a reason not to wait to link base? I'd prefer not to pull base into the hook dll or the winproc exe. It's overkill at this point (they're tiny right now). https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/tests/integ... sandbox/win/tests/integration_tests/hooking_win_proc.cc:68: L"ChromeMitigationTests", WS_OVERLAPPEDWINDOW, On 2016/04/04 00:15:09, Will Harris wrote: > nit: formatting? Done.
Also as discussed use of 'we' 'I' etc etc. Also, there is a difference between ASSERT and EXPECT in gtest - EXPECT will continue if it fails but ASSERT will immediately break execution. there's a number of places you are using EXPECT and it looks like if the code were to continue it would just crash e.g. getting procaddresses and stuff, sounds like a lot of these can be moved to ASSERT. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:228: shared_library("sbox_integration_test_hook_dll") { On 2016/04/11 22:11:58, penny wrote: > On 2016/04/04 00:15:08, Will Harris wrote: > > loadable_module > > Done. > > Do you know what the difference between the two is? Both seem to work for a > DLL... loadable_module makes a DLL shared_library makes a DLL but then adds delay load imports into anything that depends on it so you can call the exports without having to load the library and resolve the export. e.g. it's used by base.dll in a component build... https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:260: wchar_t path[MAX_PATH]; On 2016/04/11 22:11:59, penny wrote: > On 2016/04/04 00:15:08, Will Harris wrote: > > not sure exactly what is happening here, you are getting the current module > name > > then changing it to the dll path? > > > > Can you explain what is being done here - I suspect it could be done using > > base::FilePath operations rather than having to manually parse the paths using > > wcsrchr and wcsncat and messing with the raw strings? > > I actually don't think FilePath helps at all. I've added some more comments > though, and am using a vector instead of a pointer that requires deleting later. > > I need to get the absolute path to the currently running module, as that's where > the right version of the appinit DLL will be. Then I need to convert that full > path into short-name format for registry compat. I understand you need to call GetShortPathName as this isn't provided by FilePath, but I'm still uncomfortable with you looking for separatators and doing a wcsncat. This is exactly what base::FilePath is there for, can you just place the result of GetModuleFileName into a base::FilePath then use DirName() and Append() then pass result into GetShortPathName? https://codereview.chromium.org/1835003003/diff/100001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/100001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: #ifdef __cplusplus this is always true, can be removed
Patchset #7 (id:120001) has been deleted
Thanks Will! I've searched out all instances of " I ", "we", "our", "your" in this CL. Think I got them all. Let me know if you notice any others that I missed. The few new manual CheckWin8ExtensionPoint tests are forced to run one at a time using a named system mutex. As mentioned in the function comments, ASSERTS should not be used inside the tests, as cleanup and release of the mutex must happen. However, I have now updated the two main functions (TestWin8ExtensionPointHookWrapper() and TestWin8ExtensionPointAppInitWrapper()) to handle fatal failures. Ran the tests again - all good. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn File sandbox/win/BUILD.gn (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/BUILD.gn#ne... sandbox/win/BUILD.gn:228: shared_library("sbox_integration_test_hook_dll") { On 2016/04/12 19:39:12, Will Harris wrote: > On 2016/04/11 22:11:58, penny wrote: > > On 2016/04/04 00:15:08, Will Harris wrote: > > > loadable_module > > > > Done. > > > > Do you know what the difference between the two is? Both seem to work for a > > DLL... > > loadable_module makes a DLL > > shared_library makes a DLL but then adds delay load imports into anything that > depends on it so you can call the exports without having to load the library and > resolve the export. e.g. it's used by base.dll in a component build... TIL. Very useful information. Thanks for clarifying. https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/80001/sandbox/win/src/process... sandbox/win/src/process_mitigations_test.cc:260: wchar_t path[MAX_PATH]; On 2016/04/12 19:39:12, Will Harris wrote: > On 2016/04/11 22:11:59, penny wrote: > > On 2016/04/04 00:15:08, Will Harris wrote: > > > not sure exactly what is happening here, you are getting the current module > > name > > > then changing it to the dll path? > > > > > > Can you explain what is being done here - I suspect it could be done using > > > base::FilePath operations rather than having to manually parse the paths > using > > > wcsrchr and wcsncat and messing with the raw strings? > > > > I actually don't think FilePath helps at all. I've added some more comments > > though, and am using a vector instead of a pointer that requires deleting > later. > > > > I need to get the absolute path to the currently running module, as that's > where > > the right version of the appinit DLL will be. Then I need to convert that > full > > path into short-name format for registry compat. > > I understand you need to call GetShortPathName as this isn't provided by > FilePath, but I'm still uncomfortable with you looking for separatators and > doing a wcsncat. This is exactly what base::FilePath is there for, can you just > place the result of GetModuleFileName into a base::FilePath then use DirName() > and Append() then pass result into GetShortPathName? Done. I'm not really convinced this is much nicer, but not emotional about it - so happy to leave it like this. I'm slightly biased against FilePath, as I find the APIs un-intuitive and generally clumsy when trying to interact with windows system apis. For some reason it takes me way too long to figure out which FilePath apis to use, and what they actually return given the under-the-hood "thinking" it does (not entirely well imho). Then the whole "const" thing. Oh well. I've done as you asked. :) OMG it happened again. Append gets me again. You have to use the returned FilePath, it does not just append to the existing object. <sigh> Finally got it working again. https://codereview.chromium.org/1835003003/diff/100001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/100001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: #ifdef __cplusplus On 2016/04/12 19:39:13, Will Harris wrote: > this is always true, can be removed Done.
robertshield@chromium.org changed reviewers: + robertshield@chromium.org
This is really neat, some drive-by nits and suggestions https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:139: EXPECT_TRUE(false); Can you use ADD_FAILURE() here and below? https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:147: ::wcsdup(g_winproc_file)); I think this works and is a bit cleaner: base::string16 cmd_writeable(g_winproc_file)); if (!::CreateProcessW(NULL, &cmd_writeable[0], ... https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:230: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); Sleep() in tests is a common cause of flakiness: it's not clear that there's a guarantee that this would cause the hook handler to be called on a loaded down testing bot. A better (though bit more work) approach is to have the hook handler signal an Event (or some other synchronization object) that this code does a WaitForSingleObject() on with a reasonable, but much longer timeout, say 30 seconds or so. This will make your tests run as quickly as possible in the common case (not waiting unnecessarily for a second). They also won't flake unless the system is truly bogged down, in which case you can explicitly report on the Wait() having hit the timeout. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:241: // case for a non-global/targetted hook. s/targetted/targeted/ https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:256: // states nit: wrap https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:333: if (0 != orig_dlls.compare(L"")) !orig_dlls.empty() https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:380: wchar_t name[MAX_PATH]; = {} https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:755: DISABLED_CheckWin8ExtensionPoint_GlobalHook_Success) { double checking: is it intended that these tests are being added DISABLED? https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_dll.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_dll.cc:46: __in LPVOID lpvReserved) { nit: don't need the __in annotations, naming conventions usually suggest parameter names of: HINSTANCE instance, DWORD reason, LPVOID reserved https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_win_proc.cc:9: LRESULT CALLBACK WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { similar nits around parameter naming conventions, Chromium doesn't use the Hungarian notation prefixes (here and below)
Thank you very much for the drive-by Robert! Fixes are in. I re-ran the manual tests locally. wfh@, feel free to let fly with your latest comments now. :) https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:139: EXPECT_TRUE(false); On 2016/04/14 03:57:29, robertshield wrote: > Can you use ADD_FAILURE() here and below? Done. TIL. Thanks for telling me about that! https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:147: ::wcsdup(g_winproc_file)); On 2016/04/14 03:57:29, robertshield wrote: > I think this works and is a bit cleaner: > > base::string16 cmd_writeable(g_winproc_file)); > if (!::CreateProcessW(NULL, &cmd_writeable[0], ... > Done. Good one, thanks! I usually just think of the std::wstring functions returning const pointers, and wasn't sure the raw array was always first thing in the object. I'll use this from now on. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:230: base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); On 2016/04/14 03:57:29, robertshield wrote: > Sleep() in tests is a common cause of flakiness: it's not clear that there's a > guarantee that this would cause the hook handler to be called on a loaded down > testing bot. > > A better (though bit more work) approach is to have the hook handler signal an > Event (or some other synchronization object) that this code does a > WaitForSingleObject() on with a reasonable, but much longer timeout, say 30 > seconds or so. > > This will make your tests run as quickly as possible in the common case (not > waiting unnecessarily for a second). They also won't flake unless the system is > truly bogged down, in which case you can explicitly report on the Wait() having > hit the timeout. Done. <sigh> I know, I was being lazy because I knew these are manual tests. But I've gone ahead and added a proper hook event. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:241: // case for a non-global/targetted hook. On 2016/04/14 03:57:29, robertshield wrote: > s/targetted/targeted/ Done. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:256: // states On 2016/04/14 03:57:29, robertshield wrote: > nit: wrap Done. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:333: if (0 != orig_dlls.compare(L"")) On 2016/04/14 03:57:29, robertshield wrote: > !orig_dlls.empty() Done. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:380: wchar_t name[MAX_PATH]; On 2016/04/14 03:57:29, robertshield wrote: > = {} Done. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... sandbox/win/src/process_mitigations_test.cc:755: DISABLED_CheckWin8ExtensionPoint_GlobalHook_Success) { On 2016/04/14 03:57:29, robertshield wrote: > double checking: is it intended that these tests are being added DISABLED? Yes. Unfortunately, these extension point tests must run one at a time or they'll interfere with each other (hence the global named mutex held in each test). There's a possibility they could interfere with other random tests as well. But some of the tests also touch things like HKLM, and cannot be redirected like other registry tests (i.e. AppInit DLLs). I've been told that right now all tests running on our bots must run with redirection so as not to impact the bots or potentially other tests. So for now they are run manually, and two have to be run as admin as well. If I start writing more low-level security tests that don't fit the auto testing rules, I might push for special bots that'll run only "dangerous" security tests like this. But that's for the future. If you're wondering why these are DISABLED instead of MANUAL, it's because sbox_integration_tests do not currently handle these types of "gtest arguments" properly. It's on my list to debug. I can't even use the special argument to run disabled tests, it is ignored - I have to edit and remove the tag to run these tests manually. :S https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_dll.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_dll.cc:46: __in LPVOID lpvReserved) { On 2016/04/14 03:57:29, robertshield wrote: > nit: don't need the __in annotations, naming conventions usually suggest > parameter names of: HINSTANCE instance, DWORD reason, LPVOID reserved Done. https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_win_proc.cc:9: LRESULT CALLBACK WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { On 2016/04/14 03:57:29, robertshield wrote: > similar nits around parameter naming conventions, Chromium doesn't use the > Hungarian notation prefixes (here and below) Done.
On 2016/04/24 18:50:49, penny wrote: > Thank you very much for the drive-by Robert! > > Fixes are in. I re-ran the manual tests locally. > > wfh@, feel free to let fly with your latest comments now. :) > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > File sandbox/win/src/process_mitigations_test.cc (right): > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:139: EXPECT_TRUE(false); > On 2016/04/14 03:57:29, robertshield wrote: > > Can you use ADD_FAILURE() here and below? > > Done. TIL. Thanks for telling me about that! > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:147: ::wcsdup(g_winproc_file)); > On 2016/04/14 03:57:29, robertshield wrote: > > I think this works and is a bit cleaner: > > > > base::string16 cmd_writeable(g_winproc_file)); > > if (!::CreateProcessW(NULL, &cmd_writeable[0], ... > > > > Done. Good one, thanks! I usually just think of the std::wstring functions > returning const pointers, and wasn't sure the raw array was always first thing > in the object. I'll use this from now on. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:230: > base::PlatformThread::Sleep(base::TimeDelta::FromSeconds(1)); > On 2016/04/14 03:57:29, robertshield wrote: > > Sleep() in tests is a common cause of flakiness: it's not clear that there's a > > guarantee that this would cause the hook handler to be called on a loaded down > > testing bot. > > > > A better (though bit more work) approach is to have the hook handler signal an > > Event (or some other synchronization object) that this code does a > > WaitForSingleObject() on with a reasonable, but much longer timeout, say 30 > > seconds or so. > > > > This will make your tests run as quickly as possible in the common case (not > > waiting unnecessarily for a second). They also won't flake unless the system > is > > truly bogged down, in which case you can explicitly report on the Wait() > having > > hit the timeout. > > Done. <sigh> I know, I was being lazy because I knew these are manual tests. > But I've gone ahead and added a proper hook event. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:241: // case for a > non-global/targetted hook. > On 2016/04/14 03:57:29, robertshield wrote: > > s/targetted/targeted/ > > Done. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:256: // states > On 2016/04/14 03:57:29, robertshield wrote: > > nit: wrap > > Done. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:333: if (0 != > orig_dlls.compare(L"")) > On 2016/04/14 03:57:29, robertshield wrote: > > !orig_dlls.empty() > > Done. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:380: wchar_t name[MAX_PATH]; > On 2016/04/14 03:57:29, robertshield wrote: > > = {} > > Done. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/src/proces... > sandbox/win/src/process_mitigations_test.cc:755: > DISABLED_CheckWin8ExtensionPoint_GlobalHook_Success) { > On 2016/04/14 03:57:29, robertshield wrote: > > double checking: is it intended that these tests are being added DISABLED? > > Yes. Unfortunately, these extension point tests must run one at a time or > they'll interfere with each other (hence the global named mutex held in each > test). There's a possibility they could interfere with other random tests as > well. But some of the tests also touch things like HKLM, and cannot be > redirected like other registry tests (i.e. AppInit DLLs). I've been told that > right now all tests running on our bots must run with redirection so as not to > impact the bots or potentially other tests. > > So for now they are run manually, and two have to be run as admin as well. > > If I start writing more low-level security tests that don't fit the auto testing > rules, I might push for special bots that'll run only "dangerous" security tests > like this. But that's for the future. > > If you're wondering why these are DISABLED instead of MANUAL, it's because > sbox_integration_tests do not currently handle these types of "gtest arguments" > properly. It's on my list to debug. I can't even use the special argument to > run disabled tests, it is ignored - I have to edit and remove the tag to run > these tests manually. :S > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... > File sandbox/win/tests/integration_tests/hooking_dll.cc (right): > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... > sandbox/win/tests/integration_tests/hooking_dll.cc:46: __in LPVOID lpvReserved) > { > On 2016/04/14 03:57:29, robertshield wrote: > > nit: don't need the __in annotations, naming conventions usually suggest > > parameter names of: HINSTANCE instance, DWORD reason, LPVOID reserved > > Done. > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... > File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): > > https://codereview.chromium.org/1835003003/diff/140001/sandbox/win/tests/inte... > sandbox/win/tests/integration_tests/hooking_win_proc.cc:9: LRESULT CALLBACK > WndProc(HWND hwnd, UINT msg, WPARAM wParam, LPARAM lParam) { > On 2016/04/14 03:57:29, robertshield wrote: > > similar nits around parameter naming conventions, Chromium doesn't use the > > Hungarian notation prefixes (here and below) > > Done. Ping wfh@. (I'll pull in latest trunk soon, to make sure I've got the latest and have handled any clashes.)
lgtm % small nits https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_win_proc.cc:51: window_class.hbrBackground = (HBRUSH)(COLOR_WINDOW + 1); nit: c++ cast (e.g. static_cast<>) - is it even needed? also, can you use COLOR_WINDOWFRAME instead of COLOR_WINDOW + 1? https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: const DWORD g_hook_event_max_wait_ms = 3 * 1000; nit: you could use TestTimeouts::action_timeout() or something else, rather than hardcoding this (then it allows it to be overridden by the test framework)
Description was changed from ========== [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 ========== to ========== [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 ==========
I've pulled in latest trunk, and fixed the nits. Last try run now. https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/hooking_win_proc.cc (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/hooking_win_proc.cc:51: window_class.hbrBackground = (HBRUSH)(COLOR_WINDOW + 1); On 2016/05/18 09:02:29, Will Harris wrote: > nit: c++ cast (e.g. static_cast<>) - is it even needed? > > also, can you use COLOR_WINDOWFRAME instead of COLOR_WINDOW + 1? Yes, the cast is needed, otherwise "int" can't be assigned to type HBRUSH. But I've fixed it to actually be reinterpret_cast<>. Static_cast<> doesn't work here because HBRUSH is actually defined as "typedef HBRUSH__ *HBRUSH". ...and I made it COLOR_WINDOWFRAME. https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: const DWORD g_hook_event_max_wait_ms = 3 * 1000; On 2016/05/18 09:02:29, Will Harris wrote: > nit: you could use TestTimeouts::action_timeout() or something else, rather than > hardcoding this (then it allows it to be overridden by the test framework) Done.
still lgtm from me % nittynit https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: const DWORD g_hook_event_max_wait_ms = 3 * 1000; On 2016/06/14 21:45:10, penny wrote: > On 2016/05/18 09:02:29, Will Harris wrote: > > nit: you could use TestTimeouts::action_timeout() or something else, rather > than > > hardcoding this (then it allows it to be overridden by the test framework) > > Done. looks like this was just commented out, can you remove it entirely?
Patchset #10 (id:200001) has been deleted
Patchset #11 (id:240001) has been deleted
Patchset #10 (id:220001) has been deleted
Hi Will, Thanks very much. I think this is the final update - please have a quick diff. Try run is green. I'll commit if you give me two thumbs up. https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... File sandbox/win/tests/integration_tests/integration_tests_common.h (right): https://codereview.chromium.org/1835003003/diff/160001/sandbox/win/tests/inte... sandbox/win/tests/integration_tests/integration_tests_common.h:40: const DWORD g_hook_event_max_wait_ms = 3 * 1000; On 2016/06/14 21:47:36, Will Harris wrote: > On 2016/06/14 21:45:10, penny wrote: > > On 2016/05/18 09:02:29, Will Harris wrote: > > > nit: you could use TestTimeouts::action_timeout() or something else, rather > > than > > > hardcoding this (then it allows it to be overridden by the test framework) > > > > Done. > > looks like this was just commented out, can you remove it entirely? <sigh> Thank you.
lgtm. please make sure to run the manual tests one last time.
On 2016/06/16 16:53:48, Will Harris wrote: > lgtm. please make sure to run the manual tests one last time. I absolutely did before this upload. Thanks!
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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_x6...)
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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_x6...)
The CQ bit was checked by pennymac@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
The CQ bit was unchecked by commit-bot@chromium.org
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_x6...)
The CQ bit was checked by jschuh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835003003/260001
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
Description was changed from ========== [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 ========== to ========== [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} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/c06d6fb1850d6217d35a5cccb1abccd6db0e7a2a Cr-Commit-Position: refs/heads/master@{#400422} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
