Reorder IntegrityLevel enum to be less confusing

Reorder IntegrityLevel enum to be less confusing

The enum should be in ascending order like the levels themselves.

R=rvargas

[jschuh, 2014-06-16 14:37:13]

ptal

[rvargas (doing something else), 2014-06-16 19:16:58]

https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
File sandbox/win/src/security_level.h (right):

https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
Today we init variables to LAST, that has to change.

How do we know where do we have to fix places that do integrity_level_ <
new_integrity_level or similar?

[jschuh, 2014-06-17 16:45:47]

On 2014/06/16 19:16:58, rvargas wrote:
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> File sandbox/win/src/security_level.h (right):
> 
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
> Today we init variables to LAST, that has to change.

I verified that we're checking for INTEGRITY_LEVEL_LAST where needed, so it
still works correctly.


> How do we know where do we have to fix places that do integrity_level_ <
> new_integrity_level or similar?

It turns out we don't have any of those comparisons at this point. I thought we
had them in the past for tests (and may have) but it looks like my pending CL
will be the only one.

[rvargas (doing something else), 2014-06-17 17:49:12]

On 2014/06/17 16:45:47, Justin Schuh wrote:
> On 2014/06/16 19:16:58, rvargas wrote:
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > File sandbox/win/src/security_level.h (right):
> > 
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
> > Today we init variables to LAST, that has to change.
> 
> I verified that we're checking for INTEGRITY_LEVEL_LAST where needed, so it
> still works correctly.
> 

While technically we can use any value for "uninitialized" (in the middle of the
enum?) the point was that the default value that we use sits at the bottom of
the privileges, not at the top. I think it is valuable to preserve that
behavior.

> 
> > How do we know where do we have to fix places that do integrity_level_ <
> > new_integrity_level or similar?
> 
> It turns out we don't have any of those comparisons at this point. I thought
we
> had them in the past for tests (and may have) but it looks like my pending CL
> will be the only one.

[jschuh, 2014-06-17 18:12:41]

On 2014/06/17 17:49:12, rvargas wrote:
> On 2014/06/17 16:45:47, Justin Schuh wrote:
> > On 2014/06/16 19:16:58, rvargas wrote:
> > >
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > > File sandbox/win/src/security_level.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > > sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
> > > Today we init variables to LAST, that has to change.
> > 
> > I verified that we're checking for INTEGRITY_LEVEL_LAST where needed, so it
> > still works correctly.
> > 
> 
> While technically we can use any value for "uninitialized" (in the middle of
the
> enum?) the point was that the default value that we use sits at the bottom of
> the privileges, not at the top. I think it is valuable to preserve that
> behavior.

It's not, because the usage is probably a bit different than how you're
interpreting it. To explain, we don't ever want to have a default label, which
is why we explicitly initialize to a sentinel value that can never work as a
legitimate label. However, assuming it was even possible to have such a default
behavior, it's better to default to the highest integrity level anyway, because
any attempts to set it will fail since the OS will never allow you to set a
higher integrity level than that of the current token.


> > > How do we know where do we have to fix places that do integrity_level_ <
> > > new_integrity_level or similar?
> > 
> > It turns out we don't have any of those comparisons at this point. I thought
> we
> > had them in the past for tests (and may have) but it looks like my pending
CL
> > will be the only one.

[rvargas (doing something else), 2014-06-17 18:23:24]

On 2014/06/17 18:12:41, Justin Schuh wrote:
> On 2014/06/17 17:49:12, rvargas wrote:
> > On 2014/06/17 16:45:47, Justin Schuh wrote:
> > > On 2014/06/16 19:16:58, rvargas wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > > > File sandbox/win/src/security_level.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/330373002/diff/1/sandbox/win/src/security_lev...
> > > > sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
> > > > Today we init variables to LAST, that has to change.
> > > 
> > > I verified that we're checking for INTEGRITY_LEVEL_LAST where needed, so
it
> > > still works correctly.
> > > 
> > 
> > While technically we can use any value for "uninitialized" (in the middle of
> the
> > enum?) the point was that the default value that we use sits at the bottom
of
> > the privileges, not at the top. I think it is valuable to preserve that
> > behavior.
> 
> It's not, because the usage is probably a bit different than how you're
> interpreting it. To explain, we don't ever want to have a default label, which
> is why we explicitly initialize to a sentinel value that can never work as a
> legitimate label. However, assuming it was even possible to have such a
default
> behavior, it's better to default to the highest integrity level anyway,
because
> any attempts to set it will fail since the OS will never allow you to set a
> higher integrity level than that of the current token.

