Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error

Committed: https://crrev.com/f9b843b964801440b50e74b072c70e7ecb0f1921
Cr-Commit-Position: refs/heads/master@{#382419}

[kylix_rd, 2016-03-17 22:10:52]

Description was changed from

==========
Cleanup/Remove Windows XP/Vista version checks

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

to

==========
Cleanup/Remove Windows XP/Vista version checks

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

[kylix_rd, 2016-03-17 22:10:52]

kylixrd@chromium.org changed reviewers:
+ jschuh@chromium.org

[kylix_rd, 2016-03-17 22:11:53]

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

[kylix_rd, 2016-03-17 22:13:53]

Description was changed from

==========
Cleanup/Remove Windows XP/Vista version checks

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

to

==========
Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

[kylix_rd, 2016-03-17 22:52:10]

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/win_uti...
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/win_uti...
sandbox/win/src/win_utils.cc:204: }*/
Ignore this change. Commented out to get Chrome to run from a folder behind a
reparse point.

[Will Harris, 2016-03-17 23:05:59]

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/integri...
File sandbox/win/src/integrity_level_test.cc (left):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/integri...
sandbox/win/src/integrity_level_test.cc:48: if (base::win::GetVersion() !=
base::win::VERSION_VISTA)
hmm were these tests really disabled on win7/8/10?

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
File sandbox/win/src/interception.cc (left):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
sandbox/win/src/interception.cc:474: if (!WowHelper.WaitForNtdll())
this appears to be the only place this is used, so can we delete wow64.cc and
wow64.h completely?

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/policy_...
File sandbox/win/src/policy_broker.cc (right):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/policy_...
sandbox/win/src/policy_broker.cc:113: // Bug 27218: We don't have dispatch for
some x64 syscalls.
this bug appears to be fixed, can this comment be updated?

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/process...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/process...
sandbox/win/src/process_mitigations.cc:83: return false;
I think this needs more return false for more failure cases

[kylix_rd, 2016-03-17 23:40:11]

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/integri...
File sandbox/win/src/integrity_level_test.cc (left):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/integri...
sandbox/win/src/integrity_level_test.cc:48: if (base::win::GetVersion() !=
base::win::VERSION_VISTA)
On 2016/03/17 23:05:58, Will Harris wrote:
> hmm were these tests really disabled on win7/8/10?

That is certainly what it looked like. That's why I enabled them.

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
File sandbox/win/src/interception.cc (left):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
sandbox/win/src/interception.cc:474: if (!WowHelper.WaitForNtdll())
On 2016/03/17 23:05:58, Will Harris wrote:
> this appears to be the only place this is used, so can we delete wow64.cc and
> wow64.h completely?

I will certainly look into that.

[kylix_rd, 2016-03-17 23:49:32]

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
sandbox/win/src/win_utils.cc:204: }*/
According to this:
https://bugs.chromium.org/p/project-zero/issues/detail?id=486&redir=1

The issue behind why this code is here has been fixed. Is there anyway to check
whether the currently running kernel is properly patched? This code only needs
to run if the unpatched/fixed kernel is present.

One idea was to have the sandboxed process actually try the exploit early in the
startup and fail if it is successful.. of course that places the exploit code
into the product.. probably not a good thing.

Don't know if there are ways to check the kernel patch level... that may also be
by design so that exploits can't easily tune their code at runtime.

[Will Harris, 2016-03-18 05:17:04]

+forshaw for the reparse check. I think we should leave this in...

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
sandbox/win/src/win_utils.cc:204: }*/
On 2016/03/17 23:49:31, kylix_rd wrote:
> According to this:
> https://bugs.chromium.org/p/project-zero/issues/detail?id=486&redir=1
> 
> The issue behind why this code is here has been fixed. Is there anyway to
check
> whether the currently running kernel is properly patched? This code only needs
> to run if the unpatched/fixed kernel is present.
> 
> One idea was to have the sandboxed process actually try the exploit early in
the
> startup and fail if it is successful.. of course that places the exploit code
> into the product.. probably not a good thing.
> 
> Don't know if there are ways to check the kernel patch level... that may also
be
> by design so that exploits can't easily tune their code at runtime.

