Re: [PATCH] libselinux: Eliminate use of security_compute_user()

[Stephen Smalley <sds@tycho.nsa.gov>]

On 5/16/19 11:07 AM, Petr Lautrbach wrote:
> 
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
>> On 5/9/19 4:42 AM, Petr Lautrbach wrote:
>>> get_ordered_context_list() code used to ask the kernel to compute the 
>>> complete
>>> set of reachable contexts using /sys/fs/selinux/user aka
>>> security_compute_user(). This set can be so huge so that it doesn't 
>>> fit into a
>>> kernel page and security_compute_user() fails. Even if it doesn't fail,
>>> get_ordered_context_list() throws away the vast majority of the returned
>>> contexts because they don't match anything in
>>> /etc/selinux/targeted/contexts/default_contexts or
>>> /etc/selinux/targeted/contexts/users/
>>>
>>> get_ordered_context_list() is rewritten to compute set of contexts 
>>> based on
>>> /etc/selinux/targeted/contexts/users/ and
>>> /etc/selinux/targeted/contexts/default_contexts files and to return 
>>> only valid
>>> contexts, using security_check_context(), from this set.
>>>
>>> Fixes: https://github.com/SELinuxProject/selinux/issues/28
>>>
>>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>>> ---
<snip>
>> I think the main potential stumbling block here is the MLS range 
>> component. The
>> kernel policy defines the default level and allowed range for the 
>> (SELinux)
>> user, and uses this information in the kernel function 
>> mls_setup_user_range(),
>> https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/mls.c#L402, 
>>
>> to determine the most suitable MLS range for the user session, based 
>> on both the
>> from-context and the user default and range from the kernel policy.  
>> Just using
>> the level from the from-context could fail if the user isn't 
>> authorized to
>> operate at that level, and even if the user is authorized to operate 
>> at that
>> level, it could introduce a change in the default behavior if the 
>> user's default
>> level differs. I think when we have discussed this in the past on the 
>> list, we
>> were going to either export the user's default level and range 
>> information from
>> the kernel via selinuxfs and replicate the mls_setup_user_ranges() 
>> logic in
>> userspace, or have it automatically extracted from the kernel policy 
>> during
>> policy build into a userspace configuration file that could be used 
>> directly by
>> userspace.  Or something like that.  This gets a bit tricky though in 
>> that the
>> logic involves comparing MLS levels, which is intrinsically 
>> policy-specific
>> logic, and thus if we wanted to truly replicate it in userspace, we'd 
>> probably
>> need to use libsepol.  Ugh. Maybe the kernel could just provide a simple
>> selinuxfs interface for computing the result of mls_setup_user_range() 
>> and
>> return that piece.
>>
>> That said, I don't know to what extent anyone is relying on this logic 
>> and to
>> what extent it is obsoleted by the use of the level/range from 
>> seusers.  It
>> looks like today we are replacing the level/range in the original 
>> from-context
>> with the one from seusers before calling this code, in which case the 
>> fromlevel
>> is in fact the one we ultimately want to use.  So perhaps this doesn't 
>> matter
>> and we can just go with your approach.
> 
> The problem is much complicated than I originally thought and this
> patch changes the behavior of get_ordered_context_list what is probably
> not acceptable.
> 
> I'll do more tests and think about it the light of new (for me)
> information.
> 
> Thanks all for reviews and inputs.

I would like to re-visit this patch again.  I did some looking at how 
get_ordered_context_list() and its variant interfaces are currently 
being used by callers, and at the internal logic of 
get_ordered_context_list() in userspace and mls_setup_user_ranges() in 
the kernel.  Since we are already substituting the range/level from 
seusers into the from-context before calling security_compute_user(), 
and since the only sensible configuration of seusers would be to use a 
range/level that falls within (or is identical to) the SELinux user's 
authorized range, I don't think your patch is likely to break anything. 
There are corner cases where it could yield a different result but I 
would be surprised if such corner cases are in real use and arguably 
they would be configuration errors.  Consequently, I think we should 
refresh your patch, address any comments made on it previously, and 
submit it for merging and try it out.  If we encounter any real world 
breakage from it, we can consider adding a new selinuxfs node that 
exports the kernel's mls_setup_user_ranges() logic and rework 
get_ordered_context_list() to use that to obtain the MLS portion of the 
context, but I don't think it is worth doing that without a real example 
where simply applying your patch breaks something.  Thoughts?