While relying on the OS to fail to do something is a valid point, another way to
look at what I'm saying is that this enum is also used to determine the
integrity level of a process, not just a resource (not saying that the OS will
accept the call in that case). Having a default at the bottom means "launch this
process with the minimum access that you can", if for some reason all goes wrong
and we miss that the value is not initialized. That still seems better than
"launch this process with 'all access[1]'" and waiting for the OS to fail.

[1] I know I'm being very loose with the terminology. I hope that doesn't trow
you off.

> 
> 
> > > > How do we know where do we have to fix places that do integrity_level_ <
> > > > new_integrity_level or similar?
> > > 
> > > It turns out we don't have any of those comparisons at this point. I
thought
> > we
> > > had them in the past for tests (and may have) but it looks like my pending
> CL
> > > will be the only one.

[jschuh, 2014-06-17 18:30:41]

On Tue, Jun 17, 2014 at 11:23 AM, <rvargas@chromium.org> wrote:

> On 2014/06/17 18:12:41, Justin Schuh wrote:
>
>> On 2014/06/17 17:49:12, rvargas wrote:
>> > On 2014/06/17 16:45:47, Justin Schuh wrote:
>> > > On 2014/06/16 19:16:58, rvargas wrote:
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/330373002/diff/1/sandbox/
> win/src/security_level.h
>
>> > > > File sandbox/win/src/security_level.h (right):
>> > > >
>> > > >
>> > >
>> >
>>
>
> https://codereview.chromium.org/330373002/diff/1/sandbox/
> win/src/security_level.h#newcode23
>
>> > > > sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
>> > > > Today we init variables to LAST, that has to change.
>> > >
>> > > I verified that we're checking for INTEGRITY_LEVEL_LAST where needed,
>> so
>>
> it
>
>> > > still works correctly.
>> > >
>> >
>> > While technically we can use any value for "uninitialized" (in the
>> middle of
>> the
>> > enum?) the point was that the default value that we use sits at the
>> bottom
>>
> of
>
>> > the privileges, not at the top. I think it is valuable to preserve that
>> > behavior.
>>
>
>  It's not, because the usage is probably a bit different than how you're
>> interpreting it. To explain, we don't ever want to have a default label,
>> which
>> is why we explicitly initialize to a sentinel value that can never work
>> as a
>> legitimate label. However, assuming it was even possible to have such a
>>
> default
>
>> behavior, it's better to default to the highest integrity level anyway,
>>
> because
>
>> any attempts to set it will fail since the OS will never allow you to set
>> a
>> higher integrity level than that of the current token.
>>
>
> While relying on the OS to fail to do something is a valid point, another
> way to
> look at what I'm saying is that this enum is also used to determine the
> integrity level of a process, not just a resource (not saying that the OS
> will
> accept the call in that case). Having a default at the bottom means
> "launch this
> process with the minimum access that you can", if for some reason all goes
> wrong
> and we miss that the value is not initialized. That still seems better than
> "launch this process with 'all access[1]'" and waiting for the OS to fail.
>
> [1] I know I'm being very loose with the terminology. I hope that doesn't
> trow
> you off.


I understand what you're saying, but I'm saying it's not relevant due to
how the label is used. However, were it possible to leak through, the
result with the proposed ordering would be preferable (i.e. an outright
failure versus an erroneously applied label). Because doing it the other
way means you might drop the integrity label on e.g. an object, which would
be very, very bad because it would introduce a vulnerability.


 > > > How do we know where do we have to fix places that do
>> integrity_level_ <
>> > > > new_integrity_level or similar?
>> > >
>> > > It turns out we don't have any of those comparisons at this point. I
>>
> thought
>
>> > we
>> > > had them in the past for tests (and may have) but it looks like my
>> pending
>> CL
>> > > will be the only one.
>>
>
>
>
> https://codereview.chromium.org/330373002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[rvargas (doing something else), 2014-06-17 19:28:02]