I think we should leave this check in.

[jschuh, 2016-03-18 07:03:23]

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
sandbox/win/src/win_utils.cc:204: }*/
kylix_rd  - Sorry, I should have paid closer attention to your question earlier.
This code has nothing to do with forshaw's PZ bug. Rather, this supports a very
early sandbox design decision. That is, sandbox policies are path based, and if
you allow reparse points then filesystem policies get much harder to reason
about. So, the sandbox explicitly refuses to apply policies to paths with
reparse points.

Normally that approach isn't a problem, because we make very little use of
filesystem policies. However, if you look at the uses of AddRule in
sandbox_win.cc you'll see that unofficial and debug builds add a collection of
policies to enable logging and debugging in custom builds. So, that's what
you're running into.

In the future it would probably be easier to carry local changes in a local
branch that you base your CL off of. That way they won't be included in the CL
you post for review.

[kylix_rd, 2016-03-18 14:08:54]

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
File sandbox/win/src/win_utils.cc (right):

https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
sandbox/win/src/win_utils.cc:204: }*/
On 2016/03/18 07:03:23, jschuh (very slow) wrote:
> kylix_rd  - Sorry, I should have paid closer attention to your question
earlier.
> This code has nothing to do with forshaw's PZ bug. Rather, this supports a
very
> early sandbox design decision. That is, sandbox policies are path based, and
if
> you allow reparse points then filesystem policies get much harder to reason
> about. So, the sandbox explicitly refuses to apply policies to paths with
> reparse points.

Ok, that makes sense.

Something to think about; What about systems where there are
policies/configurations where "Program Files" are redirected through reparse
points or other such tricks? This may become more common as people use SSDs for
their boot drive, but then junction/symlink to their spindled drives for
programs and larger storage. This is how I've configured my system at home. I've
also used junctions to "graft in" an SSD into a folder off "C:" for development.
OS on spindle, development on SSD.

I only bring this up as something that may need to be considered in the future.
I'll also concede that this may become moot as SSDs get larger and prices
continue to drop.

[jschuh, 2016-03-18 14:59:02]

On 2016/03/18 14:08:54, kylix_rd wrote:
>
https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
> File sandbox/win/src/win_utils.cc (right):
> 
>
https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
> sandbox/win/src/win_utils.cc:204: }*/
> On 2016/03/18 07:03:23, jschuh (very slow) wrote:
> > kylix_rd  - Sorry, I should have paid closer attention to your question
> earlier.
> > This code has nothing to do with forshaw's PZ bug. Rather, this supports a
> very
> > early sandbox design decision. That is, sandbox policies are path based, and
> if
> > you allow reparse points then filesystem policies get much harder to reason
> > about. So, the sandbox explicitly refuses to apply policies to paths with
> > reparse points.
> 
> Ok, that makes sense.
> 
> Something to think about; What about systems where there are
> policies/configurations where "Program Files" are redirected through reparse
> points or other such tricks? This may become more common as people use SSDs
for
> their boot drive, but then junction/symlink to their spindled drives for
> programs and larger storage. This is how I've configured my system at home.
I've
> also used junctions to "graft in" an SSD into a folder off "C:" for
development.
> OS on spindle, development on SSD.
> 
> I only bring this up as something that may need to be considered in the
future.
> I'll also concede that this may become moot as SSDs get larger and prices
> continue to drop.

What you're describing isn't actually a problem in official builds because, as I
mentioned above, we use filesystem policies very rarely (look for AddRule calls
in sandbox_win.cc). The only reason you're hitting this in your Chromium setup
is because we have to add a number of policy exceptions for things like PDBs and
logging in developer builds.

[kylix_rd, 2016-03-18 16:16:10]

