[Win10 sandbox mitigations] Four new Win10 mitigations added.

[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

[penny, 2016-01-25 19:12:11]

pennymac@chromium.org changed reviewers:
+ jschuh@chromium.org, wfh@chromium.org

[penny, 2016-01-25 19:12:12]

Hello reviewers,

As per the CL description, this adds 4 new Windows sandbox security mitigations.
 I've currently added all 4 to every process except browser.  I've also done so
without any finch (I don't think it's necessary for these mitigations) - I'm
happy to hear if you feel very strongly about this though.  Biggest changes are
in the tests.

jschuh/scottmg/grt: I need an owner to lgtm for the base/win version change, and
wfh is not an owner there.  I'm sure there will be differing opinions on how to
handle 10586 checks (with service-pack sized changes, but without being a
service pack), but I felt it was cleaner to define this and do the build check
once in the version singleton than everywhere in the code.  Let me know if you
feel strongly otherwise.

**Note, I've successfully run manual and auto tests locally on Win10 Th1 and
Th2.  However, I can't run against try bots, or land this CL until Visual Studio
2015 with SDK 10586 lands as default on trunk -> on track to happen within next
2 weeks. 

Owners:
- wfh/jschuh: 
    content/common/*
    sandbox/win/*
- jschuh/grt/scottmg: base/win/windows_version.*
- forshaw: I just wanted to let you check out the non-system font changes if you
want.

Specific questions:

1) I've left all of the new mitigations *in* "nacl_win64" builds.  I don't think
these new mitigations are affected by the non-standard memory segment management
in nacl_win64 (the way win32k lockdown was).  Please let me know if you think
otherwise.  (REF: sandbox_win.cc, StartSandboxedProcess(), line 702-722.)

2) I'm currently adding all 4 new mitigations at pre-startup, although 3 of them
do support post-startup as well.  Local/manual testing seems fine with this -
but let me know if you have a reason to push any to post-startup.

3) Is it worth adding any of the recent mitigations (>= Win8) to the browser
process (post-startup)?  (REF: content\app\sandbox_helper_win.cc,
InitializeSandboxInfo(), line 18-21.)

4) FilterPostStartupProcessMitigations is a bit of a strange little function
that "forces" certain post-startup mitigations to be turned on.  I'm not sure
what the threshold is for adding mitigations to this function... should I add
any >= win10 mitigations in there?  (REF: process_mitigations.cc,
FilterPostStartupProcessMitigations(), line 318-344.)


Many thanks for your time all.  Ping me if you have any questions.

[Will Harris, 2016-01-25 19:32:36]

some initial comments. can you also run git cl format? thanks.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations.h (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations.h:30: bool* no_child_processes);
add comment for what this argument does

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:211: else {
please run git cl format to format these. should be

} else {

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/tests/common/co...
File sandbox/win/tests/common/controller.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/tests/common/co...
sandbox/win/tests/common/controller.cc:138: if (!enable_create_process) {
Can this not be moved to InternalRunTest and a new method added on TestRunner to
disable CSRSS lockdown, rather than adding all these extra constructors?

[jschuh, 2016-01-25 23:53:51]

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#...
base/win/windows_version.h:33: VERSION_WIN10_10586,  // Version 1511, Threshold
2.
This could be nicer. How about:

  VERSION_WIN10_TH2,  // Threshold 2: Version 1511, Build 10586.

https://codereview.chromium.org/1626623003/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1626623003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:708: sandbox::MITIGATION_NONSYSTEM_FONT_DISABLE |
This will break the GDI path, which some people are enabling via a flag. In a
non-egligible number of cases this as a workaround for bugs in our font fallback
logic. Those are bugs we need to fix, so I don't think we need to block this
mitigation. However, it is a problem we need to be aware of.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:340: DWORD child_process_creation =
PROCESS_CREATION_CHILD_PROCESS_RESTRICTED;
This feels awkward. The job object already has flags to determine if child
processes are being blocked. So, why not just infer from that rather than plumb
it in and out of the process mitigations policy? If you want to avoid making
this function more complicated, you can just put the logic in an anonymous
helper function.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations.h (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations.h:27: void
ConvertProcessMitigationsToPolicy(MitigationFlags flags,
As I mentioned above, I wouldn't thread child process blocking through this
function, and instead would infer it from the job object.

[brucedawson, 2016-01-26 21:04:48]

Let me know if this gets approved and is blocked on VS 2015. If necessary we can
update VS 2013 to ship the 10586 SDK. The package for this has been created and
uploaded and is available in crrev.com/1616553002 if needed or wanted.

[penny, 2016-01-26 21:52:13]

Description was changed from

==========
[Win10 sandbox mitigations]  Four new Win10 mitigations added.

1. Disable non-system font loading (MITIGATION_NONSYSTEM_FONT_DISABLE).
2. Disable image loads from remote devices (MITIGATION_IMAGE_LOAD_NO_REMOTE).
3. Disable loading images that are labelled low integrity mandatory
   (MITIGATION_IMAGE_LOAD_NO_LOW_LABEL).
4. Disable child process creation
   (MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED).

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_10586 has been added to identify
Threshold 2/1511/10586.

BUG=504006
==========

to

==========
[Win10 sandbox mitigations]  Four new Win10 mitigations added.

1. Disable non-system font loading (MITIGATION_NONSYSTEM_FONT_DISABLE).
2. Disable image loads from remote devices (MITIGATION_IMAGE_LOAD_NO_REMOTE).
3. Disable loading images that are labelled low integrity mandatory
   (MITIGATION_IMAGE_LOAD_NO_LOW_LABEL).
4. Disable child process creation
   (MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED).

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
==========

[penny, 2016-01-26 22:37:10]

Thanks Bruce!  I'll keep in touch with you about this.

Thanks Will and Justin, changes are in.

W.R.T the child process restriction:  I know what you're saying Justin, that I
could tie this mitigation to the Job level.  I did consider that.  However, I
actually think that this should be a security mitigation independent from Job
level.  It seems to me that a product which uses the Chromium sandbox might want
to have a process with a Job level higher than JOB_LIMITED_USER (or no Job at
all), that can't spawn any children.  I feel like it's more powerful to offer
the ability to disable child process creation on its own.  Thoughts?

Side note: WinBase.h, it'll be interesting to see what they do for the next
Windows "threshold" when they add more mitigation support.  They've run out of
bits for defining more mitigation flags for
PROC_THREAD_ATTRIBUTE_MITIGATION_POLICY.  :)

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#...
base/win/windows_version.h:33: VERSION_WIN10_10586,  // Version 1511, Threshold
2.
On 2016/01/25 23:53:51, jschuh (very slow) wrote:
> This could be nicer. How about:
> 
>   VERSION_WIN10_TH2,  // Threshold 2: Version 1511, Build 10586.

Done.  Sounds fine to me!

https://codereview.chromium.org/1626623003/diff/1/content/common/sandbox_win.cc
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1626623003/diff/1/content/common/sandbox_win....
content/common/sandbox_win.cc:708: sandbox::MITIGATION_NONSYSTEM_FONT_DISABLE |
On 2016/01/25 23:53:51, jschuh (very slow) wrote:
> This will break the GDI path, which some people are enabling via a flag. In a
> non-egligible number of cases this as a workaround for bugs in our font
fallback
> logic. Those are bugs we need to fix, so I don't think we need to block this
> mitigation. However, it is a problem we need to be aware of.

Acknowledged.  I was not aware of this at all, thank you.  Is there an ETA on
the fixes - or can you throw me any links?  Would it help to make this a
post-startup mitigation (i.e. is the GDI path only used at startup before we
drop into low privileges, or could it happen at any time)?

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/broker_serv...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/broker_serv...
sandbox/win/src/broker_services.cc:340: DWORD child_process_creation =
PROCESS_CREATION_CHILD_PROCESS_RESTRICTED;
On 2016/01/25 23:53:51, jschuh (very slow) wrote:
> This feels awkward. The job object already has flags to determine if child
> processes are being blocked. So, why not just infer from that rather than
plumb
> it in and out of the process mitigations policy? If you want to avoid making
> this function more complicated, you can just put the logic in an anonymous
> helper function.

Acknowledged.  See my comments on this.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations.h (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations.h:27: void
ConvertProcessMitigationsToPolicy(MitigationFlags flags,
On 2016/01/25 23:53:51, jschuh (very slow) wrote:
> As I mentioned above, I wouldn't thread child process blocking through this
> function, and instead would infer it from the job object.

Acknowledged.  See my comments about this.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations.h:30: bool* no_child_processes);
On 2016/01/25 19:32:35, Will Harris wrote:
> add comment for what this argument does

Done.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/src/process_mit...
sandbox/win/src/process_mitigations_test.cc:211: else {
On 2016/01/25 19:32:35, Will Harris wrote:
> please run git cl format to format these. should be
> 
> } else {

Done.

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/tests/common/co...
File sandbox/win/tests/common/controller.cc (right):

https://codereview.chromium.org/1626623003/diff/1/sandbox/win/tests/common/co...
sandbox/win/tests/common/controller.cc:138: if (!enable_create_process) {
On 2016/01/25 19:32:36, Will Harris wrote:
> Can this not be moved to InternalRunTest and a new method added on TestRunner
to
> disable CSRSS lockdown, rather than adding all these extra constructors?

Done.  Excellent idea.  For some reason I didn't consider moving the existing
change to disable ALPCs out of init - certainly will next time.

[jschuh, 2016-01-26 23:42:03]

On 2016/01/26 22:37:10, penny wrote:
> Thanks Bruce!  I'll keep in touch with you about this.
> 
> Thanks Will and Justin, changes are in.
> 
> W.R.T the child process restriction:  I know what you're saying Justin, that I
> could tie this mitigation to the Job level.  I did consider that.  However, I
> actually think that this should be a security mitigation independent from Job
> level.  It seems to me that a product which uses the Chromium sandbox might
want
> to have a process with a Job level higher than JOB_LIMITED_USER (or no Job at
> all), that can't spawn any children.  I feel like it's more powerful to offer
> the ability to disable child process creation on its own.  Thoughts?

After a spirited in-person discussion everyone agreed on tying this to the job
level. Either that, or people just wanted me to shut up and go away. I really
can't tell the difference.

[penny, 2016-01-27 01:06:35]

Description was changed from

==========
[Win10 sandbox mitigations]  Four new Win10 mitigations added.

1. Disable non-system font loading (MITIGATION_NONSYSTEM_FONT_DISABLE).
2. Disable image loads from remote devices (MITIGATION_IMAGE_LOAD_NO_REMOTE).
3. Disable loading images that are labelled low integrity mandatory
   (MITIGATION_IMAGE_LOAD_NO_LOW_LABEL).
4. Disable child process creation
   (MITIGATION_CHILD_PROCESS_CREATION_RESTRICTED).

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
==========

to

==========
[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
==========

[penny, 2016-01-27 01:15:55]

Patchset #3 (id:40001) has been deleted

[penny, 2016-01-27 01:22:42]

Ok, I've tied the new create-child-process restriction to the JobLevel in
BrokerServicesBase::SpawnTarget().  I had to expose a GetJobLevel() function to
do so.

Also slightly tweaked the child creation tests.  They run on all versions of
Windows now and just change the job level.

Any feedback on the specific questions from my first set of comments?

[Will Harris, 2016-01-27 02:04:27]

looking good. A lot of the time there is a base:: API that will do what you want
without you having to do it, so a lot of my comments are around that.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/broker_...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/broker_...
sandbox/win/src/broker_services.cc:365: policy->GetJobLevel() <=
JOB_LIMITED_USER) {
policy_base

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

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:37: typedef BOOL(WINAPI*
RemoveFontMemResourceExFunction)(HANDLE resource_handle);
no real need for these here with c++11 syntactic magic pixie dust. when you want
to use them just declare a type in the scope you're going to use it (function
wide, or compilation unit wide) - using e.g.

typedef decltype(AddFontMemResourceEx)* AddFontMemResourceExType;

then reinterpret_cast into that type directly.  You could refactor the existing
code to do this as well if you wanted...

This means we're just using the SDK definitions.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:104: bool LoadFileToMemory(const
std::wstring& file_name, std::vector<char>& data) {
it seems base::File::Read() does a lot of what this function already does, not a
huge deal but sounds like it could be used instead here?

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:178: void
TestWin10ImageLoadLowLabel(bool is_success_test) {
this is a cool test.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:181:
EXPECT_TRUE(::GetWindowsDirectory(dir_buffer, MAX_PATH));
you could use base::PathService::Get(base::DIR_WINDOWS, &path) to get these
paths... then you'd have a base::FilePath you can append to easily using all the
FilePath functions. See also uses below.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:186: new_path = new_path +
L"\\temp\\lowIL_calc.exe";
can't use temp, as multiple tests might be running at the same time on the same
host...

can you use base::ScopedTempDir? This also handles the delete at the end.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:188:
EXPECT_TRUE(::CopyFileW(orig_path.c_str(), new_path.c_str(), false));
base::CopyFile and using base::FilePath instead of raw strings...?

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:200: if (::CreateProcessW(NULL,
cmd_line.get(), NULL, NULL, false, 0, NULL, NULL,
base::LaunchProcess() here might be easier.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:234: test += ((is_success_test) ? L"
success" : L" failure");
nit: for all these success | failure tests can't you just have the
SBOX_TESTS_COMMAND return a static return value depending on whether it
"succeeeded" or "failed" rather than passing an extra parameter

then you could just compare in the TEST i.e. expect sandbox::SBOX_TEST_SUCCEEDED
for the ones you expect to succeed and sandbox::SBOX_TEST_FAILED for the ones
you expect to fail?

Seems easier than delegating this logic (including comparing the command lines)
to the SBOX_TEST_COMMAND...?

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:270: if (::CreateProcessW(NULL,
cmd_line.get(), NULL, NULL, false, 0, NULL, NULL,
base::LaunchProcess maybe?

unless you want to launch suspended just to avoid popping calc :)

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:283:
/*****************************************************************************/
This comment style is not chromium, and also below.

remove all the *'s please

[penny, 2016-01-28 19:25:16]

Thanks much.  Latest fixes uploaded.


Also re-adding a few questions I had from the start (for anyone to answer):

1) I've left all of the new mitigations *in* "nacl_win64" builds.  I don't think
these new mitigations are affected by the non-standard memory segment management
in nacl_win64 (the way win32k lockdown was).  Please let me know if you think
otherwise.  (REF: sandbox_win.cc, StartSandboxedProcess(), line 715-721.)

