Re: [PATCH] libselinux: fix string conversion of unknown perms

[Mike Palmiotto <mike.palmiotto@crunchydata.com>]

On Mon, Sep 16, 2019 at 4:01 PM Stephen Smalley <sds@tycho.nsa.gov> wrote:
>
> On 9/10/19 3:53 PM, Mike Palmiotto wrote:
> > Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown
> > classes/permissions, but missed the case where an unknown permission is loaded
> > and then subsequently logged, either via denial or auditallow. If a permission
> > set has some valid values mixed with unknown values, say `{ read write foo }`,
> > a check on `{ read write foo }` would fail to log the entire set.
> >
> > To fix this, skip over the bad permissions/classes when expanding them to
> > strings. The unknowns should be logged during `selinux_set_mapping`, so
> > there is no need for further logging of the actual unknown permissions.
> >
> > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com>
> > ---
> >   libselinux/src/stringrep.c | 28 ++++++++++++----------------
> >   1 file changed, 12 insertions(+), 16 deletions(-)
> >
> > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c
> > index ad29f76d..85579422 100644
> > --- a/libselinux/src/stringrep.c
> > +++ b/libselinux/src/stringrep.c
> > @@ -276,19 +276,15 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       char *ptr;
> >
> >       /* first pass computes the required length */
> > -     while (tmp) {
> > +     for (i = 0; tmp; tmp >>= 1, i++) {
>
> Remove the redundant initialization in the declaration now that you are
> doing it here (which is better, I agree).
>
> >               if (tmp & 1) {
> >                       str = security_av_perm_to_string(tclass, av & (1<<i));
> > -                     if (str)
> > -                             len += strlen(str) + 1;
> > -                     else {
> > -                             rc = -1;
> > -                             errno = EINVAL;
> > -                             goto out;
> > +                     if (!str) {
> > +                             continue;
> >                       }
>
> No need to bracket it when it is a single statement.
>
> > +
> > +                     len += strlen(str) + 1;
>
> Might be clearer as:
>         if (str)
>                 len += strlen(str) + 1;
> And just let it fall through to the end of the loop otherwise - no need
> for explicit continue here.
>
> >               }
> > -             tmp >>= 1;
> > -             i++;
> >       }
> >
> >       *res = malloc(len);
> > @@ -298,7 +294,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       }
> >
> >       /* second pass constructs the string */
> > -     i = 0;
> >       tmp = av;
> >       ptr = *res;
> >
> > @@ -308,12 +303,13 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res)
> >       }
> >
> >       ptr += sprintf(ptr, "{ ");
> > -     while (tmp) {
> > -             if (tmp & 1)
> > -                     ptr += sprintf(ptr, "%s ", security_av_perm_to_string(
> > -                                            tclass, av & (1<<i)));
> > -             tmp >>= 1;
> > -             i++;
> > +     for (i = 0; tmp; tmp >>= 1, i++) {
> > +             if (tmp & 1) {
> > +                     str = security_av_perm_to_string(tclass, av & (1<<i));
> > +                     if (str) {
> > +                             ptr += sprintf(ptr, "%s ", str);
> > +                     }
>
> No need for { } around a single statement.
>
> > +             }
> >       }
> >       sprintf(ptr, "}");
> >   out:
> >
>

Thanks for the review. Fixed all of the above in v2.

-- 
Mike Palmiotto
https://crunchydata.com