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

[William Roberts <bill.c.roberts@gmail.com>]

On Thu, May 9, 2019 at 1:43 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.

Whats the best way to test this?

>
> Fixes: https://github.com/SELinuxProject/selinux/issues/28
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libselinux/src/get_context_list.c | 185 ++++++++++--------------------
>  1 file changed, 60 insertions(+), 125 deletions(-)
>
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e4658..a36c6253 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -114,61 +114,24 @@ int get_default_context(const char *user,
>         return 0;
>  }
>
> -static int find_partialcon(char ** list,
> -                          unsigned int nreach, char *part)
> -{
> -       const char *conrole, *contype;
> -       char *partrole, *parttype, *ptr;
> -       context_t con;
> -       unsigned int i;
> -
> -       partrole = part;
> -       ptr = part;
> -       while (*ptr && !isspace(*ptr) && *ptr != ':')
> -               ptr++;
> -       if (*ptr != ':')
> -               return -1;
> -       *ptr++ = 0;
> -       parttype = ptr;
> -       while (*ptr && !isspace(*ptr) && *ptr != ':')
> -               ptr++;
> -       *ptr = 0;
> -
> -       for (i = 0; i < nreach; i++) {
> -               con = context_new(list[i]);
> -               if (!con)
> -                       return -1;
> -               conrole = context_role_get(con);
> -               contype = context_type_get(con);
> -               if (!conrole || !contype) {
> -                       context_free(con);
> -                       return -1;
> -               }
> -               if (!strcmp(conrole, partrole) && !strcmp(contype, parttype)) {
> -                       context_free(con);
> -                       return i;
> -               }
> -               context_free(con);
> -       }
> -
> -       return -1;
> -}
> -
> -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;
> -       ssize_t len;
> +       ssize_t len, ulen;
>         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,7 +143,8 @@ static int get_context_order(FILE * fp,
>                 return -1;
>         fromrole = context_role_get(con);
>         fromtype = context_type_get(con);
> -       if (!fromrole || !fromtype) {
> +       fromlevel = context_range_get(con);
> +       if (!fromrole || !fromtype || !fromlevel) {
>                 context_free(con);
>                 return -1;
>         }
> @@ -243,23 +207,52 @@ 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. */
> -                       start = end;
> -                       continue;
> +               /* Check whether a new context is valid */
> +               ulen = strlen(user) + strlen(start) + 1;

is their anything guaranteeing this doesn't overflow and result in
malloc(0) which is undefined?
also ulen is signed, why? All uses are where a size_t is valid, I
think it can change to a size_t.

> +               usercon_str = malloc(ulen);

spaces after tab, see:
https://github.com/SELinuxProject/selinux/pull/146/files#diff-55a16252233f48b841c06ffbe22bb66dR211

> +               if (!usercon_str) {
> +                       rc = -1;
> +                       goto out;
>                 }
>
> -               /* If a match is found and the entry is not already ordered
> -                  (e.g. due to prior match in prior config file), then set
> -                  the ordering for it. */
> -               i = rc;
> -               if (ordering[i] == nreach)
> -                       ordering[i] = (*nordered)++;
> +               /* set range from fromcon in the new usercon */
> +               snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
> +               if (!(usercon = context_new(usercon_str))) {

You can drop the too free(usercon_str) if you rework this logic:
usercon = context_new(usercon_str))
free(usercon_str);
if (!usercon) {
    // error
    continue;
}

> +                       fprintf(stderr,
> +                               "%s: can't create a context from %s\n",
> +                               __FUNCTION__, usercon_str);
> +                       free(usercon_str);
> +
> +                       continue;

Why is it OK to continue?

> +               }
> +               free(usercon_str);
> +               context_range_set(usercon, fromlevel);
> +               usercon_str = context_str(usercon);
> +
> +               if (security_check_context(usercon_str) == 0) {
> +                       if (*nreachable == 0) {
> +                               new_reachable = malloc(2 * sizeof(char *));
> +                               if (!new_reachable) {
> +                                       context_free(usercon);
> +                                       rc = -1;
> +                                       goto out;
> +                               }
> +                       } else {
> +                               new_reachable = realloc(*reachable, (*nreachable + 2) * sizeof(char *));
> +                               if (!new_reachable) {
> +                                       context_free(usercon);
> +                                       rc = -1;
> +                                       goto out;
> +                               }
> +                       }
> +                       new_reachable[*nreachable] = strdup(usercon_str);

oom error

> +                       new_reachable[*nreachable + 1] = 0;
> +                       *reachable = new_reachable;
> +                       *nreachable += 1;
> +               }
> +               context_free(usercon);
>                 start = end;
>         }
> -
>         rc = 0;
>
>        out:
> @@ -313,21 +306,6 @@ static int get_failsafe_context(const char *user, char ** newcon)
>         return 0;
>  }
>
> -struct context_order {
> -       char * con;
> -       unsigned int order;
> -};
> -
> -static int order_compare(const void *A, const void *B)
> -{
> -       const struct context_order *c1 = A, *c2 = B;
> -       if (c1->order < c2->order)
> -               return -1;
> -       else if (c1->order > c2->order)
> -               return 1;
> -       return strcmp(c1->con, c2->con);
> -}
> -
>  int get_ordered_context_list_with_level(const char *user,
>                                         const char *level,
>                                         char * fromcon,
> @@ -395,11 +373,8 @@ int get_ordered_context_list(const char *user,
>                              char *** list)
>  {
>         char **reachable = NULL;
> -       unsigned int *ordering = NULL;
> -       struct context_order *co = NULL;
> -       char **ptr;
>         int rc = 0;
> -       unsigned int nreach = 0, nordered = 0, freefrom = 0, i;
> +       unsigned nreachable = 0, freefrom = 0;
>         FILE *fp;
>         char *fname = NULL;
>         size_t fname_len;
> @@ -413,23 +388,6 @@ int get_ordered_context_list(const char *user,
>                 freefrom = 1;
>         }
>
> -       /* Determine the set of reachable contexts for the user. */
> -       rc = security_compute_user(fromcon, user, &reachable);
> -       if (rc < 0)
> -               goto failsafe;
> -       nreach = 0;
> -       for (ptr = reachable; *ptr; ptr++)
> -               nreach++;
> -       if (!nreach)
> -               goto failsafe;
> -
> -       /* Initialize ordering array. */
> -       ordering = malloc(nreach * sizeof(unsigned int));
> -       if (!ordering)
> -               goto failsafe;
> -       for (i = 0; i < nreach; i++)
> -               ordering[i] = nreach;
> -
>         /* Determine the ordering to apply from the optional per-user config
>            and from the global config. */
>         fname_len = strlen(user_contexts_path) + strlen(user) + 2;
> @@ -440,8 +398,8 @@ int get_ordered_context_list(const char *user,
>         fp = fopen(fname, "re");
>         if (fp) {
>                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
> -               rc = get_context_order(fp, fromcon, reachable, nreach, ordering,
> -                                      &nordered);
> +               rc = get_context_user(fp, fromcon, user, &reachable, &nreachable);
> +
>                 fclose(fp);
>                 if (rc < 0 && errno != ENOENT) {
>                         fprintf(stderr,
> @@ -454,8 +412,7 @@ int get_ordered_context_list(const char *user,
>         fp = fopen(selinux_default_context_path(), "re");
>         if (fp) {
>                 __fsetlocking(fp, FSETLOCKING_BYCALLER);
> -               rc = get_context_order(fp, fromcon, reachable, nreach, ordering,
> -                                      &nordered);
> +               rc = get_context_user(fp, fromcon, user, &reachable, &nreachable);
>                 fclose(fp);
>                 if (rc < 0 && errno != ENOENT) {
>                         fprintf(stderr,
> @@ -463,40 +420,18 @@ int get_ordered_context_list(const char *user,
>                                 __FUNCTION__, selinux_default_context_path());
>                         /* Fall through */
>                 }
> -               rc = 0;
> +               rc = nreachable;
>         }
>
> -       if (!nordered)
> +       if (!nreachable)
>                 goto failsafe;
>
> -       /* Apply the ordering. */
> -       co = malloc(nreach * sizeof(struct context_order));
> -       if (!co)
> -               goto failsafe;
> -       for (i = 0; i < nreach; i++) {
> -               co[i].con = reachable[i];
> -               co[i].order = ordering[i];
> -       }
> -       qsort(co, nreach, sizeof(struct context_order), order_compare);
> -       for (i = 0; i < nreach; i++)
> -               reachable[i] = co[i].con;
> -       free(co);
> -
> -       /* Only report the ordered entries to the caller. */
> -       if (nordered <= nreach) {
> -               for (i = nordered; i < nreach; i++)
> -                       free(reachable[i]);
> -               reachable[nordered] = NULL;
> -               rc = nordered;
> -       }
> -
>        out:
>         if (rc > 0)
>                 *list = reachable;
>         else
>                 freeconary(reachable);
>
> -       free(ordering);
>         if (freefrom)
>                 freecon(fromcon);
>
> --
> 2.21.0
>