2) I'm currently adding all 4 new mitigations at pre-startup, although 3 of them
do support post-startup as well.  Local/manual testing seems fine with this -
but let me know if you have a reason to push any to post-startup.  (jschuh@, how
about the non-system font disable for GDI reasons?)

3) Is it worth adding any of the recent mitigations (>= Win8) to the browser
process (post-startup)?  (REF: content\app\sandbox_helper_win.cc,
InitializeSandboxInfo(), line 18-21.)

4) FilterPostStartupProcessMitigations is a bit of a strange little function
that "forces" certain post-startup mitigations to be turned on.  I'm not sure
what the threshold is for adding mitigations to this function... should I add
any >= win10 mitigations in there?  (REF: process_mitigations.cc,
FilterPostStartupProcessMitigations(), line 312-338.)

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/broker_...
File sandbox/win/src/broker_services.cc (right):

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/broker_...
sandbox/win/src/broker_services.cc:365: policy->GetJobLevel() <=
JOB_LIMITED_USER) {
On 2016/01/27 02:04:26, Will Harris wrote:
> policy_base

Done.  Good one.  I was copying the call to ConvertProcessMitigationsToPolicy()
above.  I've changed both to policy_base (which is a downcast from TargetPolicy
to PolicyBase) so nothing references |policy| after the cast.

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

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:37: typedef BOOL(WINAPI*
RemoveFontMemResourceExFunction)(HANDLE resource_handle);
On 2016/01/27 02:04:27, Will Harris wrote:
> no real need for these here with c++11 syntactic magic pixie dust. when you
want
> to use them just declare a type in the scope you're going to use it (function
> wide, or compilation unit wide) - using e.g.
> 
> typedef decltype(AddFontMemResourceEx)* AddFontMemResourceExType;
> 
> then reinterpret_cast into that type directly.  You could refactor the
existing
> code to do this as well if you wanted...
> 
> This means we're just using the SDK definitions.

