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

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

On 2/10/20 10:00 AM, Ondrej Mosnacek wrote:
> I haven't looked at the whole patch properly yet, but noting what I
> found so far (see below)...
> 
> On Sat, Feb 8, 2020 at 8:36 AM Petr Lautrbach <plautrba@redhat.com> 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>
>> ---
>>
>> v3 changes:
>>
>> - check return values of context_range_set() and context_str()
>> - do not add duplicate entries to reachable contexts
>>
>>
>> libselinux/src/get_context_list.c | 219 ++++++++++++++----------------
>>   1 file changed, 103 insertions(+), 116 deletions(-)
>>
>> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
>> index 689e46589f30..cc39f8c6a96c 100644
>> --- a/libselinux/src/get_context_list.c
>> +++ b/libselinux/src/get_context_list.c
> 
> <snip>
> 
>> -static int get_context_order(FILE * fp,
>> +static int get_context_user(FILE * fp,
>>                               char * fromcon,
>> -                            char ** reachable,
>> -                            unsigned int nreach,
>> -                            unsigned int *ordering, unsigned int *nordered)
>> +                            const char * user,
>> +                            char ***reachable,
>> +                            unsigned int *nreachable)
>>   {
>>          char *start, *end = NULL;
>>          char *line = NULL;
>> -       size_t line_len = 0;
>> +       size_t line_len = 0, usercon_len;
>> +       size_t user_len = strlen(user);
>>          ssize_t len;
>>          int found = 0;
>> -       const char *fromrole, *fromtype;
>> +       const char *fromrole, *fromtype, *fromlevel;
>>          char *linerole, *linetype;
>> -       unsigned int i;
>> +       char **new_reachable = NULL;
>> +       char *usercon_str;
>>          context_t con;
>> +       context_t usercon;
>> +
>>          int rc;
>>
>>          errno = -EINVAL;
>> @@ -180,6 +158,7 @@ static int get_context_order(FILE * fp,
>>                  return -1;
>>          fromrole = context_role_get(con);
>>          fromtype = context_type_get(con);
>> +       fromlevel = context_range_get(con);
>>          if (!fromrole || !fromtype) {
>>                  context_free(con);
>>                  return -1;
>> @@ -243,23 +222,89 @@ static int get_context_order(FILE * fp,
>>                  if (*end)
>>                          *end++ = 0;
>>
>> -               /* Check for a match in the reachable list. */
>> -               rc = find_partialcon(reachable, nreach, start);
>> -               if (rc < 0) {
>> -                       /* No match, skip it. */
>> +               /* Check whether a new context is valid */
>> +               if (SIZE_MAX - user_len < strlen(start) + 1) {
> 
> You need to also account for either the null character or the colon
> (not sure which one you missed).
> 
>> +                       fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
>> +                       errno = EINVAL;
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +               usercon_len = user_len + strlen(start) + 1;
>> +               usercon_str = malloc(usercon_len);
> 
> Again, you are not accounting for one of '\0' or ':' here.
> 
>> +               if (!usercon_str) {
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +
>> +               /* set range from fromcon in the new usercon */
>> +               snprintf(usercon_str, usercon_len - 1, "%s:%s", user, start);
> 
> The second argument should be just usercon_len (assuming you fix the
> above) - see snprintf(3):
> 
> "The functions snprintf() and vsnprintf() write at most size bytes
> (including the terminating null byte ('\0')) to str."
> 
> So this should always result in a string with 2 last characters
> truncated (followed by a null character and an unused byte). Or am I
> missing something? Did you get correct strings when you tested this?

It is truncating the "s0" level from start but then we are explicitly 
overriding the level via context_range_set(), so the bug gets masked.