diff mbox series

selinux: refactor mls_context_to_sid() and make it stricter

Message ID 20180806211932.198488-1-jannh@google.com (mailing list archive)
State Accepted
Headers show
Series selinux: refactor mls_context_to_sid() and make it stricter | expand

Commit Message

Jann Horn via Selinux Aug. 6, 2018, 9:19 p.m. UTC
The intended behavior change for this patch is to reject any MLS strings
that contain (trailing) garbage if p->mls_enabled is true.

As suggested by Paul Moore, change mls_context_to_sid() so that the two
parts of the range are extracted before the rest of the parsing. Because
now we don't have to scan for two different separators simultaneously
everywhere, we can actually switch to strchr() everywhere instead of the
open-coded loops that scan for two separators at once.

mls_context_to_sid() used to signal how much of the input string was parsed
by updating `*scontext`. However, there is actually no case in which
mls_context_to_sid() only parses a subset of the input and still returns
a success (other than the buggy case with a second '-' in which it
incorrectly claims to have consumed the entire string). Turn `scontext`
into a simple pointer argument and stop redundantly checking whether the
entire input was consumed in string_to_context_struct(). This also lets us
remove the `scontext_len` argument from `string_to_context_struct()`.

Signed-off-by: Jann Horn <jannh@google.com>
---
Refactored version of
"[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
Paul's comments. WDYT?
I've thrown some inputs at it, and it seems to work.

 security/selinux/ss/mls.c      | 178 ++++++++++++++-------------------
 security/selinux/ss/mls.h      |   2 +-
 security/selinux/ss/services.c |  12 +--
 3 files changed, 82 insertions(+), 110 deletions(-)

Comments

Paul Moore Aug. 9, 2018, 1:56 a.m. UTC | #1
On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:
>
> The intended behavior change for this patch is to reject any MLS strings
> that contain (trailing) garbage if p->mls_enabled is true.
>
> As suggested by Paul Moore, change mls_context_to_sid() so that the two
> parts of the range are extracted before the rest of the parsing. Because
> now we don't have to scan for two different separators simultaneously
> everywhere, we can actually switch to strchr() everywhere instead of the
> open-coded loops that scan for two separators at once.
>
> mls_context_to_sid() used to signal how much of the input string was parsed
> by updating `*scontext`. However, there is actually no case in which
> mls_context_to_sid() only parses a subset of the input and still returns
> a success (other than the buggy case with a second '-' in which it
> incorrectly claims to have consumed the entire string). Turn `scontext`
> into a simple pointer argument and stop redundantly checking whether the
> entire input was consumed in string_to_context_struct(). This also lets us
> remove the `scontext_len` argument from `string_to_context_struct()`.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> Refactored version of
> "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> Paul's comments. WDYT?
> I've thrown some inputs at it, and it seems to work.
>
>  security/selinux/ss/mls.c      | 178 ++++++++++++++-------------------
>  security/selinux/ss/mls.h      |   2 +-
>  security/selinux/ss/services.c |  12 +--
>  3 files changed, 82 insertions(+), 110 deletions(-)

Thanks for the rework, this looks much better than what we currently
have.  I have some comments/questions below ...

> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 39475fb455bc..2fe459df3c85 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
>  /*
>   * Set the MLS fields in the security context structure
>   * `context' based on the string representation in
> - * the string `*scontext'.  Update `*scontext' to
> - * point to the end of the string representation of
> - * the MLS fields.
> + * the string `scontext'.
>   *
>   * This function modifies the string in place, inserting
>   * NULL characters to terminate the MLS fields.
> @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
>   */
>  int mls_context_to_sid(struct policydb *pol,
>                        char oldc,
> -                      char **scontext,
> +                      char *scontext,
>                        struct context *context,
>                        struct sidtab *s,
>                        u32 def_sid)
>  {
> -
> -       char delim;
> -       char *scontextp, *p, *rngptr;
> +       char *sensitivity, *cur_cat, *next_cat, *rngptr;
>         struct level_datum *levdatum;
>         struct cat_datum *catdatum, *rngdatum;
> -       int l, rc = -EINVAL;
> +       int l, rc, i;
> +       char *rangep[2];
>
>         if (!pol->mls_enabled) {
> -               if (def_sid != SECSID_NULL && oldc)
> -                       *scontext += strlen(*scontext) + 1;
> -               return 0;
> +               if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> +                       return 0;
> +               return -EINVAL;

Why are we simply not always return 0 in the case where MLS is not
enabled in the policy?  The mls_context_to_sid() is pretty much a
no-op in this case (even more so with your pat regardless of input and
I worry that returning EINVAL here is a deviation from current
behavior which could cause problems.

>         }
>
>         /*
> @@ -261,113 +258,94 @@ int mls_context_to_sid(struct policydb *pol,
>                 struct context *defcon;
>
>                 if (def_sid == SECSID_NULL)
> -                       goto out;
> +                       return -EINVAL;
>
>                 defcon = sidtab_search(s, def_sid);
>                 if (!defcon)
> -                       goto out;
> +                       return -EINVAL;
>
> -               rc = mls_context_cpy(context, defcon);
> -               goto out;
> +               return mls_context_cpy(context, defcon);
>         }
>
> -       /* Extract low sensitivity. */
> -       scontextp = p = *scontext;
> -       while (*p && *p != ':' && *p != '-')
> -               p++;
> -
> -       delim = *p;
> -       if (delim != '\0')
> -               *p++ = '\0';
> +       /*
> +        * If we're dealing with a range, figure out where the two parts
> +        * of the range begin.
> +        */
> +       rangep[0] = scontext;
> +       rangep[1] = strchr(scontext, '-');
> +       if (rangep[1]) {
> +               rangep[1][0] = '\0';
> +               rangep[1]++;
> +       }
>
> +       /* For each part of the range: */
>         for (l = 0; l < 2; l++) {
> -               levdatum = hashtab_search(pol->p_levels.table, scontextp);
> -               if (!levdatum) {
> -                       rc = -EINVAL;
> -                       goto out;
> -               }
> +               /* Split sensitivity and category set. */
> +               sensitivity = rangep[l];
> +               if (sensitivity == NULL)
> +                       break;
> +               next_cat = strchr(sensitivity, ':');
> +               if (next_cat)
> +                       *(next_cat++) = '\0';
>
> +               /* Parse sensitivity. */
> +               levdatum = hashtab_search(pol->p_levels.table, sensitivity);
> +               if (!levdatum)
> +                       return -EINVAL;
>                 context->range.level[l].sens = levdatum->level->sens;
>
> -               if (delim == ':') {
> -                       /* Extract category set. */
> -                       while (1) {
> -                               scontextp = p;
> -                               while (*p && *p != ',' && *p != '-')
> -                                       p++;
> -                               delim = *p;
> -                               if (delim != '\0')
> -                                       *p++ = '\0';
> -
> -                               /* Separate into range if exists */
> -                               rngptr = strchr(scontextp, '.');
> -                               if (rngptr != NULL) {
> -                                       /* Remove '.' */
> -                                       *rngptr++ = '\0';
> -                               }
> +               /* Extract category set. */
> +               while (next_cat != NULL) {
> +                       cur_cat = next_cat;
> +                       next_cat = strchr(next_cat, ',');
> +                       if (next_cat != NULL)
> +                               *(next_cat++) = '\0';
> +
> +                       /* Separate into range if exists */
> +                       rngptr = strchr(cur_cat, '.');
> +                       if (rngptr != NULL) {
> +                               /* Remove '.' */

On the chance you need to respin this patch, you can probably get rid
of the above comment and the if-body braces; we don't have "Remove X"
comments in other similar places in this function.

> +                               *rngptr++ = '\0';
> +                       }
>
> -                               catdatum = hashtab_search(pol->p_cats.table,
> -                                                         scontextp);
> -                               if (!catdatum) {
> -                                       rc = -EINVAL;
> -                                       goto out;
> -                               }
> +                       catdatum = hashtab_search(pol->p_cats.table, cur_cat);
> +                       if (!catdatum)
> +                               return -EINVAL;
>
> -                               rc = ebitmap_set_bit(&context->range.level[l].cat,
> -                                                    catdatum->value - 1, 1);
> -                               if (rc)
> -                                       goto out;
> -
> -                               /* If range, set all categories in range */
> -                               if (rngptr) {
> -                                       int i;
> -
> -                                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> -                                       if (!rngdatum) {
> -                                               rc = -EINVAL;
> -                                               goto out;
> -                                       }
> -
> -                                       if (catdatum->value >= rngdatum->value) {
> -                                               rc = -EINVAL;
> -                                               goto out;
> -                                       }
> -
> -                                       for (i = catdatum->value; i < rngdatum->value; i++) {
> -                                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> -                                               if (rc)
> -                                                       goto out;
> -                                       }
> -                               }
> +                       rc = ebitmap_set_bit(&context->range.level[l].cat,
> +                                            catdatum->value - 1, 1);
> +                       if (rc)
> +                               return rc;
> +
> +                       /* If range, set all categories in range */
> +                       if (rngptr == NULL)
> +                               continue;
> +
> +                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> +                       if (!rngdatum)
> +                               return -EINVAL;
> +
> +                       if (catdatum->value >= rngdatum->value)
> +                               return -EINVAL;
>
> -                               if (delim != ',')
> -                                       break;
> +                       for (i = catdatum->value; i < rngdatum->value; i++) {
> +                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> +                               if (rc)
> +                                       return rc;
>                         }
>                 }
> -               if (delim == '-') {
> -                       /* Extract high sensitivity. */
> -                       scontextp = p;
> -                       while (*p && *p != ':')
> -                               p++;
> -
> -                       delim = *p;
> -                       if (delim != '\0')
> -                               *p++ = '\0';
> -               } else
> -                       break;
>         }
>
> -       if (l == 0) {
> +       /* If we didn't see a '-', the range start is also the range end. */
> +       if (rangep[1] == NULL) {
>                 context->range.level[1].sens = context->range.level[0].sens;
>                 rc = ebitmap_cpy(&context->range.level[1].cat,
>                                  &context->range.level[0].cat);
>                 if (rc)
> -                       goto out;
> +                       return rc;
>         }
> -       *scontext = ++p;
> -       rc = 0;
> -out:
> -       return rc;
> +
> +       return 0;

In the case where we have a MLS policy loaded (pol->mls_enabled != 0)
and scontext is empty (scontext[0] = '\0'), we could end up returning
0 couldn't we?  It seems like we might want a quick check for this
before we parse the low/high portions of the field into the rangep
array.

>  }
>
>  /*
> @@ -379,21 +357,19 @@ int mls_context_to_sid(struct policydb *pol,
>  int mls_from_string(struct policydb *p, char *str, struct context *context,
>                     gfp_t gfp_mask)
>  {
> -       char *tmpstr, *freestr;
> +       char *tmpstr;
>         int rc;
>
>         if (!p->mls_enabled)
>                 return -EINVAL;
>
> -       /* we need freestr because mls_context_to_sid will change
> -          the value of tmpstr */
> -       tmpstr = freestr = kstrdup(str, gfp_mask);
> +       tmpstr = kstrdup(str, gfp_mask);
>         if (!tmpstr) {
>                 rc = -ENOMEM;
>         } else {
> -               rc = mls_context_to_sid(p, ':', &tmpstr, context,
> +               rc = mls_context_to_sid(p, ':', tmpstr, context,
>                                         NULL, SECSID_NULL);
> -               kfree(freestr);
> +               kfree(tmpstr);
>         }
>
>         return rc;
> diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
> index 9a3ff7af70ad..67093647576d 100644
> --- a/security/selinux/ss/mls.h
> +++ b/security/selinux/ss/mls.h
> @@ -34,7 +34,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l);
>
>  int mls_context_to_sid(struct policydb *p,
>                        char oldc,
> -                      char **scontext,
> +                      char *scontext,
>                        struct context *context,
>                        struct sidtab *s,
>                        u32 def_sid);
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index dd2ceec06fef..9212d4dd817a 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -1367,7 +1367,6 @@ int security_sid_to_context_force(struct selinux_state *state, u32 sid,
>  static int string_to_context_struct(struct policydb *pol,
>                                     struct sidtab *sidtabp,
>                                     char *scontext,
> -                                   u32 scontext_len,
>                                     struct context *ctx,
>                                     u32 def_sid)
>  {
> @@ -1428,15 +1427,12 @@ static int string_to_context_struct(struct policydb *pol,
>
>         ctx->type = typdatum->value;
>
> -       rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
> +       rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
>         if (rc)
>                 goto out;
>
> -       rc = -EINVAL;
> -       if ((p - scontext) < scontext_len)
> -               goto out;
> -
>         /* Check the validity of the new context. */
> +       rc = -EINVAL;
>         if (!policydb_context_isvalid(pol, ctx))
>                 goto out;
>         rc = 0;
> @@ -1491,7 +1487,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
>         policydb = &state->ss->policydb;
>         sidtab = &state->ss->sidtab;
>         rc = string_to_context_struct(policydb, sidtab, scontext2,
> -                                     scontext_len, &context, def_sid);
> +                                     &context, def_sid);
>         if (rc == -EINVAL && force) {
>                 context.str = str;
>                 context.len = strlen(str) + 1;
> @@ -1959,7 +1955,7 @@ static int convert_context(u32 key,
>                         goto out;
>
>                 rc = string_to_context_struct(args->newp, NULL, s,
> -                                             c->len, &ctx, SECSID_NULL);
> +                                             &ctx, SECSID_NULL);
>                 kfree(s);
>                 if (!rc) {
>                         printk(KERN_INFO "SELinux:  Context %s became valid (mapped).\n",
> --
> 2.18.0.597.ga71716f1ad-goog

--
paul moore
www.paul-moore.com
Paul Moore Aug. 9, 2018, 2:07 a.m. UTC | #2
On Wed, Aug 8, 2018 at 9:56 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:
> >
> > The intended behavior change for this patch is to reject any MLS strings
> > that contain (trailing) garbage if p->mls_enabled is true.
> >
> > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > parts of the range are extracted before the rest of the parsing. Because
> > now we don't have to scan for two different separators simultaneously
> > everywhere, we can actually switch to strchr() everywhere instead of the
> > open-coded loops that scan for two separators at once.
> >
> > mls_context_to_sid() used to signal how much of the input string was parsed
> > by updating `*scontext`. However, there is actually no case in which
> > mls_context_to_sid() only parses a subset of the input and still returns
> > a success (other than the buggy case with a second '-' in which it
> > incorrectly claims to have consumed the entire string). Turn `scontext`
> > into a simple pointer argument and stop redundantly checking whether the
> > entire input was consumed in string_to_context_struct(). This also lets us
> > remove the `scontext_len` argument from `string_to_context_struct()`.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> > Refactored version of
> > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> > Paul's comments. WDYT?
> > I've thrown some inputs at it, and it seems to work.
> >
> >  security/selinux/ss/mls.c      | 178 ++++++++++++++-------------------
> >  security/selinux/ss/mls.h      |   2 +-
> >  security/selinux/ss/services.c |  12 +--
> >  3 files changed, 82 insertions(+), 110 deletions(-)
>
> Thanks for the rework, this looks much better than what we currently
> have.  I have some comments/questions below ...
>
> > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > index 39475fb455bc..2fe459df3c85 100644
> > --- a/security/selinux/ss/mls.c
> > +++ b/security/selinux/ss/mls.c
> > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> >  /*
> >   * Set the MLS fields in the security context structure
> >   * `context' based on the string representation in
> > - * the string `*scontext'.  Update `*scontext' to
> > - * point to the end of the string representation of
> > - * the MLS fields.
> > + * the string `scontext'.
> >   *
> >   * This function modifies the string in place, inserting
> >   * NULL characters to terminate the MLS fields.
> > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> >   */
> >  int mls_context_to_sid(struct policydb *pol,
> >                        char oldc,
> > -                      char **scontext,
> > +                      char *scontext,
> >                        struct context *context,
> >                        struct sidtab *s,
> >                        u32 def_sid)
> >  {
> > -
> > -       char delim;
> > -       char *scontextp, *p, *rngptr;
> > +       char *sensitivity, *cur_cat, *next_cat, *rngptr;
> >         struct level_datum *levdatum;
> >         struct cat_datum *catdatum, *rngdatum;
> > -       int l, rc = -EINVAL;
> > +       int l, rc, i;
> > +       char *rangep[2];
> >
> >         if (!pol->mls_enabled) {
> > -               if (def_sid != SECSID_NULL && oldc)
> > -                       *scontext += strlen(*scontext) + 1;
> > -               return 0;
> > +               if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > +                       return 0;
> > +               return -EINVAL;
>
> Why are we simply not always return 0 in the case where MLS is not
> enabled in the policy?  The mls_context_to_sid() is pretty much a
> no-op in this case (even more so with your pat regardless of input and
> I worry that returning EINVAL here is a deviation from current
> behavior which could cause problems.

Sorry, I was rephrasing the text above when I accidentally hit send.
While my emails are generally a good source of typos, the above is
pretty bad, so let me try again ...

Why are we simply not always returning 0 in the case where MLS is not
enabled in the policy?  The mls_context_to_sid() function is pretty
much a no-op in this case regardless of input and I worry that
returning EINVAL here is a deviation from current behavior which could
cause problems.

> >         }
> >
> >         /*
> > @@ -261,113 +258,94 @@ int mls_context_to_sid(struct policydb *pol,
> >                 struct context *defcon;
> >
> >                 if (def_sid == SECSID_NULL)
> > -                       goto out;
> > +                       return -EINVAL;
> >
> >                 defcon = sidtab_search(s, def_sid);
> >                 if (!defcon)
> > -                       goto out;
> > +                       return -EINVAL;
> >
> > -               rc = mls_context_cpy(context, defcon);
> > -               goto out;
> > +               return mls_context_cpy(context, defcon);
> >         }
> >
> > -       /* Extract low sensitivity. */
> > -       scontextp = p = *scontext;
> > -       while (*p && *p != ':' && *p != '-')
> > -               p++;
> > -
> > -       delim = *p;
> > -       if (delim != '\0')
> > -               *p++ = '\0';
> > +       /*
> > +        * If we're dealing with a range, figure out where the two parts
> > +        * of the range begin.
> > +        */
> > +       rangep[0] = scontext;
> > +       rangep[1] = strchr(scontext, '-');
> > +       if (rangep[1]) {
> > +               rangep[1][0] = '\0';
> > +               rangep[1]++;
> > +       }
> >
> > +       /* For each part of the range: */
> >         for (l = 0; l < 2; l++) {
> > -               levdatum = hashtab_search(pol->p_levels.table, scontextp);
> > -               if (!levdatum) {
> > -                       rc = -EINVAL;
> > -                       goto out;
> > -               }
> > +               /* Split sensitivity and category set. */
> > +               sensitivity = rangep[l];
> > +               if (sensitivity == NULL)
> > +                       break;
> > +               next_cat = strchr(sensitivity, ':');
> > +               if (next_cat)
> > +                       *(next_cat++) = '\0';
> >
> > +               /* Parse sensitivity. */
> > +               levdatum = hashtab_search(pol->p_levels.table, sensitivity);
> > +               if (!levdatum)
> > +                       return -EINVAL;
> >                 context->range.level[l].sens = levdatum->level->sens;
> >
> > -               if (delim == ':') {
> > -                       /* Extract category set. */
> > -                       while (1) {
> > -                               scontextp = p;
> > -                               while (*p && *p != ',' && *p != '-')
> > -                                       p++;
> > -                               delim = *p;
> > -                               if (delim != '\0')
> > -                                       *p++ = '\0';
> > -
> > -                               /* Separate into range if exists */
> > -                               rngptr = strchr(scontextp, '.');
> > -                               if (rngptr != NULL) {
> > -                                       /* Remove '.' */
> > -                                       *rngptr++ = '\0';
> > -                               }
> > +               /* Extract category set. */
> > +               while (next_cat != NULL) {
> > +                       cur_cat = next_cat;
> > +                       next_cat = strchr(next_cat, ',');
> > +                       if (next_cat != NULL)
> > +                               *(next_cat++) = '\0';
> > +
> > +                       /* Separate into range if exists */
> > +                       rngptr = strchr(cur_cat, '.');
> > +                       if (rngptr != NULL) {
> > +                               /* Remove '.' */
>
> On the chance you need to respin this patch, you can probably get rid
> of the above comment and the if-body braces; we don't have "Remove X"
> comments in other similar places in this function.
>
> > +                               *rngptr++ = '\0';
> > +                       }
> >
> > -                               catdatum = hashtab_search(pol->p_cats.table,
> > -                                                         scontextp);
> > -                               if (!catdatum) {
> > -                                       rc = -EINVAL;
> > -                                       goto out;
> > -                               }
> > +                       catdatum = hashtab_search(pol->p_cats.table, cur_cat);
> > +                       if (!catdatum)
> > +                               return -EINVAL;
> >
> > -                               rc = ebitmap_set_bit(&context->range.level[l].cat,
> > -                                                    catdatum->value - 1, 1);
> > -                               if (rc)
> > -                                       goto out;
> > -
> > -                               /* If range, set all categories in range */
> > -                               if (rngptr) {
> > -                                       int i;
> > -
> > -                                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > -                                       if (!rngdatum) {
> > -                                               rc = -EINVAL;
> > -                                               goto out;
> > -                                       }
> > -
> > -                                       if (catdatum->value >= rngdatum->value) {
> > -                                               rc = -EINVAL;
> > -                                               goto out;
> > -                                       }
> > -
> > -                                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > -                                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > -                                               if (rc)
> > -                                                       goto out;
> > -                                       }
> > -                               }
> > +                       rc = ebitmap_set_bit(&context->range.level[l].cat,
> > +                                            catdatum->value - 1, 1);
> > +                       if (rc)
> > +                               return rc;
> > +
> > +                       /* If range, set all categories in range */
> > +                       if (rngptr == NULL)
> > +                               continue;
> > +
> > +                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > +                       if (!rngdatum)
> > +                               return -EINVAL;
> > +
> > +                       if (catdatum->value >= rngdatum->value)
> > +                               return -EINVAL;
> >
> > -                               if (delim != ',')
> > -                                       break;
> > +                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > +                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > +                               if (rc)
> > +                                       return rc;
> >                         }
> >                 }
> > -               if (delim == '-') {
> > -                       /* Extract high sensitivity. */
> > -                       scontextp = p;
> > -                       while (*p && *p != ':')
> > -                               p++;
> > -
> > -                       delim = *p;
> > -                       if (delim != '\0')
> > -                               *p++ = '\0';
> > -               } else
> > -                       break;
> >         }
> >
> > -       if (l == 0) {
> > +       /* If we didn't see a '-', the range start is also the range end. */
> > +       if (rangep[1] == NULL) {
> >                 context->range.level[1].sens = context->range.level[0].sens;
> >                 rc = ebitmap_cpy(&context->range.level[1].cat,
> >                                  &context->range.level[0].cat);
> >                 if (rc)
> > -                       goto out;
> > +                       return rc;
> >         }
> > -       *scontext = ++p;
> > -       rc = 0;
> > -out:
> > -       return rc;
> > +
> > +       return 0;
>
> In the case where we have a MLS policy loaded (pol->mls_enabled != 0)
> and scontext is empty (scontext[0] = '\0'), we could end up returning
> 0 couldn't we?  It seems like we might want a quick check for this
> before we parse the low/high portions of the field into the rangep
> array.
>
> >  }
> >
> >  /*
> > @@ -379,21 +357,19 @@ int mls_context_to_sid(struct policydb *pol,
> >  int mls_from_string(struct policydb *p, char *str, struct context *context,
> >                     gfp_t gfp_mask)
> >  {
> > -       char *tmpstr, *freestr;
> > +       char *tmpstr;
> >         int rc;
> >
> >         if (!p->mls_enabled)
> >                 return -EINVAL;
> >
> > -       /* we need freestr because mls_context_to_sid will change
> > -          the value of tmpstr */
> > -       tmpstr = freestr = kstrdup(str, gfp_mask);
> > +       tmpstr = kstrdup(str, gfp_mask);
> >         if (!tmpstr) {
> >                 rc = -ENOMEM;
> >         } else {
> > -               rc = mls_context_to_sid(p, ':', &tmpstr, context,
> > +               rc = mls_context_to_sid(p, ':', tmpstr, context,
> >                                         NULL, SECSID_NULL);
> > -               kfree(freestr);
> > +               kfree(tmpstr);
> >         }
> >
> >         return rc;
> > diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
> > index 9a3ff7af70ad..67093647576d 100644
> > --- a/security/selinux/ss/mls.h
> > +++ b/security/selinux/ss/mls.h
> > @@ -34,7 +34,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l);
> >
> >  int mls_context_to_sid(struct policydb *p,
> >                        char oldc,
> > -                      char **scontext,
> > +                      char *scontext,
> >                        struct context *context,
> >                        struct sidtab *s,
> >                        u32 def_sid);
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index dd2ceec06fef..9212d4dd817a 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -1367,7 +1367,6 @@ int security_sid_to_context_force(struct selinux_state *state, u32 sid,
> >  static int string_to_context_struct(struct policydb *pol,
> >                                     struct sidtab *sidtabp,
> >                                     char *scontext,
> > -                                   u32 scontext_len,
> >                                     struct context *ctx,
> >                                     u32 def_sid)
> >  {
> > @@ -1428,15 +1427,12 @@ static int string_to_context_struct(struct policydb *pol,
> >
> >         ctx->type = typdatum->value;
> >
> > -       rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
> > +       rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> >         if (rc)
> >                 goto out;
> >
> > -       rc = -EINVAL;
> > -       if ((p - scontext) < scontext_len)
> > -               goto out;
> > -
> >         /* Check the validity of the new context. */
> > +       rc = -EINVAL;
> >         if (!policydb_context_isvalid(pol, ctx))
> >                 goto out;
> >         rc = 0;
> > @@ -1491,7 +1487,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
> >         policydb = &state->ss->policydb;
> >         sidtab = &state->ss->sidtab;
> >         rc = string_to_context_struct(policydb, sidtab, scontext2,
> > -                                     scontext_len, &context, def_sid);
> > +                                     &context, def_sid);
> >         if (rc == -EINVAL && force) {
> >                 context.str = str;
> >                 context.len = strlen(str) + 1;
> > @@ -1959,7 +1955,7 @@ static int convert_context(u32 key,
> >                         goto out;
> >
> >                 rc = string_to_context_struct(args->newp, NULL, s,
> > -                                             c->len, &ctx, SECSID_NULL);
> > +                                             &ctx, SECSID_NULL);
> >                 kfree(s);
> >                 if (!rc) {
> >                         printk(KERN_INFO "SELinux:  Context %s became valid (mapped).\n",
> > --
> > 2.18.0.597.ga71716f1ad-goog
>
> --
> paul moore
> www.paul-moore.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jann Horn via Selinux Aug. 10, 2018, 11:01 p.m. UTC | #3
On Thu, Aug 9, 2018 at 4:07 AM Paul Moore <pmoore@redhat.com> wrote:
>
> On Wed, Aug 8, 2018 at 9:56 PM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:
> > >
> > > The intended behavior change for this patch is to reject any MLS strings
> > > that contain (trailing) garbage if p->mls_enabled is true.
> > >
> > > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > > parts of the range are extracted before the rest of the parsing. Because
> > > now we don't have to scan for two different separators simultaneously
> > > everywhere, we can actually switch to strchr() everywhere instead of the
> > > open-coded loops that scan for two separators at once.
> > >
> > > mls_context_to_sid() used to signal how much of the input string was parsed
> > > by updating `*scontext`. However, there is actually no case in which
> > > mls_context_to_sid() only parses a subset of the input and still returns
> > > a success (other than the buggy case with a second '-' in which it
> > > incorrectly claims to have consumed the entire string). Turn `scontext`
> > > into a simple pointer argument and stop redundantly checking whether the
> > > entire input was consumed in string_to_context_struct(). This also lets us
> > > remove the `scontext_len` argument from `string_to_context_struct()`.
> > >
> > > Signed-off-by: Jann Horn <jannh@google.com>
> > > ---
> > > Refactored version of
> > > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> > > Paul's comments. WDYT?
> > > I've thrown some inputs at it, and it seems to work.
> > >
> > >  security/selinux/ss/mls.c      | 178 ++++++++++++++-------------------
> > >  security/selinux/ss/mls.h      |   2 +-
> > >  security/selinux/ss/services.c |  12 +--
> > >  3 files changed, 82 insertions(+), 110 deletions(-)
> >
> > Thanks for the rework, this looks much better than what we currently
> > have.  I have some comments/questions below ...
> >
> > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > > index 39475fb455bc..2fe459df3c85 100644
> > > --- a/security/selinux/ss/mls.c
> > > +++ b/security/selinux/ss/mls.c
> > > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> > >  /*
> > >   * Set the MLS fields in the security context structure
> > >   * `context' based on the string representation in
> > > - * the string `*scontext'.  Update `*scontext' to
> > > - * point to the end of the string representation of
> > > - * the MLS fields.
> > > + * the string `scontext'.
> > >   *
> > >   * This function modifies the string in place, inserting
> > >   * NULL characters to terminate the MLS fields.
> > > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> > >   */
> > >  int mls_context_to_sid(struct policydb *pol,
> > >                        char oldc,
> > > -                      char **scontext,
> > > +                      char *scontext,
> > >                        struct context *context,
> > >                        struct sidtab *s,
> > >                        u32 def_sid)
> > >  {
> > > -
> > > -       char delim;
> > > -       char *scontextp, *p, *rngptr;
> > > +       char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > >         struct level_datum *levdatum;
> > >         struct cat_datum *catdatum, *rngdatum;
> > > -       int l, rc = -EINVAL;
> > > +       int l, rc, i;
> > > +       char *rangep[2];
> > >
> > >         if (!pol->mls_enabled) {
> > > -               if (def_sid != SECSID_NULL && oldc)
> > > -                       *scontext += strlen(*scontext) + 1;
> > > -               return 0;
> > > +               if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > > +                       return 0;
> > > +               return -EINVAL;
> >
> > Why are we simply not always return 0 in the case where MLS is not
> > enabled in the policy?  The mls_context_to_sid() is pretty much a
> > no-op in this case (even more so with your pat regardless of input and
> > I worry that returning EINVAL here is a deviation from current
> > behavior which could cause problems.
>
> Sorry, I was rephrasing the text above when I accidentally hit send.
> While my emails are generally a good source of typos, the above is
> pretty bad, so let me try again ...
>
> Why are we simply not always returning 0 in the case where MLS is not
> enabled in the policy?  The mls_context_to_sid() function is pretty
> much a no-op in this case regardless of input and I worry that
> returning EINVAL here is a deviation from current behavior which could
> cause problems.

Reverse call graph of mls_context_to_sid():

mls_context_to_sid()
    mls_from_string()
        selinux_audit_rule_init()
            [...]
    string_to_context_struct()
        security_context_to_sid_core()
            [...]
        convert_context()
            [...]

string_to_context_struct() currently has the following code:

        rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
        if (rc)
                goto out;

        rc = -EINVAL;
        if ((p - scontext) < scontext_len)
                goto out;

In my patch, I tried to preserve the behavior of
string_to_context_struct(), at the expense of slightly changing the
behavior of selinux_audit_rule_init().

But if you think that's a bad idea or unnecessary, say so and I'll change it.

> > >         }
> > >
> > >         /*
> > > @@ -261,113 +258,94 @@ int mls_context_to_sid(struct policydb *pol,
> > >                 struct context *defcon;
> > >
> > >                 if (def_sid == SECSID_NULL)
> > > -                       goto out;
> > > +                       return -EINVAL;
> > >
> > >                 defcon = sidtab_search(s, def_sid);
> > >                 if (!defcon)
> > > -                       goto out;
> > > +                       return -EINVAL;
> > >
> > > -               rc = mls_context_cpy(context, defcon);
> > > -               goto out;
> > > +               return mls_context_cpy(context, defcon);
> > >         }
> > >
> > > -       /* Extract low sensitivity. */
> > > -       scontextp = p = *scontext;
> > > -       while (*p && *p != ':' && *p != '-')
> > > -               p++;
> > > -
> > > -       delim = *p;
> > > -       if (delim != '\0')
> > > -               *p++ = '\0';
> > > +       /*
> > > +        * If we're dealing with a range, figure out where the two parts
> > > +        * of the range begin.
> > > +        */
> > > +       rangep[0] = scontext;
> > > +       rangep[1] = strchr(scontext, '-');
> > > +       if (rangep[1]) {
> > > +               rangep[1][0] = '\0';
> > > +               rangep[1]++;
> > > +       }
> > >
> > > +       /* For each part of the range: */
> > >         for (l = 0; l < 2; l++) {
> > > -               levdatum = hashtab_search(pol->p_levels.table, scontextp);
> > > -               if (!levdatum) {
> > > -                       rc = -EINVAL;
> > > -                       goto out;
> > > -               }
> > > +               /* Split sensitivity and category set. */
> > > +               sensitivity = rangep[l];
> > > +               if (sensitivity == NULL)
> > > +                       break;
> > > +               next_cat = strchr(sensitivity, ':');
> > > +               if (next_cat)
> > > +                       *(next_cat++) = '\0';
> > >
> > > +               /* Parse sensitivity. */
> > > +               levdatum = hashtab_search(pol->p_levels.table, sensitivity);
> > > +               if (!levdatum)
> > > +                       return -EINVAL;
> > >                 context->range.level[l].sens = levdatum->level->sens;
> > >
> > > -               if (delim == ':') {
> > > -                       /* Extract category set. */
> > > -                       while (1) {
> > > -                               scontextp = p;
> > > -                               while (*p && *p != ',' && *p != '-')
> > > -                                       p++;
> > > -                               delim = *p;
> > > -                               if (delim != '\0')
> > > -                                       *p++ = '\0';
> > > -
> > > -                               /* Separate into range if exists */
> > > -                               rngptr = strchr(scontextp, '.');
> > > -                               if (rngptr != NULL) {
> > > -                                       /* Remove '.' */
> > > -                                       *rngptr++ = '\0';
> > > -                               }
> > > +               /* Extract category set. */
> > > +               while (next_cat != NULL) {
> > > +                       cur_cat = next_cat;
> > > +                       next_cat = strchr(next_cat, ',');
> > > +                       if (next_cat != NULL)
> > > +                               *(next_cat++) = '\0';
> > > +
> > > +                       /* Separate into range if exists */
> > > +                       rngptr = strchr(cur_cat, '.');
> > > +                       if (rngptr != NULL) {
> > > +                               /* Remove '.' */
> >
> > On the chance you need to respin this patch, you can probably get rid
> > of the above comment and the if-body braces; we don't have "Remove X"
> > comments in other similar places in this function.
> >
> > > +                               *rngptr++ = '\0';
> > > +                       }
> > >
> > > -                               catdatum = hashtab_search(pol->p_cats.table,
> > > -                                                         scontextp);
> > > -                               if (!catdatum) {
> > > -                                       rc = -EINVAL;
> > > -                                       goto out;
> > > -                               }
> > > +                       catdatum = hashtab_search(pol->p_cats.table, cur_cat);
> > > +                       if (!catdatum)
> > > +                               return -EINVAL;
> > >
> > > -                               rc = ebitmap_set_bit(&context->range.level[l].cat,
> > > -                                                    catdatum->value - 1, 1);
> > > -                               if (rc)
> > > -                                       goto out;
> > > -
> > > -                               /* If range, set all categories in range */
> > > -                               if (rngptr) {
> > > -                                       int i;
> > > -
> > > -                                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > > -                                       if (!rngdatum) {
> > > -                                               rc = -EINVAL;
> > > -                                               goto out;
> > > -                                       }
> > > -
> > > -                                       if (catdatum->value >= rngdatum->value) {
> > > -                                               rc = -EINVAL;
> > > -                                               goto out;
> > > -                                       }
> > > -
> > > -                                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > > -                                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > > -                                               if (rc)
> > > -                                                       goto out;
> > > -                                       }
> > > -                               }
> > > +                       rc = ebitmap_set_bit(&context->range.level[l].cat,
> > > +                                            catdatum->value - 1, 1);
> > > +                       if (rc)
> > > +                               return rc;
> > > +
> > > +                       /* If range, set all categories in range */
> > > +                       if (rngptr == NULL)
> > > +                               continue;
> > > +
> > > +                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > > +                       if (!rngdatum)
> > > +                               return -EINVAL;
> > > +
> > > +                       if (catdatum->value >= rngdatum->value)
> > > +                               return -EINVAL;
> > >
> > > -                               if (delim != ',')
> > > -                                       break;
> > > +                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > > +                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > > +                               if (rc)
> > > +                                       return rc;
> > >                         }
> > >                 }
> > > -               if (delim == '-') {
> > > -                       /* Extract high sensitivity. */
> > > -                       scontextp = p;
> > > -                       while (*p && *p != ':')
> > > -                               p++;
> > > -
> > > -                       delim = *p;
> > > -                       if (delim != '\0')
> > > -                               *p++ = '\0';
> > > -               } else
> > > -                       break;
> > >         }
> > >
> > > -       if (l == 0) {
> > > +       /* If we didn't see a '-', the range start is also the range end. */
> > > +       if (rangep[1] == NULL) {
> > >                 context->range.level[1].sens = context->range.level[0].sens;
> > >                 rc = ebitmap_cpy(&context->range.level[1].cat,
> > >                                  &context->range.level[0].cat);
> > >                 if (rc)
> > > -                       goto out;
> > > +                       return rc;
> > >         }
> > > -       *scontext = ++p;
> > > -       rc = 0;
> > > -out:
> > > -       return rc;
> > > +
> > > +       return 0;
> >
> > In the case where we have a MLS policy loaded (pol->mls_enabled != 0)
> > and scontext is empty (scontext[0] = '\0'), we could end up returning
> > 0 couldn't we?  It seems like we might want a quick check for this
> > before we parse the low/high portions of the field into the rangep
> > array.
> >
> > >  }
> > >
> > >  /*
> > > @@ -379,21 +357,19 @@ int mls_context_to_sid(struct policydb *pol,
> > >  int mls_from_string(struct policydb *p, char *str, struct context *context,
> > >                     gfp_t gfp_mask)
> > >  {
> > > -       char *tmpstr, *freestr;
> > > +       char *tmpstr;
> > >         int rc;
> > >
> > >         if (!p->mls_enabled)
> > >                 return -EINVAL;
> > >
> > > -       /* we need freestr because mls_context_to_sid will change
> > > -          the value of tmpstr */
> > > -       tmpstr = freestr = kstrdup(str, gfp_mask);
> > > +       tmpstr = kstrdup(str, gfp_mask);
> > >         if (!tmpstr) {
> > >                 rc = -ENOMEM;
> > >         } else {
> > > -               rc = mls_context_to_sid(p, ':', &tmpstr, context,
> > > +               rc = mls_context_to_sid(p, ':', tmpstr, context,
> > >                                         NULL, SECSID_NULL);
> > > -               kfree(freestr);
> > > +               kfree(tmpstr);
> > >         }
> > >
> > >         return rc;
> > > diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
> > > index 9a3ff7af70ad..67093647576d 100644
> > > --- a/security/selinux/ss/mls.h
> > > +++ b/security/selinux/ss/mls.h
> > > @@ -34,7 +34,7 @@ int mls_level_isvalid(struct policydb *p, struct mls_level *l);
> > >
> > >  int mls_context_to_sid(struct policydb *p,
> > >                        char oldc,
> > > -                      char **scontext,
> > > +                      char *scontext,
> > >                        struct context *context,
> > >                        struct sidtab *s,
> > >                        u32 def_sid);
> > > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > > index dd2ceec06fef..9212d4dd817a 100644
> > > --- a/security/selinux/ss/services.c
> > > +++ b/security/selinux/ss/services.c
> > > @@ -1367,7 +1367,6 @@ int security_sid_to_context_force(struct selinux_state *state, u32 sid,
> > >  static int string_to_context_struct(struct policydb *pol,
> > >                                     struct sidtab *sidtabp,
> > >                                     char *scontext,
> > > -                                   u32 scontext_len,
> > >                                     struct context *ctx,
> > >                                     u32 def_sid)
> > >  {
> > > @@ -1428,15 +1427,12 @@ static int string_to_context_struct(struct policydb *pol,
> > >
> > >         ctx->type = typdatum->value;
> > >
> > > -       rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
> > > +       rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
> > >         if (rc)
> > >                 goto out;
> > >
> > > -       rc = -EINVAL;
> > > -       if ((p - scontext) < scontext_len)
> > > -               goto out;
> > > -
> > >         /* Check the validity of the new context. */
> > > +       rc = -EINVAL;
> > >         if (!policydb_context_isvalid(pol, ctx))
> > >                 goto out;
> > >         rc = 0;
> > > @@ -1491,7 +1487,7 @@ static int security_context_to_sid_core(struct selinux_state *state,
> > >         policydb = &state->ss->policydb;
> > >         sidtab = &state->ss->sidtab;
> > >         rc = string_to_context_struct(policydb, sidtab, scontext2,
> > > -                                     scontext_len, &context, def_sid);
> > > +                                     &context, def_sid);
> > >         if (rc == -EINVAL && force) {
> > >                 context.str = str;
> > >                 context.len = strlen(str) + 1;
> > > @@ -1959,7 +1955,7 @@ static int convert_context(u32 key,
> > >                         goto out;
> > >
> > >                 rc = string_to_context_struct(args->newp, NULL, s,
> > > -                                             c->len, &ctx, SECSID_NULL);
> > > +                                             &ctx, SECSID_NULL);
> > >                 kfree(s);
> > >                 if (!rc) {
> > >                         printk(KERN_INFO "SELinux:  Context %s became valid (mapped).\n",
> > > --
> > > 2.18.0.597.ga71716f1ad-goog
> >
> > --
> > paul moore
> > www.paul-moore.com
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> paul moore
> security @ redhat
Paul Moore Aug. 13, 2018, 11:38 p.m. UTC | #4
On Fri, Aug 10, 2018 at 7:01 PM Jann Horn <jannh@google.com> wrote:
> On Thu, Aug 9, 2018 at 4:07 AM Paul Moore <pmoore@redhat.com> wrote:
> > On Wed, Aug 8, 2018 at 9:56 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:
> > > >
> > > > The intended behavior change for this patch is to reject any MLS strings
> > > > that contain (trailing) garbage if p->mls_enabled is true.
> > > >
> > > > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > > > parts of the range are extracted before the rest of the parsing. Because
> > > > now we don't have to scan for two different separators simultaneously
> > > > everywhere, we can actually switch to strchr() everywhere instead of the
> > > > open-coded loops that scan for two separators at once.
> > > >
> > > > mls_context_to_sid() used to signal how much of the input string was parsed
> > > > by updating `*scontext`. However, there is actually no case in which
> > > > mls_context_to_sid() only parses a subset of the input and still returns
> > > > a success (other than the buggy case with a second '-' in which it
> > > > incorrectly claims to have consumed the entire string). Turn `scontext`
> > > > into a simple pointer argument and stop redundantly checking whether the
> > > > entire input was consumed in string_to_context_struct(). This also lets us
> > > > remove the `scontext_len` argument from `string_to_context_struct()`.
> > > >
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > ---
> > > > Refactored version of
> > > > "[PATCH] selinux: stricter parsing in mls_context_to_sid()" based on
> > > > Paul's comments. WDYT?
> > > > I've thrown some inputs at it, and it seems to work.
> > > >
> > > >  security/selinux/ss/mls.c      | 178 ++++++++++++++-------------------
> > > >  security/selinux/ss/mls.h      |   2 +-
> > > >  security/selinux/ss/services.c |  12 +--
> > > >  3 files changed, 82 insertions(+), 110 deletions(-)
> > >
> > > Thanks for the rework, this looks much better than what we currently
> > > have.  I have some comments/questions below ...
> > >
> > > > diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> > > > index 39475fb455bc..2fe459df3c85 100644
> > > > --- a/security/selinux/ss/mls.c
> > > > +++ b/security/selinux/ss/mls.c
> > > > @@ -218,9 +218,7 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> > > >  /*
> > > >   * Set the MLS fields in the security context structure
> > > >   * `context' based on the string representation in
> > > > - * the string `*scontext'.  Update `*scontext' to
> > > > - * point to the end of the string representation of
> > > > - * the MLS fields.
> > > > + * the string `scontext'.
> > > >   *
> > > >   * This function modifies the string in place, inserting
> > > >   * NULL characters to terminate the MLS fields.
> > > > @@ -235,22 +233,21 @@ int mls_context_isvalid(struct policydb *p, struct context *c)
> > > >   */
> > > >  int mls_context_to_sid(struct policydb *pol,
> > > >                        char oldc,
> > > > -                      char **scontext,
> > > > +                      char *scontext,
> > > >                        struct context *context,
> > > >                        struct sidtab *s,
> > > >                        u32 def_sid)
> > > >  {
> > > > -
> > > > -       char delim;
> > > > -       char *scontextp, *p, *rngptr;
> > > > +       char *sensitivity, *cur_cat, *next_cat, *rngptr;
> > > >         struct level_datum *levdatum;
> > > >         struct cat_datum *catdatum, *rngdatum;
> > > > -       int l, rc = -EINVAL;
> > > > +       int l, rc, i;
> > > > +       char *rangep[2];
> > > >
> > > >         if (!pol->mls_enabled) {
> > > > -               if (def_sid != SECSID_NULL && oldc)
> > > > -                       *scontext += strlen(*scontext) + 1;
> > > > -               return 0;
> > > > +               if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
> > > > +                       return 0;
> > > > +               return -EINVAL;
> > >
> > > Why are we simply not always return 0 in the case where MLS is not
> > > enabled in the policy?  The mls_context_to_sid() is pretty much a
> > > no-op in this case (even more so with your pat regardless of input and
> > > I worry that returning EINVAL here is a deviation from current
> > > behavior which could cause problems.
> >
> > Sorry, I was rephrasing the text above when I accidentally hit send.
> > While my emails are generally a good source of typos, the above is
> > pretty bad, so let me try again ...
> >
> > Why are we simply not always returning 0 in the case where MLS is not
> > enabled in the policy?  The mls_context_to_sid() function is pretty
> > much a no-op in this case regardless of input and I worry that
> > returning EINVAL here is a deviation from current behavior which could
> > cause problems.
>
> Reverse call graph of mls_context_to_sid():
>
> mls_context_to_sid()
>     mls_from_string()
>         selinux_audit_rule_init()
>             [...]
>     string_to_context_struct()
>         security_context_to_sid_core()
>             [...]
>         convert_context()
>             [...]
>
> string_to_context_struct() currently has the following code:
>
>         rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
>         if (rc)
>                 goto out;
>
>         rc = -EINVAL;
>         if ((p - scontext) < scontext_len)
>                 goto out;
>
> In my patch, I tried to preserve the behavior of
> string_to_context_struct(), at the expense of slightly changing the
> behavior of selinux_audit_rule_init().
>
> But if you think that's a bad idea or unnecessary, say so and I'll change it.

I'll be honest and mention that I kinda spaced on the change you made
to string_to_context_struct() while I was looking over the
mls_context_to_sid() changes.  As you point out, I believe
string_to_context_struct() will continue to work as expected for
callers so that should be okay.

The selinux_audit_rule_init() function is a little different, but
looking at some of the callers, and thinking about it a bit more, it
probably isn't a bad thing to return EINVAL as in your patch.  The
label isn't valid given that the system isn't in MLS mode and letting
admins know that should be okay.  So I guess we can leave this section
of the patch as-is, but understand that we might need to tweak this if
we end up breaking something important.

As an aside, I believe my other comments on this patch still stand.
It's a nice improvement but I think there are some other small things
that need to be addressed.
Jann Horn via Selinux Aug. 31, 2018, 3:46 p.m. UTC | #5
On Thu, Aug 9, 2018 at 3:56 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:
> >
> > The intended behavior change for this patch is to reject any MLS strings
> > that contain (trailing) garbage if p->mls_enabled is true.
> >
> > As suggested by Paul Moore, change mls_context_to_sid() so that the two
> > parts of the range are extracted before the rest of the parsing. Because
> > now we don't have to scan for two different separators simultaneously
> > everywhere, we can actually switch to strchr() everywhere instead of the
> > open-coded loops that scan for two separators at once.
> >
> > mls_context_to_sid() used to signal how much of the input string was parsed
> > by updating `*scontext`. However, there is actually no case in which
> > mls_context_to_sid() only parses a subset of the input and still returns
> > a success (other than the buggy case with a second '-' in which it
> > incorrectly claims to have consumed the entire string). Turn `scontext`
> > into a simple pointer argument and stop redundantly checking whether the
> > entire input was consumed in string_to_context_struct(). This also lets us
> > remove the `scontext_len` argument from `string_to_context_struct()`.
[...]
> > -       /* Extract low sensitivity. */
> > -       scontextp = p = *scontext;
> > -       while (*p && *p != ':' && *p != '-')
> > -               p++;
> > -
> > -       delim = *p;
> > -       if (delim != '\0')
> > -               *p++ = '\0';
> > +       /*
> > +        * If we're dealing with a range, figure out where the two parts
> > +        * of the range begin.
> > +        */
> > +       rangep[0] = scontext;
> > +       rangep[1] = strchr(scontext, '-');
> > +       if (rangep[1]) {
> > +               rangep[1][0] = '\0';
> > +               rangep[1]++;
> > +       }
> >
> > +       /* For each part of the range: */
> >         for (l = 0; l < 2; l++) {
> > -               levdatum = hashtab_search(pol->p_levels.table, scontextp);
> > -               if (!levdatum) {
> > -                       rc = -EINVAL;
> > -                       goto out;
> > -               }
> > +               /* Split sensitivity and category set. */
> > +               sensitivity = rangep[l];
> > +               if (sensitivity == NULL)
> > +                       break;
> > +               next_cat = strchr(sensitivity, ':');
> > +               if (next_cat)
> > +                       *(next_cat++) = '\0';
> >
> > +               /* Parse sensitivity. */
> > +               levdatum = hashtab_search(pol->p_levels.table, sensitivity);
> > +               if (!levdatum)
> > +                       return -EINVAL;
> >                 context->range.level[l].sens = levdatum->level->sens;
> >
> > -               if (delim == ':') {
> > -                       /* Extract category set. */
> > -                       while (1) {
> > -                               scontextp = p;
> > -                               while (*p && *p != ',' && *p != '-')
> > -                                       p++;
> > -                               delim = *p;
> > -                               if (delim != '\0')
> > -                                       *p++ = '\0';
> > -
> > -                               /* Separate into range if exists */
> > -                               rngptr = strchr(scontextp, '.');
> > -                               if (rngptr != NULL) {
> > -                                       /* Remove '.' */
> > -                                       *rngptr++ = '\0';
> > -                               }
> > +               /* Extract category set. */
> > +               while (next_cat != NULL) {
> > +                       cur_cat = next_cat;
> > +                       next_cat = strchr(next_cat, ',');
> > +                       if (next_cat != NULL)
> > +                               *(next_cat++) = '\0';
> > +
> > +                       /* Separate into range if exists */
> > +                       rngptr = strchr(cur_cat, '.');
> > +                       if (rngptr != NULL) {
> > +                               /* Remove '.' */
>
> On the chance you need to respin this patch, you can probably get rid
> of the above comment and the if-body braces; we don't have "Remove X"
> comments in other similar places in this function.

I'll amend that.

> > +                               *rngptr++ = '\0';
> > +                       }
> >
> > -                               catdatum = hashtab_search(pol->p_cats.table,
> > -                                                         scontextp);
> > -                               if (!catdatum) {
> > -                                       rc = -EINVAL;
> > -                                       goto out;
> > -                               }
> > +                       catdatum = hashtab_search(pol->p_cats.table, cur_cat);
> > +                       if (!catdatum)
> > +                               return -EINVAL;
> >
> > -                               rc = ebitmap_set_bit(&context->range.level[l].cat,
> > -                                                    catdatum->value - 1, 1);
> > -                               if (rc)
> > -                                       goto out;
> > -
> > -                               /* If range, set all categories in range */
> > -                               if (rngptr) {
> > -                                       int i;
> > -
> > -                                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > -                                       if (!rngdatum) {
> > -                                               rc = -EINVAL;
> > -                                               goto out;
> > -                                       }
> > -
> > -                                       if (catdatum->value >= rngdatum->value) {
> > -                                               rc = -EINVAL;
> > -                                               goto out;
> > -                                       }
> > -
> > -                                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > -                                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > -                                               if (rc)
> > -                                                       goto out;
> > -                                       }
> > -                               }
> > +                       rc = ebitmap_set_bit(&context->range.level[l].cat,
> > +                                            catdatum->value - 1, 1);
> > +                       if (rc)
> > +                               return rc;
> > +
> > +                       /* If range, set all categories in range */
> > +                       if (rngptr == NULL)
> > +                               continue;
> > +
> > +                       rngdatum = hashtab_search(pol->p_cats.table, rngptr);
> > +                       if (!rngdatum)
> > +                               return -EINVAL;
> > +
> > +                       if (catdatum->value >= rngdatum->value)
> > +                               return -EINVAL;
> >
> > -                               if (delim != ',')
> > -                                       break;
> > +                       for (i = catdatum->value; i < rngdatum->value; i++) {
> > +                               rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
> > +                               if (rc)
> > +                                       return rc;
> >                         }
> >                 }
> > -               if (delim == '-') {
> > -                       /* Extract high sensitivity. */
> > -                       scontextp = p;
> > -                       while (*p && *p != ':')
> > -                               p++;
> > -
> > -                       delim = *p;
> > -                       if (delim != '\0')
> > -                               *p++ = '\0';
> > -               } else
> > -                       break;
> >         }
> >
> > -       if (l == 0) {
> > +       /* If we didn't see a '-', the range start is also the range end. */
> > +       if (rangep[1] == NULL) {
> >                 context->range.level[1].sens = context->range.level[0].sens;
> >                 rc = ebitmap_cpy(&context->range.level[1].cat,
> >                                  &context->range.level[0].cat);
> >                 if (rc)
> > -                       goto out;
> > +                       return rc;
> >         }
> > -       *scontext = ++p;
> > -       rc = 0;
> > -out:
> > -       return rc;
> > +
> > +       return 0;
>
> In the case where we have a MLS policy loaded (pol->mls_enabled != 0)
> and scontext is empty (scontext[0] = '\0'), we could end up returning
> 0 couldn't we?  It seems like we might want a quick check for this
> before we parse the low/high portions of the field into the rangep
> array.

I don't think so. In the first loop iteration, `sensitivity` will be
an empty string, and so the hashtab_search() should return NULL,
leading to -EINVAL. Am I missing something?

> As an aside, I believe my other comments on this patch still stand.
> It's a nice improvement but I think there are some other small things
> that need to be addressed.

Is there anything I need to fix apart from the overly verbose comment
and the unnecessary curly braces?
Paul Moore Sept. 5, 2018, 10:16 p.m. UTC | #6
On Fri, Aug 31, 2018 at 11:47 AM Jann Horn <jannh@google.com> wrote:
> On Thu, Aug 9, 2018 at 3:56 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn <jannh@google.com> wrote:

...

> > In the case where we have a MLS policy loaded (pol->mls_enabled != 0)
> > and scontext is empty (scontext[0] = '\0'), we could end up returning
> > 0 couldn't we?  It seems like we might want a quick check for this
> > before we parse the low/high portions of the field into the rangep
> > array.
>
> I don't think so. In the first loop iteration, `sensitivity` will be
> an empty string, and so the hashtab_search() should return NULL,
> leading to -EINVAL. Am I missing something?

Looking at this again, no, I think you've got it right.  My guess is
that I just mistook the NULL sensitivity check at the top of the loop
as getting triggered in this case, which isn't the case here.  Sorry
for the noise.

> > As an aside, I believe my other comments on this patch still stand.
> > It's a nice improvement but I think there are some other small things
> > that need to be addressed.
>
> Is there anything I need to fix apart from the overly verbose comment
> and the unnecessary curly braces?

Nope.  I wouldn't even bother with that brace/comment changes, those
were minor nits and only worth changing if you needed to respin the
patch for some other reason.

Consider the patch merged, thanks!

--
paul moore
www.paul-moore.com
diff mbox series

Patch

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 39475fb455bc..2fe459df3c85 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -218,9 +218,7 @@  int mls_context_isvalid(struct policydb *p, struct context *c)
 /*
  * Set the MLS fields in the security context structure
  * `context' based on the string representation in
- * the string `*scontext'.  Update `*scontext' to
- * point to the end of the string representation of
- * the MLS fields.
+ * the string `scontext'.
  *
  * This function modifies the string in place, inserting
  * NULL characters to terminate the MLS fields.
@@ -235,22 +233,21 @@  int mls_context_isvalid(struct policydb *p, struct context *c)
  */
 int mls_context_to_sid(struct policydb *pol,
 		       char oldc,
-		       char **scontext,
+		       char *scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid)
 {
-
-	char delim;
-	char *scontextp, *p, *rngptr;
+	char *sensitivity, *cur_cat, *next_cat, *rngptr;
 	struct level_datum *levdatum;
 	struct cat_datum *catdatum, *rngdatum;
-	int l, rc = -EINVAL;
+	int l, rc, i;
+	char *rangep[2];
 
 	if (!pol->mls_enabled) {
-		if (def_sid != SECSID_NULL && oldc)
-			*scontext += strlen(*scontext) + 1;
-		return 0;
+		if ((def_sid != SECSID_NULL && oldc) || (*scontext) == '\0')
+			return 0;
+		return -EINVAL;
 	}
 
 	/*
@@ -261,113 +258,94 @@  int mls_context_to_sid(struct policydb *pol,
 		struct context *defcon;
 
 		if (def_sid == SECSID_NULL)
-			goto out;
+			return -EINVAL;
 
 		defcon = sidtab_search(s, def_sid);
 		if (!defcon)
-			goto out;
+			return -EINVAL;
 
-		rc = mls_context_cpy(context, defcon);
-		goto out;
+		return mls_context_cpy(context, defcon);
 	}
 
-	/* Extract low sensitivity. */
-	scontextp = p = *scontext;
-	while (*p && *p != ':' && *p != '-')
-		p++;
-
-	delim = *p;
-	if (delim != '\0')
-		*p++ = '\0';
+	/*
+	 * If we're dealing with a range, figure out where the two parts
+	 * of the range begin.
+	 */
+	rangep[0] = scontext;
+	rangep[1] = strchr(scontext, '-');
+	if (rangep[1]) {
+		rangep[1][0] = '\0';
+		rangep[1]++;
+	}
 
+	/* For each part of the range: */
 	for (l = 0; l < 2; l++) {
-		levdatum = hashtab_search(pol->p_levels.table, scontextp);
-		if (!levdatum) {
-			rc = -EINVAL;
-			goto out;
-		}
+		/* Split sensitivity and category set. */
+		sensitivity = rangep[l];
+		if (sensitivity == NULL)
+			break;
+		next_cat = strchr(sensitivity, ':');
+		if (next_cat)
+			*(next_cat++) = '\0';
 
+		/* Parse sensitivity. */
+		levdatum = hashtab_search(pol->p_levels.table, sensitivity);
+		if (!levdatum)
+			return -EINVAL;
 		context->range.level[l].sens = levdatum->level->sens;
 
-		if (delim == ':') {
-			/* Extract category set. */
-			while (1) {
-				scontextp = p;
-				while (*p && *p != ',' && *p != '-')
-					p++;
-				delim = *p;
-				if (delim != '\0')
-					*p++ = '\0';
-
-				/* Separate into range if exists */
-				rngptr = strchr(scontextp, '.');
-				if (rngptr != NULL) {
-					/* Remove '.' */
-					*rngptr++ = '\0';
-				}
+		/* Extract category set. */
+		while (next_cat != NULL) {
+			cur_cat = next_cat;
+			next_cat = strchr(next_cat, ',');
+			if (next_cat != NULL)
+				*(next_cat++) = '\0';
+
+			/* Separate into range if exists */
+			rngptr = strchr(cur_cat, '.');
+			if (rngptr != NULL) {
+				/* Remove '.' */
+				*rngptr++ = '\0';
+			}
 
-				catdatum = hashtab_search(pol->p_cats.table,
-							  scontextp);
-				if (!catdatum) {
-					rc = -EINVAL;
-					goto out;
-				}
+			catdatum = hashtab_search(pol->p_cats.table, cur_cat);
+			if (!catdatum)
+				return -EINVAL;
 
-				rc = ebitmap_set_bit(&context->range.level[l].cat,
-						     catdatum->value - 1, 1);
-				if (rc)
-					goto out;
-
-				/* If range, set all categories in range */
-				if (rngptr) {
-					int i;
-
-					rngdatum = hashtab_search(pol->p_cats.table, rngptr);
-					if (!rngdatum) {
-						rc = -EINVAL;
-						goto out;
-					}
-
-					if (catdatum->value >= rngdatum->value) {
-						rc = -EINVAL;
-						goto out;
-					}
-
-					for (i = catdatum->value; i < rngdatum->value; i++) {
-						rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
-						if (rc)
-							goto out;
-					}
-				}
+			rc = ebitmap_set_bit(&context->range.level[l].cat,
+					     catdatum->value - 1, 1);
+			if (rc)
+				return rc;
+
+			/* If range, set all categories in range */
+			if (rngptr == NULL)
+				continue;
+
+			rngdatum = hashtab_search(pol->p_cats.table, rngptr);
+			if (!rngdatum)
+				return -EINVAL;
+
+			if (catdatum->value >= rngdatum->value)
+				return -EINVAL;
 
-				if (delim != ',')
-					break;
+			for (i = catdatum->value; i < rngdatum->value; i++) {
+				rc = ebitmap_set_bit(&context->range.level[l].cat, i, 1);
+				if (rc)
+					return rc;
 			}
 		}
-		if (delim == '-') {
-			/* Extract high sensitivity. */
-			scontextp = p;
-			while (*p && *p != ':')
-				p++;
-
-			delim = *p;
-			if (delim != '\0')
-				*p++ = '\0';
-		} else
-			break;
 	}
 
-	if (l == 0) {
+	/* If we didn't see a '-', the range start is also the range end. */
+	if (rangep[1] == NULL) {
 		context->range.level[1].sens = context->range.level[0].sens;
 		rc = ebitmap_cpy(&context->range.level[1].cat,
 				 &context->range.level[0].cat);
 		if (rc)
-			goto out;
+			return rc;
 	}
-	*scontext = ++p;
-	rc = 0;
-out:
-	return rc;
+
+	return 0;
 }
 
 /*
@@ -379,21 +357,19 @@  int mls_context_to_sid(struct policydb *pol,
 int mls_from_string(struct policydb *p, char *str, struct context *context,
 		    gfp_t gfp_mask)
 {
-	char *tmpstr, *freestr;
+	char *tmpstr;
 	int rc;
 
 	if (!p->mls_enabled)
 		return -EINVAL;
 
-	/* we need freestr because mls_context_to_sid will change
-	   the value of tmpstr */
-	tmpstr = freestr = kstrdup(str, gfp_mask);
+	tmpstr = kstrdup(str, gfp_mask);
 	if (!tmpstr) {
 		rc = -ENOMEM;
 	} else {
-		rc = mls_context_to_sid(p, ':', &tmpstr, context,
+		rc = mls_context_to_sid(p, ':', tmpstr, context,
 					NULL, SECSID_NULL);
-		kfree(freestr);
+		kfree(tmpstr);
 	}
 
 	return rc;
diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h
index 9a3ff7af70ad..67093647576d 100644
--- a/security/selinux/ss/mls.h
+++ b/security/selinux/ss/mls.h
@@ -34,7 +34,7 @@  int mls_level_isvalid(struct policydb *p, struct mls_level *l);
 
 int mls_context_to_sid(struct policydb *p,
 		       char oldc,
-		       char **scontext,
+		       char *scontext,
 		       struct context *context,
 		       struct sidtab *s,
 		       u32 def_sid);
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index dd2ceec06fef..9212d4dd817a 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -1367,7 +1367,6 @@  int security_sid_to_context_force(struct selinux_state *state, u32 sid,
 static int string_to_context_struct(struct policydb *pol,
 				    struct sidtab *sidtabp,
 				    char *scontext,
-				    u32 scontext_len,
 				    struct context *ctx,
 				    u32 def_sid)
 {
@@ -1428,15 +1427,12 @@  static int string_to_context_struct(struct policydb *pol,
 
 	ctx->type = typdatum->value;
 
-	rc = mls_context_to_sid(pol, oldc, &p, ctx, sidtabp, def_sid);
+	rc = mls_context_to_sid(pol, oldc, p, ctx, sidtabp, def_sid);
 	if (rc)
 		goto out;
 
-	rc = -EINVAL;
-	if ((p - scontext) < scontext_len)
-		goto out;
-
 	/* Check the validity of the new context. */
+	rc = -EINVAL;
 	if (!policydb_context_isvalid(pol, ctx))
 		goto out;
 	rc = 0;
@@ -1491,7 +1487,7 @@  static int security_context_to_sid_core(struct selinux_state *state,
 	policydb = &state->ss->policydb;
 	sidtab = &state->ss->sidtab;
 	rc = string_to_context_struct(policydb, sidtab, scontext2,
-				      scontext_len, &context, def_sid);
+				      &context, def_sid);
 	if (rc == -EINVAL && force) {
 		context.str = str;
 		context.len = strlen(str) + 1;
@@ -1959,7 +1955,7 @@  static int convert_context(u32 key,
 			goto out;
 
 		rc = string_to_context_struct(args->newp, NULL, s,
-					      c->len, &ctx, SECSID_NULL);
+					      &ctx, SECSID_NULL);
 		kfree(s);
 		if (!rc) {
 			printk(KERN_INFO "SELinux:  Context %s became valid (mapped).\n",