Done.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:104: bool LoadFileToMemory(const
std::wstring& file_name, std::vector<char>& data) {
On 2016/01/27 02:04:27, Will Harris wrote:
> it seems base::File::Read() does a lot of what this function already does, not
a
> huge deal but sounds like it could be used instead here?

Done.  I'm using base::File now.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:181:
EXPECT_TRUE(::GetWindowsDirectory(dir_buffer, MAX_PATH));
On 2016/01/27 02:04:27, Will Harris wrote:
> you could use base::PathService::Get(base::DIR_WINDOWS, &path) to get these
> paths... then you'd have a base::FilePath you can append to easily using all
the
> FilePath functions. See also uses below.

Done.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:186: new_path = new_path +
L"\\temp\\lowIL_calc.exe";
On 2016/01/27 02:04:27, Will Harris wrote:
> can't use temp, as multiple tests might be running at the same time on the
same
> host...
> 
> can you use base::ScopedTempDir? This also handles the delete at the end.

Nice.  ScopedTempDir is handy.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:188:
EXPECT_TRUE(::CopyFileW(orig_path.c_str(), new_path.c_str(), false));
On 2016/01/27 02:04:27, Will Harris wrote:
> base::CopyFile and using base::FilePath instead of raw strings...?

Done.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:200: if (::CreateProcessW(NULL,
cmd_line.get(), NULL, NULL, false, 0, NULL, NULL,
On 2016/01/27 02:04:27, Will Harris wrote:
> base::LaunchProcess() here might be easier.

Done.  Took me quite a while to figure out how to use strings with the
LaunchProcess API (FilePath vs CommandLine vs std::wstring) - but it's probably
worth it in the end.  Also, it seems weird to not be able to close the thread
handle via the API.  I can only call Close() to close the process handle.  It's
fine for the tests though.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:234: test += ((is_success_test) ? L"
success" : L" failure");
On 2016/01/27 02:04:26, Will Harris wrote:
> nit: for all these success | failure tests can't you just have the
> SBOX_TESTS_COMMAND return a static return value depending on whether it
> "succeeeded" or "failed" rather than passing an extra parameter
> 
> then you could just compare in the TEST i.e. expect
sandbox::SBOX_TEST_SUCCEEDED
> for the ones you expect to succeed and sandbox::SBOX_TEST_FAILED for the ones
> you expect to fail?
> 
> Seems easier than delegating this logic (including comparing the command
lines)
> to the SBOX_TEST_COMMAND...?

Done.

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:270: if (::CreateProcessW(NULL,
cmd_line.get(), NULL, NULL, false, 0, NULL, NULL,
On 2016/01/27 02:04:27, Will Harris wrote:
> base::LaunchProcess maybe?
> 
> unless you want to launch suspended just to avoid popping calc :)

Done.  Popping calc is the most thrilling part of my day!

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:283:
/*****************************************************************************/
On 2016/01/27 02:04:26, Will Harris wrote:
> This comment style is not chromium, and also below.
> 
> remove all the *'s please

Soooo, how does one create a visual horizontal separation in a file (to organize
chunks of related functions)?  With so many test functions building up in this
file that are all very similar, I was wasting a lot of time trying to just find
functions.  It's much easier now to find the functions related to testing a
specific mitigation.

I've changed the stars to dashes (and like that better).  I can't seem to find
any mention of related commenting in
http://www.chromium.org/developers/coding-style,
http://chromium-cpp.appspot.com/,
or
https://google.github.io/styleguide/cppguide.html#Comments.

Not wanting to split this into multiple files, I feel that these visual breaks
help to organize and navigate this file.  And it's only going to get bigger.

[jschuh, 2016-01-29 22:59:29]

Lgtm on base/win and my other comments. Get wfh@ to sign off on his feedback and
you're good to go.

[Will Harris, 2016-01-30 00:28:43]

almost there!

On 2016/01/28 19:25:16, penny wrote:
> 1) I've left all of the new mitigations *in* "nacl_win64" builds.  I don't
think
> these new mitigations are affected by the non-standard memory segment
management
> in nacl_win64 (the way win32k lockdown was).  Please let me know if you think
> otherwise.  (REF: sandbox_win.cc, StartSandboxedProcess(), line 715-721.)

I'm almost tempted to put the application of these mitigations in chromium in
another CL. This makes them far more easily reverted. justin, what do you think?

I don't think nacl_win64 should have any issues with these mitigations. The
reason why the win32k ones are wrapped in NACL_WIN64 deps are because of
linkage, the 64-bit nacl64.exe shipped with 32-bit Chrome to work on 64-bit OS
doesn't have access to the entire content API and the function to know if win32k
lockdown is enabled or not... long story. Aside, since win32k lockdown is
working fine on nacl on 64-bit Windows, we should probably just hard enable this
on NACL64 builds...

> 
> 2) I'm currently adding all 4 new mitigations at pre-startup, although 3 of
them
> do support post-startup as well.  Local/manual testing seems fine with this -
> but let me know if you have a reason to push any to post-startup.  (jschuh@,
how
> about the non-system font disable for GDI reasons?)

I think pre-startup should be fine.

> 
> 3) Is it worth adding any of the recent mitigations (>= Win8) to the browser
> process (post-startup)?  (REF: content\app\sandbox_helper_win.cc,
> InitializeSandboxInfo(), line 18-21.)

see email thread.

> 
> 4) FilterPostStartupProcessMitigations is a bit of a strange little function
> that "forces" certain post-startup mitigations to be turned on.  I'm not sure
> what the threshold is for adding mitigations to this function... should I add
> any >= win10 mitigations in there?  (REF: process_mitigations.cc,
> FilterPostStartupProcessMitigations(), line 312-338.)

FilterPostStartupProcessMitigations filters the mitigations to those that can
only be applied post startup (and thus have to be transferred to the target
while its suspended). Since you're applying all of yours before target process
launch using UpdateProcThreadAttribute I don't think you need to add any to this
function. You only would if there were any new ones that could only be applied
in-process.

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

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:283:
/*****************************************************************************/
On 2016/01/28 19:25:15, penny wrote:
> On 2016/01/27 02:04:26, Will Harris wrote:
> > This comment style is not chromium, and also below.
> > 
> > remove all the *'s please
> 
> Soooo, how does one create a visual horizontal separation in a file (to
organize
> chunks of related functions)?  With so many test functions building up in this
> file that are all very similar, I was wasting a lot of time trying to just
find
> functions.  It's much easier now to find the functions related to testing a
> specific mitigation.
> 
> I've changed the stars to dashes (and like that better).  I can't seem to find
> any mention of related commenting in
> http://www.chromium.org/developers/coding-style,
> http://chromium-cpp.appspot.com/,
> or
> https://google.github.io/styleguide/cppguide.html#Comments.
> 
> Not wanting to split this into multiple files, I feel that these visual breaks
> help to organize and navigate this file.  And it's only going to get bigger.
> 

okay - //-- seems to have precedent with other files in base (e.g. location.cc)
and is consistent with other // commands in this file, so sgtm. Please add one
extra - at the end though to be consistent though :)

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

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:168: if (setup_proc.IsValid()) {
avoid the indent by just doing ASSERT_TRUE, test will just die at that point,
which is fine.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:172: if (exit_code == 0)
again, mountain coding is usually avoided in chromium, we prefer to fail or
return early - perhaps just add some ASSERT_TRUE here?

also below, just chromium coding style to return early, return often.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:179: setup_proc.Close();
this should happen automatically at setup_proc destruction

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:234: } else {
style: no need for the else, as you return.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:456: base::File
file(base::FilePath(static_cast<std::wstring>(argv[0])),
shouldn't need a static_cast here, try:

base::File file(base::FilePath(argv[0]))

as FilePath::StringPieceType is already based on wide characters on Windows.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:466: int read = file.Read(0,
&font_data[0], len);
strange base::File::Read returns an int and not a int64? This code looks
strangely familiar.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:479: } else {
no need for else

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/tests/commo...
File sandbox/win/tests/common/controller.h (right):

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/tests/commo...
sandbox/win/tests/common/controller.h:108: void SetDisableCsrss(bool
disable_csrss) { disable_csrss_ = disable_csrss; }
add comment

[penny, 2016-02-01 20:43:22]

On 2016/01/30 00:28:43, Will Harris wrote:
> almost there!
> 
> On 2016/01/28 19:25:16, penny wrote:
> > 1) I've left all of the new mitigations *in* "nacl_win64" builds.  I don't
> think
> > these new mitigations are affected by the non-standard memory segment
> management
> > in nacl_win64 (the way win32k lockdown was).  Please let me know if you
think
> > otherwise.  (REF: sandbox_win.cc, StartSandboxedProcess(), line 715-721.)
> 
> I'm almost tempted to put the application of these mitigations in chromium in
> another CL. This makes them far more easily reverted. justin, what do you
think?
> 
* This makes sense to me.  I've commented out the enabling in sandbox_win.cc,
StartSandboxedProcess().  I'll follow up this CL with a tiny one that turns them
on.

> I don't think nacl_win64 should have any issues with these mitigations. The
> reason why the win32k ones are wrapped in NACL_WIN64 deps are because of
> linkage, the 64-bit nacl64.exe shipped with 32-bit Chrome to work on 64-bit OS
> doesn't have access to the entire content API and the function to know if
win32k
> lockdown is enabled or not... long story. Aside, since win32k lockdown is
> working fine on nacl on 64-bit Windows, we should probably just hard enable
this
> on NACL64 builds...
> 
* I'll follow up with another tiny CL that removes the define from around win32k
lockdown.

> > 
> > 2) I'm currently adding all 4 new mitigations at pre-startup, although 3 of
> them
> > do support post-startup as well.  Local/manual testing seems fine with this
-
> > but let me know if you have a reason to push any to post-startup.  (jschuh@,
> how
> > about the non-system font disable for GDI reasons?)
> 
> I think pre-startup should be fine.
> 
> > 
> > 3) Is it worth adding any of the recent mitigations (>= Win8) to the browser
> > process (post-startup)?  (REF: content\app\sandbox_helper_win.cc,
> > InitializeSandboxInfo(), line 18-21.)
> 
> see email thread.
> 
* So just for clarification, my question was independent from the email chain
about disabling legacy hooking in browser from chrome_elf.  This question is
about content\app\sandbox_helper_win.cc, InitializeSandboxInfo() - where we turn
on some browser mitigations after startup.

