Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 1626623003: [Win10 sandbox mitigations] Four new Win10 mitigations added. (Closed)

Created:
3 years, 8 months ago by penny
Modified:
3 years, 7 months ago
Reviewers:
jschuh, Will Harris
CC:
chromium-reviews, vmpstr+watch_chromium.org, grt+watch_chromium.org, jam, rickyz+watch_chromium.org, darin-cc_chromium.org, wfh+watch_chromium.org, scottmg, forshaw
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Win10 sandbox mitigations] Four new Win10 mitigations added. 1. Disable non-system font loading on >= WIN10 (MITIGATION_NONSYSTEM_FONT_DISABLE). 2. Disable image loads from remote devices on >= WIN10_TH2 (MITIGATION_IMAGE_LOAD_NO_REMOTE). 3. Disable loading images that are labelled low integrity mandatory on >= WIN10_TH2 (MITIGATION_IMAGE_LOAD_NO_LOW_LABEL). 4. Extra disabling of child process creation on >= WIN10_TH2. In BrokerServicesBase::SpawnTarget(), if JobLevel <= JOB_LIMITED_USER, set PROC_THREAD_ATTRIBUTE_CHILD_PROCESS_POLICY to PROCESS_CREATION_CHILD_PROCESS_RESTRICTED via UpdateProcThreadAttribute(). This CL enables all four mitigations on every Chrome process except for browser. sbox_integration_tests have also been updated appropriately. base::win::VERSION_WIN10_TH2 has been added to identify Threshold 2/1511/10586. BUG=504006 R=jschuh@chromium.org, wfh@chromium.org Committed: https://chromium.googlesource.com/chromium/src/+/441d852dbcb7b9b31328393c7e31562b1e268399

Patch Set 1 #

Total comments: 14

Patch Set 2 : Code review changes, part 1. #

Patch Set 3 : Code review changes, part 2. #

Total comments: 23

Patch Set 4 : Code review changes, part 3. "Use more base APIs." #

Total comments: 16

Patch Set 5 : Code review changes, part 4. "Getting close." #

Total comments: 7

Patch Set 6 : Code review changes, part 4.5. "Sooo close." #

Patch Set 7 : Code review changes, part 5. "Fix the nit." #

Unified diffs Side-by-side diffs Delta from patch set Stats (+628 lines, -55 lines) Patch
M base/win/windows_version.h View 1 1 chunk +8 lines, -7 lines 0 comments Download
M base/win/windows_version.cc View 1 1 chunk +5 lines, -1 line 0 comments Download
M content/common/sandbox_win.cc View 1 2 3 4 5 2 chunks +8 lines, -5 lines 0 comments Download
M sandbox/win/src/broker_services.cc View 1 2 3 4 chunks +21 lines, -5 lines 0 comments Download
M sandbox/win/src/process_mitigations.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M sandbox/win/src/process_mitigations.cc View 1 2 4 chunks +73 lines, -12 lines 0 comments Download
M sandbox/win/src/process_mitigations_test.cc View 1 2 3 4 5 6 6 chunks +462 lines, -11 lines 0 comments Download
M sandbox/win/src/sandbox_policy.h View 1 2 1 chunk +3 lines, -0 lines 0 comments Download
M sandbox/win/src/sandbox_policy_base.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M sandbox/win/src/sandbox_policy_base.cc View 1 2 2 chunks +5 lines, -1 line 0 comments Download
M sandbox/win/src/security_level.h View 1 2 3 chunks +14 lines, -2 lines 0 comments Download
M sandbox/win/tests/common/controller.h View 1 2 3 4 3 chunks +7 lines, -1 line 0 comments Download
M sandbox/win/tests/common/controller.cc View 1 3 chunks +19 lines, -9 lines 0 comments Download

Messages