On 2016/03/18 14:59:02, jschuh (very slow) wrote:
> On 2016/03/18 14:08:54, kylix_rd wrote:
> >
>
https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
> > File sandbox/win/src/win_utils.cc (right):
> > 
> >
>
https://codereview.chromium.org/1814863004/diff/40001/sandbox/win/src/win_uti...
> > sandbox/win/src/win_utils.cc:204: }*/
> > On 2016/03/18 07:03:23, jschuh (very slow) wrote:
> > > kylix_rd  - Sorry, I should have paid closer attention to your question
> > earlier.
> > > This code has nothing to do with forshaw's PZ bug. Rather, this supports a
> > very
> > > early sandbox design decision. That is, sandbox policies are path based,
and
> > if
> > > you allow reparse points then filesystem policies get much harder to
reason
> > > about. So, the sandbox explicitly refuses to apply policies to paths with
> > > reparse points.
> > 
> > Ok, that makes sense.
> > 
> > Something to think about; What about systems where there are
> > policies/configurations where "Program Files" are redirected through reparse
> > points or other such tricks? This may become more common as people use SSDs
> for
> > their boot drive, but then junction/symlink to their spindled drives for
> > programs and larger storage. This is how I've configured my system at home.
> I've
> > also used junctions to "graft in" an SSD into a folder off "C:" for
> development.
> > OS on spindle, development on SSD.
> > 
> > I only bring this up as something that may need to be considered in the
> future.
> > I'll also concede that this may become moot as SSDs get larger and prices
> > continue to drop.
> 
> What you're describing isn't actually a problem in official builds because, as
I
> mentioned above, we use filesystem policies very rarely (look for AddRule
calls
> in sandbox_win.cc). The only reason you're hitting this in your Chromium setup
> is because we have to add a number of policy exceptions for things like PDBs
and
> logging in developer builds.

Ok, now I see what you're referring to. I've still a lot to learn about the code
base and its idiosyncrasies.

I've worked around the issue here by running chrome.exe from the original path
instead of through the junction.

[kylix_rd, 2016-03-21 19:09:09]

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
File sandbox/win/src/interception.cc (left):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/interce...
sandbox/win/src/interception.cc:474: if (!WowHelper.WaitForNtdll())
On 2016/03/17 23:05:58, Will Harris wrote:
> this appears to be the only place this is used, so can we delete wow64.cc and
> wow64.h completely?

Done.

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/policy_...
File sandbox/win/src/policy_broker.cc (right):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/policy_...
sandbox/win/src/policy_broker.cc:113: // Bug 27218: We don't have dispatch for
some x64 syscalls.
On 2016/03/17 23:05:58, Will Harris wrote:
> this bug appears to be fixed, can this comment be updated?

Done.

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/process...
File sandbox/win/src/process_mitigations.cc (right):

https://codereview.chromium.org/1814863004/diff/20001/sandbox/win/src/process...
sandbox/win/src/process_mitigations.cc:83: return false;
On 2016/03/17 23:05:59, Will Harris wrote:
> I think this needs more return false for more failure cases

Done.

[Will Harris, 2016-03-21 19:12:41]

lgtm

[jschuh, 2016-03-21 21:29:11]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2016-03-21 21:29:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814863004/70001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814863004/70001

[commit-bot: I haz the power, 2016-03-21 22:28:52]

Description was changed from

==========
Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

to

==========
Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

[commit-bot: I haz the power, 2016-03-21 22:28:54]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-03-21 22:30:45]

Description was changed from

==========
Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error
==========

to

==========
Cleanup/Remove Windows XP/Vista version checks from Windows sandbox code

903c28eb7d6d9cc14b0c67066e849608b6e5ada1
Initial sandbox VP/Vista version check sanitization

2f6c387f12b9570c59906356ec4d45437411542b
Fixed build error

Committed: https://crrev.com/f9b843b964801440b50e74b072c70e7ecb0f1921
Cr-Commit-Position: refs/heads/master@{#382419}
==========

[commit-bot: I haz the power, 2016-03-21 22:30:47]

Patchset 5 (id:??) landed as
https://crrev.com/f9b843b964801440b50e74b072c70e7ecb0f1921
Cr-Commit-Position: refs/heads/master@{#382419}