On 2014/06/17 18:30:41, Justin Schuh wrote:
> On Tue, Jun 17, 2014 at 11:23 AM, <mailto:rvargas@chromium.org> wrote:
> 
> > On 2014/06/17 18:12:41, Justin Schuh wrote:
> >
> >> On 2014/06/17 17:49:12, rvargas wrote:
> >> > On 2014/06/17 16:45:47, Justin Schuh wrote:
> >> > > On 2014/06/16 19:16:58, rvargas wrote:
> >> > > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/330373002/diff/1/sandbox/
> > win/src/security_level.h
> >
> >> > > > File sandbox/win/src/security_level.h (right):
> >> > > >
> >> > > >
> >> > >
> >> >
> >>
> >
> > https://codereview.chromium.org/330373002/diff/1/sandbox/
> > win/src/security_level.h#newcode23
> >
> >> > > > sandbox/win/src/security_level.h:23: INTEGRITY_LEVEL_LAST
> >> > > > Today we init variables to LAST, that has to change.
> >> > >
> >> > > I verified that we're checking for INTEGRITY_LEVEL_LAST where needed,
> >> so
> >>
> > it
> >
> >> > > still works correctly.
> >> > >
> >> >
> >> > While technically we can use any value for "uninitialized" (in the
> >> middle of
> >> the
> >> > enum?) the point was that the default value that we use sits at the
> >> bottom
> >>
> > of
> >
> >> > the privileges, not at the top. I think it is valuable to preserve that
> >> > behavior.
> >>
> >
> >  It's not, because the usage is probably a bit different than how you're
> >> interpreting it. To explain, we don't ever want to have a default label,
> >> which
> >> is why we explicitly initialize to a sentinel value that can never work
> >> as a
> >> legitimate label. However, assuming it was even possible to have such a
> >>
> > default
> >
> >> behavior, it's better to default to the highest integrity level anyway,
> >>
> > because
> >
> >> any attempts to set it will fail since the OS will never allow you to set
> >> a
> >> higher integrity level than that of the current token.
> >>
> >
> > While relying on the OS to fail to do something is a valid point, another
> > way to
> > look at what I'm saying is that this enum is also used to determine the
> > integrity level of a process, not just a resource (not saying that the OS
> > will
> > accept the call in that case). Having a default at the bottom means
> > "launch this
> > process with the minimum access that you can", if for some reason all goes
> > wrong
> > and we miss that the value is not initialized. That still seems better than
> > "launch this process with 'all access[1]'" and waiting for the OS to fail.
> >
> > [1] I know I'm being very loose with the terminology. I hope that doesn't
> > trow
> > you off.
> 
> 
> I understand what you're saying, but I'm saying it's not relevant due to
> how the label is used. However, were it possible to leak through, the
> result with the proposed ordering would be preferable (i.e. an outright
> failure versus an erroneously applied label). Because doing it the other
> way means you might drop the integrity label on e.g. an object, which would
> be very, very bad because it would introduce a vulnerability.

But then you are specifically talking about using the integrity level on a
resource, right? That is not the only use of the enum (are we doing that at all
currently?). See for instance StartRestrictedProcessInJob()


> 
> 
>  > > > How do we know where do we have to fix places that do
> >> integrity_level_ <
> >> > > > new_integrity_level or similar?
> >> > >
> >> > > It turns out we don't have any of those comparisons at this point. I
> >>
> > thought
> >
> >> > we
> >> > > had them in the past for tests (and may have) but it looks like my
> >> pending
> >> CL
> >> > > will be the only one.
> >>
> >
> >
> >
> > https://codereview.chromium.org/330373002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jschuh, 2014-06-17 20:52:34]

Lets try to reset this discussion. The IntegrityLevel values cannot be passed to
Windows system functions directly. We need to map the value to a SDDL string
using GetIntegrityLevelString(), which returns a NULL on INTEGRITY_LEVEL_LAST.
So, anything passing INTEGRITY_LEVEL_LAST is just going to crash and does not
represent a risk. Given that, are you comfortable with this CL?

[rvargas (doing something else), 2014-06-17 21:32:26]

On 2014/06/17 20:52:34, Justin Schuh wrote:
> Lets try to reset this discussion. The IntegrityLevel values cannot be passed
to
> Windows system functions directly. We need to map the value to a SDDL string
> using GetIntegrityLevelString(), which returns a NULL on INTEGRITY_LEVEL_LAST.
> So, anything passing INTEGRITY_LEVEL_LAST is just going to crash and does not
> represent a risk. Given that, are you comfortable with this CL?

I'm not.

I'm fine with reversing the order. I'm also fine with having
INTEGRITY_LEVEL_LAST at the end (well, it is the last one no matter what). I'm
not OK with changing the enum and nothing else. There are not that many uses of
INTEGRITY_LEVEL_LAST, but they should be revised. 

For example, the code on StartRestrictedProcessInJob() reads wrong to me
regardless of being a crash/vulnerability or not. To me it makes sense to say
"create this token so that this process runs with the lowest integrity level[1],
whatever that means, unless it is overridden by someone else". It doesn't make
sense to say "create this token so that this process runs with the highest
integrity level, whatever that means, unless it is overridden by someone else".
I don't like having code that relies by design on things being fixed later by
someone else.

Now if you were to argue that calling CreateRestrictedToken(...,
INTEGRITY_LEVEL_LAST,...) is just a way to say "I don't know what integrity
level to use so take this invalid value here that has to be fixed later", I
would say that then we need another constant below INTEGRITY_LEVEL_UNTRUSTED to
be used in that case.

[1] As you point out, this is not an OS call, this uses our definition and
ordering of IntegrityLevel and we are implicitly saying that there is an
ordering to that.