Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Fri, Aug 31, 2018 at 11:47 AM Jann Horn wrote: > On Thu, Aug 9, 2018 at 3:56 AM Paul Moore wrote: > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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 ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Thu, Aug 9, 2018 at 3:56 AM Paul Moore wrote:
>
> On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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;
> >
> > -
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Fri, Aug 10, 2018 at 7:01 PM Jann Horn wrote:
> On Thu, Aug 9, 2018 at 4:07 AM Paul Moore wrote:
> > On Wed, Aug 8, 2018 at 9:56 PM Paul Moore wrote:
> > > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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
> > > > ---
> > > > 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()
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Thu, Aug 9, 2018 at 4:07 AM Paul Moore wrote:
>
> On Wed, Aug 8, 2018 at 9:56 PM Paul Moore wrote:
> >
> > On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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
> > > ---
> > > 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, ,
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Wed, Aug 8, 2018 at 9:56 PM Paul Moore wrote:
>
> On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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
> > ---
> > 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);
> > }
> >
> > -
Re: [PATCH] selinux: refactor mls_context_to_sid() and make it stricter
On Mon, Aug 6, 2018 at 5:19 PM Jann Horn 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
> ---
> 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);
> -
[PATCH] selinux: refactor mls_context_to_sid() and make it stricter
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
---
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(-)
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