Given that the browser process is fairly high privileged, I'm just asking if you
think it's worth adding any of the newer security mitigations to our browser
process?  I.e.: any point in disabling image loads from remote devices or
mandatory low IL images here?  Non-system font loading?  We currently only turn
on:

sandbox::MITIGATION_DEP |
sandbox::MITIGATION_DEP_NO_ATL_THUNK |
sandbox::MITIGATION_HARDEN_TOKEN_IL_POLICY 

I suppose there is also now the option of enabling more mitigations pre-startup
from chrome_elf (in another CL), but DLL extensions was a bit of a special case
that needs to be done that early.
> > 
> > 4) FilterPostStartupProcessMitigations is a bit of a strange little function
> > that "forces" certain post-startup mitigations to be turned on.  I'm not
sure
> > what the threshold is for adding mitigations to this function... should I
add
> > any >= win10 mitigations in there?  (REF: process_mitigations.cc,
> > FilterPostStartupProcessMitigations(), line 312-338.)
> 
> FilterPostStartupProcessMitigations filters the mitigations to those that can
> only be applied post startup (and thus have to be transferred to the target
> while its suspended). Since you're applying all of yours before target process
> launch using UpdateProcThreadAttribute I don't think you need to add any to
this
> function. You only would if there were any new ones that could only be applied
> in-process.

