Add UIPI support for sandbox alternate desktop

Add UIPI support for sandbox alternate desktop

Processes must initialize user32 at a lower integrity level to
enable UIPI. So, we have to drop the integrity label of the
alternate desktop to allow processes to attach to the alternate
desktop at reduced integrity levels.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279424

[jschuh, 2014-06-13 16:13:05]

PTAL - I've always been meaning to make the renderers launch at low integrity,
but hadn't gotten around to it until now.

[jschuh, 2014-06-13 18:04:05]

Sorry, sent out that email before I was entirely done. This version is tweaked a
bit, and has proper test coverage.

[rvargas (doing something else), 2014-06-13 19:46:11]

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/restrict...
File sandbox/win/src/restricted_token_utils.h (right):

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/restrict...
sandbox/win/src/restricted_token_utils.h:83: const wchar_t*
GetIntegrityLevelString(IntegrityLevel integrity_level);
nit: this should go at line 76

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:81: INTEGRITY_LEVEL_SYSTEM;
nit: Shouldn't we use _LEVEL_LAST here? It shouldn't matter but it seems like a
better default.

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:529: // process has an integrity label
that can access it. So, we lower the label
label -> level

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:533: alternate_desktop_integrity_label_ <
integrity_level_ &&
_label_ -> level

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:533: alternate_desktop_integrity_label_ <
integrity_level_ &&
... then this would be 
alternate_desktop_integrity_label_ == INTEGRITY_LEVEL_LAST

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:542: ::SetLastError(result);
nit: I don't think we promise a last error.

[jschuh, 2014-06-13 22:29:37]

ptal

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/restrict...
File sandbox/win/src/restricted_token_utils.h (right):

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/restrict...
sandbox/win/src/restricted_token_utils.h:83: const wchar_t*
GetIntegrityLevelString(IntegrityLevel integrity_level);
On 2014/06/13 19:46:11, rvargas wrote:
> nit: this should go at line 76 

Done.

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:81: INTEGRITY_LEVEL_SYSTEM;
On 2014/06/13 19:46:11, rvargas wrote:
> nit: Shouldn't we use _LEVEL_LAST here? It shouldn't matter but it seems like
a
> better default.

Done, but it makes the conditional below a bit more complicated.

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:529: // process has an integrity label
that can access it. So, we lower the label
On 2014/06/13 19:46:11, rvargas wrote:
> label -> level

Fine, but you have to deal with the ire of Bell and LaPadula.

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:533: alternate_desktop_integrity_label_ <
integrity_level_ &&
On 2014/06/13 19:46:11, rvargas wrote:
> ... then this would be 
> alternate_desktop_integrity_label_ == INTEGRITY_LEVEL_LAST

Done, but that's not quite how it works due to the integrity level ordering.

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:542: ::SetLastError(result);
On 2014/06/13 19:46:11, rvargas wrote:
> nit: I don't think we promise a last error.

Done.

[rvargas (doing something else), 2014-06-13 23:18:48]

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:529: // process has an integrity label
that can access it. So, we lower the label
On 2014/06/13 22:29:36, Justin Schuh wrote:
> On 2014/06/13 19:46:11, rvargas wrote:
> > label -> level
> 
> Fine, but you have to deal with the ire of Bell and LaPadula.

no me asusta el acertijo!

https://codereview.chromium.org/330853002/diff/80001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:533: alternate_desktop_integrity_label_ <
integrity_level_ &&
On 2014/06/13 22:29:36, Justin Schuh wrote:
> On 2014/06/13 19:46:11, rvargas wrote:
> > ... then this would be 
> > alternate_desktop_integrity_label_ == INTEGRITY_LEVEL_LAST
> 
> Done, but that's not quite how it works due to the integrity level ordering.

Yeah, I got that... the way I was reading this condition was:
if we are using another desktop and
we are setting an integrity level ('cause it's not the default) and
alternate_desktop_integrity_level_ has not been initialized (because it is a
static and we don't want to reinit over and over)
... and, BTW, if we are not elevating the integrity level of the desktop
then... set IL.

However, that last condition looks to me more like an assert than something to
keep up front in the condition. That's why I suggested moving the last one to
what I thought to primarily mean (thinking we could just DCHECK inside the if).

Why do you want to keep both checks for ad_il_ and not for il_?. It feels weird
to me to be checking the relative values of integrity levels, especially at this
point. If we want to do it, shouldn't we do that explicitly somewhere else,
preferably close to where the policy is set instead of process creation?

Now it looks like you want to allow moving the IL of the desktop with new policy
instances... if that's the case we should move away from statics.

https://codereview.chromium.org/330853002/diff/90001/sandbox/win/src/sandbox_...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/90001/sandbox/win/src/sandbox_...
sandbox/win/src/sandbox_policy_base.cc:544: }
tiny nit: this should not have {} but I see them also at line 525 :(

[jschuh, 2014-06-14 14:23:14]

This patch depends on IntegrityLevel reording in
https://codereview.chromium.org/330373002/

[jschuh, 2014-06-20 23:00:39]

ptal - removed integrity level reordering dependency.

[rvargas (doing something else), 2014-06-20 23:13:08]

LGTM

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:81: INTEGRITY_LEVEL_LAST;
At this point I would be OK if you want to set this to SYSTEM and remove one of
the conditions from the check. Up to you. (Sorry about that).

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:524: if (ERROR_SUCCESS != result) {
nit: Could you remove these {}

[jschuh, 2014-06-20 23:21:04]

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
File sandbox/win/src/sandbox_policy_base.cc (right):

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:81: INTEGRITY_LEVEL_LAST;
On 2014/06/20 23:13:08, rvargas wrote:
> At this point I would be OK if you want to set this to SYSTEM and remove one
of
> the conditions from the check. Up to you. (Sorry about that).

Done.

https://codereview.chromium.org/330853002/diff/170001/sandbox/win/src/sandbox...
sandbox/win/src/sandbox_policy_base.cc:524: if (ERROR_SUCCESS != result) {
On 2014/06/20 23:13:08, rvargas wrote:
> nit: Could you remove these {}

Done.

[jschuh, 2014-06-20 23:21:08]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-06-20 23:24:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jschuh@chromium.org/330853002/190001

[commit-bot: I haz the power, 2014-06-21 03:18:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-21 05:04:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-21 05:04:08]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[jschuh, 2014-06-21 05:37:37]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-06-21 05:39:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jschuh@chromium.org/330853002/190001

[jschuh, 2014-06-21 05:39:22]

The CQ bit was unchecked by jschuh@chromium.org

[jschuh, 2014-06-24 16:38:08]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-06-24 16:39:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jschuh@chromium.org/330853002/190001

[commit-bot: I haz the power, 2014-06-24 17:12:20]

Change committed as 279424