Total messages: 29 (6 generated)
penny
Hello reviewers, As per the CL description, this adds 4 new Windows sandbox security mitigations. ...
3 years, 7 months ago (2016-01-25 19:12:12 UTC) #2
Will Harris
some initial comments. can you also run git cl format? thanks. https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mitigations.h File sandbox/win/src/process_mitigations.h (right): ...
3 years, 7 months ago (2016-01-25 19:32:36 UTC) #3
jschuh
https://codereview.chromium.org/1626623003/diff/1/base/win/windows_version.h File base/win/windows_version.h (right): https://codereview.chromium.org/1626623003/diff/1/base/win/windows_version.h#newcode33 base/win/windows_version.h:33: VERSION_WIN10_10586, // Version 1511, Threshold 2. This could be ...
3 years, 7 months ago (2016-01-25 23:53:51 UTC) #4
brucedawson
Let me know if this gets approved and is blocked on VS 2015. If necessary ...
3 years, 7 months ago (2016-01-26 21:04:48 UTC) #5
penny
Thanks Bruce! I'll keep in touch with you about this. Thanks Will and Justin, changes ...
3 years, 7 months ago (2016-01-26 22:37:10 UTC) #7
jschuh
On 2016/01/26 22:37:10, penny wrote: > Thanks Bruce! I'll keep in touch with you about ...
3 years, 7 months ago (2016-01-26 23:42:03 UTC) #8
penny
Ok, I've tied the new create-child-process restriction to the JobLevel in BrokerServicesBase::SpawnTarget(). I had to ...
3 years, 7 months ago (2016-01-27 01:22:42 UTC) #11
Will Harris
looking good. A lot of the time there is a base:: API that will do ...
3 years, 7 months ago (2016-01-27 02:04:27 UTC) #12
penny
Thanks much. Latest fixes uploaded. Also re-adding a few questions I had from the start ...
3 years, 7 months ago (2016-01-28 19:25:16 UTC) #13
jschuh
Lgtm on base/win and my other comments. Get wfh@ to sign off on his feedback ...
3 years, 7 months ago (2016-01-29 22:59:29 UTC) #14
Will Harris
almost there! On 2016/01/28 19:25:16, penny wrote: > 1) I've left all of the new ...
3 years, 7 months ago (2016-01-30 00:28:43 UTC) #15
penny
On 2016/01/30 00:28:43, Will Harris wrote: > almost there! > > On 2016/01/28 19:25:16, penny ...
3 years, 7 months ago (2016-02-01 20:43:22 UTC) #16
Will Harris
On 2016/02/01 20:43:22, penny wrote: > * So just for clarification, my question was independent ...
3 years, 7 months ago (2016-02-01 21:33:57 UTC) #17
penny
Thanks Will! 1) Regarding content\app\sandbox_helper_win.cc, InitializeSandboxInfo(): I'm going to leave this for a follow-up CL ...
3 years, 7 months ago (2016-02-02 00:10:01 UTC) #18
Will Harris
lgtm % one very tiny nit. https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/process_mitigations_test.cc File sandbox/win/src/process_mitigations_test.cc (right): https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/process_mitigations_test.cc#newcode178 sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code == 0); ...
3 years, 7 months ago (2016-02-02 01:19:18 UTC) #19
penny
On 2016/02/02 01:19:18, Will Harris wrote: > lgtm % one very tiny nit. > > ...
3 years, 7 months ago (2016-02-02 18:05:04 UTC) #20
penny
On 2016/02/02 18:05:04, penny wrote: > On 2016/02/02 01:19:18, Will Harris wrote: > > lgtm ...
3 years, 7 months ago (2016-02-03 00:03:51 UTC) #21
commit-bot: I haz the power
Patchset 7 (id:??) landed as https://crrev.com/441d852dbcb7b9b31328393c7e31562b1e268399 Cr-Commit-Position: refs/heads/master@{#373265}
3 years, 7 months ago (2016-02-03 17:36:22 UTC) #23
penny
Committed patchset #7 (id:140001) manually as 441d852dbcb7b9b31328393c7e31562b1e268399 (presubmit successful).
3 years, 7 months ago (2016-02-03 17:37:10 UTC) #25
penny
On 2016/02/03 17:37:10, penny wrote: > Committed patchset #7 (id:140001) manually as > 441d852dbcb7b9b31328393c7e31562b1e268399 (presubmit ...
3 years, 7 months ago (2016-02-03 18:47:52 UTC) #26
Nico
clang/win complains: FAILED: ninja -t msvc -e environment.x64 -- "..\..\third_party/llvm-build/Release+Asserts/bin/clang-cl" -m64 /nologo /showIncludes /FC @obj\sandbox\win\src\sandbox_win64.process_mitigations.obj.rsp ...
3 years, 7 months ago (2016-02-03 20:20:02 UTC) #27
Nico
ETIMEOUT: https://codereview.chromium.org/1660103005/
3 years, 7 months ago (2016-02-03 20:40:02 UTC) #28
domenic
3 years, 7 months ago (2016-02-04 19:33:26 UTC) #29
Message was sent while issue was closed.
Build broken on Windows 10 due to this change:
https://code.google.com/p/chromium/issues/detail?id=584389

Powered by Google App Engine
This is Rietveld 408576698