* I agree that my new mitigations don't need to be added here, because we're
enabling them pre-startup.  My confusion with this function is due to the fact
that we set SOME delayed mitigations in sandbox_win.cc, StartSandboxedProcess():

  // Post-startup mitigations.
  mitigations = sandbox::MITIGATION_STRICT_HANDLE_CHECKS |
                sandbox::MITIGATION_DLL_SEARCH_ORDER;

  if (policy->SetDelayedProcessMitigations(mitigations) != sandbox::SBOX_ALL_OK)
    return base::Process();

...which makes a lot of sense.  But then there's this strange function that gets
called from within policy_base->AddTarget(), that seems to forcibly add
"pseudo-mitigations":

  // Add in delayed mitigations and pseudo-mitigations enforced at startup.
  g_shared_delayed_mitigations = delayed_mitigations_ |
      FilterPostStartupProcessMitigations(mitigations_);

You say it's adding mitigations that can "only" be done post-startup... but
MITIGATION_STRICT_HANDLE_CHECKS can only be done post-startup, and it's not
added inside this function.  Why don't we setup all delayed mitigations from
sandbox_win.cc, StartSandboxedProcess(), in ONE PLACE?  That's my real question.

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

https://codereview.chromium.org/1626623003/diff/60001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:283:
/*****************************************************************************/
On 2016/01/30 00:28:43, Will Harris wrote:
> On 2016/01/28 19:25:15, penny wrote:
> > On 2016/01/27 02:04:26, Will Harris wrote:
> > > This comment style is not chromium, and also below.
> > > 
> > > remove all the *'s please
> > 
> > Soooo, how does one create a visual horizontal separation in a file (to
> organize
> > chunks of related functions)?  With so many test functions building up in
this
> > file that are all very similar, I was wasting a lot of time trying to just
> find
> > functions.  It's much easier now to find the functions related to testing a
> > specific mitigation.
> > 
> > I've changed the stars to dashes (and like that better).  I can't seem to
find
> > any mention of related commenting in
> > http://www.chromium.org/developers/coding-style,
> > http://chromium-cpp.appspot.com/,
> > or
> > https://google.github.io/styleguide/cppguide.html#Comments.
> > 
> > Not wanting to split this into multiple files, I feel that these visual
breaks
> > help to organize and navigate this file.  And it's only going to get bigger.
> > 
> 
> okay - //-- seems to have precedent with other files in base (e.g.
location.cc)
> and is consistent with other // commands in this file, so sgtm. Please add one
> extra - at the end though to be consistent though :)

Done!  I appreciate you noticing that I'm missing a character.  I thought they
were 80 chars long, but I was actually interpreting the Visual Studio "col #"
count wrong.  My ocd is still shuddering at this close call.

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

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:168: if (setup_proc.IsValid()) {
On 2016/01/30 00:28:43, Will Harris wrote:
> avoid the indent by just doing ASSERT_TRUE, test will just die at that point,
> which is fine.

Done.  You just expanded my gTest universe.  I didn't know there were "ASSERT"
defines for tests.  They're not used anywhere else in these tests, so I assumed
we could only use the "EXPECT" defines (which don't stop execution).  :)

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:172: if (exit_code == 0)
On 2016/01/30 00:28:43, Will Harris wrote:
> again, mountain coding is usually avoided in chromium, we prefer to fail or
> return early - perhaps just add some ASSERT_TRUE here?
> 
> also below, just chromium coding style to return early, return often.

Done.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:179: setup_proc.Close();
On 2016/01/30 00:28:43, Will Harris wrote:
> this should happen automatically at setup_proc destruction

Done.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:234: } else {
On 2016/01/30 00:28:43, Will Harris wrote:
> style: no need for the else, as you return.

Done.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:456: base::File
file(base::FilePath(static_cast<std::wstring>(argv[0])),
On 2016/01/30 00:28:43, Will Harris wrote:
> shouldn't need a static_cast here, try:
> 
> base::File file(base::FilePath(argv[0]))
> 
> as FilePath::StringPieceType is already based on wide characters on Windows.

Done.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:466: int read = file.Read(0,
&font_data[0], len);
On 2016/01/30 00:28:43, Will Harris wrote:
> strange base::File::Read returns an int and not a int64? This code looks
> strangely familiar.

Acknowledged.  I thought the exact same thing when implementing.  :S

It's a bit awkward dealing with an 'int' returned, when the other APIs deal in
int64s.  Very familiar.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/src/process...
sandbox/win/src/process_mitigations_test.cc:479: } else {
On 2016/01/30 00:28:43, Will Harris wrote:
> no need for else

Done.

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/tests/commo...
File sandbox/win/tests/common/controller.h (right):

https://codereview.chromium.org/1626623003/diff/80001/sandbox/win/tests/commo...
sandbox/win/tests/common/controller.h:108: void SetDisableCsrss(bool
disable_csrss) { disable_csrss_ = disable_csrss; }
On 2016/01/30 00:28:43, Will Harris wrote:
> add comment

Done.  Thanks.

[Will Harris, 2016-02-01 21:33:57]

On 2016/02/01 20:43:22, penny wrote:
> * So just for clarification, my question was independent from the email chain
> about disabling legacy hooking in browser from chrome_elf.  This question is
> about content\app\sandbox_helper_win.cc, InitializeSandboxInfo() - where we
turn
> on some browser mitigations after startup.
> 
> Given that the browser process is fairly high privileged, I'm just asking if
you
> think it's worth adding any of the newer security mitigations to our browser
> process?  I.e.: any point in disabling image loads from remote devices or
> mandatory low IL images here?  Non-system font loading?  We currently only
turn
> on:
> 
> sandbox::MITIGATION_DEP |
> sandbox::MITIGATION_DEP_NO_ATL_THUNK |
> sandbox::MITIGATION_HARDEN_TOKEN_IL_POLICY 
> 
> I suppose there is also now the option of enabling more mitigations
pre-startup
> from chrome_elf (in another CL), but DLL extensions was a bit of a special
case
> that needs to be done that early.

yes I certainly think it's worth enabling more mitigations on browser. This can
probably be done in a follow on CL though. Let's keep this one to just adding
support for the new win10 ones.

> > > 
> > > 4) FilterPostStartupProcessMitigations is a bit of a strange little
function
> > > that "forces" certain post-startup mitigations to be turned on.  I'm not
> sure
> > > what the threshold is for adding mitigations to this function... should I
> add
> > > any >= win10 mitigations in there?  (REF: process_mitigations.cc,
> > > FilterPostStartupProcessMitigations(), line 312-338.)
> > 
> > FilterPostStartupProcessMitigations filters the mitigations to those that
can
> > only be applied post startup (and thus have to be transferred to the target
> > while its suspended). Since you're applying all of yours before target
process
> > launch using UpdateProcThreadAttribute I don't think you need to add any to
> this
> > function. You only would if there were any new ones that could only be
applied
> > in-process.
> 
> * I agree that my new mitigations don't need to be added here, because we're
> enabling them pre-startup.  My confusion with this function is due to the fact
> that we set SOME delayed mitigations in sandbox_win.cc,
StartSandboxedProcess():
> 
>   // Post-startup mitigations.
>   mitigations = sandbox::MITIGATION_STRICT_HANDLE_CHECKS |
>                 sandbox::MITIGATION_DLL_SEARCH_ORDER;
> 
>   if (policy->SetDelayedProcessMitigations(mitigations) !=
sandbox::SBOX_ALL_OK)
>     return base::Process();
> 
> ...which makes a lot of sense.  But then there's this strange function that
gets
> called from within policy_base->AddTarget(), that seems to forcibly add
> "pseudo-mitigations":
> 
>   // Add in delayed mitigations and pseudo-mitigations enforced at startup.
>   g_shared_delayed_mitigations = delayed_mitigations_ |
>       FilterPostStartupProcessMitigations(mitigations_);
> 
> You say it's adding mitigations that can "only" be done post-startup... but
> MITIGATION_STRICT_HANDLE_CHECKS can only be done post-startup, and it's not
> added inside this function.  Why don't we setup all delayed mitigations from
> sandbox_win.cc, StartSandboxedProcess(), in ONE PLACE?  That's my real
question.

That sounds like a bug then, either that or I'm misunderstanding what
FilterPostStartupProcessMitigations does.

If MITIGATION_STRICT_HANDLE_CHECKS can really only be applied post startup then
it needs to be passed to the target to engage, and right now it's being filtered
out. Are you sure? This seems pretty serious. I'm not sure I believe this. I
will look at this more shortly.

https://codereview.chromium.org/1626623003/diff/100001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1626623003/diff/100001/content/common/sandbox...
content/common/sandbox_win.cc:708: // Enabling in follow-up CL.
Don't comment them out. just don't add them please. There is no dead or disabled
code in chromium, it's just not committed.

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code == 0);
ASSERT_EQ so you get the print out of the exit_code if it's not 0.

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
sandbox/win/src/process_mitigations_test.cc:231:
//------------------------------------------------------------------------------
like++ the extra -

[penny, 2016-02-02 00:10:01]

Thanks Will!

1) Regarding content\app\sandbox_helper_win.cc, InitializeSandboxInfo():

I'm going to leave this for a follow-up CL to update post-startup
browser-process mitigations.

2) Regarding FilterPostStartupProcessMitigations():

After analyzing the code path, I'm happy we're not leaving any security holes. 
However,
this function looks like a clash between historical code iterations of new code.
 It's
on my list to clean up this function and sandbox_win.cc to ensure the right
mitigations
are set for pre-startup and post-startup, and that we do this in one place. 
It'll be a
follow-up CL.

https://codereview.chromium.org/1626623003/diff/100001/content/common/sandbox...
File content/common/sandbox_win.cc (right):

https://codereview.chromium.org/1626623003/diff/100001/content/common/sandbox...
content/common/sandbox_win.cc:708: // Enabling in follow-up CL.
On 2016/02/01 21:33:57, Will Harris wrote:
> Don't comment them out. just don't add them please. There is no dead or
disabled
> code in chromium, it's just not committed.

Done.

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code == 0);
On 2016/02/01 21:33:57, Will Harris wrote:
> ASSERT_EQ so you get the print out of the exit_code if it's not 0.

Done.  My gTest mind is blown once again.  Didn't know that ASSERT_EQ would
result in printout of the values if false.

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
sandbox/win/src/process_mitigations_test.cc:231:
//------------------------------------------------------------------------------
On 2016/02/01 21:33:57, Will Harris wrote:
> like++ the extra -

Acknowledged.

[Will Harris, 2016-02-02 01:19:18]

lgtm % one very tiny nit.

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
File sandbox/win/src/process_mitigations_test.cc (right):

https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code == 0);
On 2016/02/02 00:10:01, penny wrote:
> On 2016/02/01 21:33:57, Will Harris wrote:
> > ASSERT_EQ so you get the print out of the exit_code if it's not 0.
> 
> Done.  My gTest mind is blown once again.  Didn't know that ASSERT_EQ would
> result in printout of the values if false.

Sorry! One more nit: this should be

ASSERT_EQ(0, exit_code)

https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi...

[penny, 2016-02-02 18:05:04]

On 2016/02/02 01:19:18, Will Harris wrote:
> lgtm % one very tiny nit.
> 
>
https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
> File sandbox/win/src/process_mitigations_test.cc (right):
> 
>
https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
> sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code == 0);
> On 2016/02/02 00:10:01, penny wrote:
> > On 2016/02/01 21:33:57, Will Harris wrote:
> > > ASSERT_EQ so you get the print out of the exit_code if it's not 0.
> > 
> > Done.  My gTest mind is blown once again.  Didn't know that ASSERT_EQ would
> > result in printout of the values if false.
> 
> Sorry! One more nit: this should be
> 
> ASSERT_EQ(0, exit_code)
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi...

It's a good nit!  Thanks for the link as well.

Thanks to you (and jschuh) for the great reviews.  I'll be holding off on try
bots and landing until the VS 2015 (10586) lands.

[penny, 2016-02-03 00:03:51]

On 2016/02/02 18:05:04, penny wrote:
> On 2016/02/02 01:19:18, Will Harris wrote:
> > lgtm % one very tiny nit.
> > 
> >
>
https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
> > File sandbox/win/src/process_mitigations_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/1626623003/diff/100001/sandbox/win/src/proces...
> > sandbox/win/src/process_mitigations_test.cc:178: ASSERT_TRUE(exit_code ==
0);
> > On 2016/02/02 00:10:01, penny wrote:
> > > On 2016/02/01 21:33:57, Will Harris wrote:
> > > > ASSERT_EQ so you get the print out of the exit_code if it's not 0.
> > > 
> > > Done.  My gTest mind is blown once again.  Didn't know that ASSERT_EQ
would
> > > result in printout of the values if false.
> > 
> > Sorry! One more nit: this should be
> > 
> > ASSERT_EQ(0, exit_code)
> > 
> >
>
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi...
> 
> It's a good nit!  Thanks for the link as well.
> 
> Thanks to you (and jschuh) for the great reviews.  I'll be holding off on try
> bots and landing until the VS 2015 (10586) lands.

FYI: Bruce has landed the change for SDK 10586 when using VS 2013.  So no longer
blocked on that.
https://codereview.chromium.org/1616553002/

Thanks Bruce!

[commit-bot: I haz the power, 2016-02-03 17:36:21]

Description was changed from

==========
[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
==========

to

==========
[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://crrev.com/441d852dbcb7b9b31328393c7e31562b1e268399
Cr-Commit-Position: refs/heads/master@{#373265}
==========

[commit-bot: I haz the power, 2016-02-03 17:36:22]

Patchset 7 (id:??) landed as
https://crrev.com/441d852dbcb7b9b31328393c7e31562b1e268399
Cr-Commit-Position: refs/heads/master@{#373265}

[penny, 2016-02-03 17:37:08]

Description was changed from

==========
[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://crrev.com/441d852dbcb7b9b31328393c7e31562b1e268399
Cr-Commit-Position: refs/heads/master@{#373265}
==========

to

==========
[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/+/441d852dbcb7b9b31328393c7e31...
==========

[penny, 2016-02-03 17:37:10]

Committed patchset #7 (id:140001) manually as
441d852dbcb7b9b31328393c7e31562b1e268399 (presubmit successful).

[penny, 2016-02-03 18:47:52]

On 2016/02/03 17:37:10, penny wrote:
> Committed patchset #7 (id:140001) manually as
> 441d852dbcb7b9b31328393c7e31562b1e268399 (presubmit successful).

The follow-up CL that enables the three new mitigations:
https://codereview.chromium.org/1666753002/

[Nico, 2016-02-03 20:20:02]

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
/c ..\..\sandbox\win\src\process_mitigations.cc
/Foobj\sandbox\win\src\sandbox_win64.process_mitigations.obj
/Fdobj\sandbox\sandbox_win64.cc.pdb 
..\..\sandbox\win\src\process_mitigations.cc(182,54) :  error: suggest braces
around initialization of subobject [-Werror,-Wmissing-braces]
    PROCESS_MITIGATION_FONT_DISABLE_POLICY policy = {0};
                                                     ^
                                                     {}
..\..\sandbox\win\src\process_mitigations.cc(198,52) :  error: suggest braces
around initialization of subobject [-Werror,-Wmissing-braces]
    PROCESS_MITIGATION_IMAGE_LOAD_POLICY policy = {0};
                                                   ^

Can you fix, please? To initialize the whole struct to 0, just say `= {}`

[Nico, 2016-02-03 20:40:02]

ETIMEOUT: https://codereview.chromium.org/1660103005/

[domenic, 2016-02-04 19:33:26]

Build broken on Windows 10 due to this change:
https://code.google.com/p/chromium/issues/detail?id=584389