[PATCH] selinux: stricter parsing in mls_context_to_sid()

[PATCH] selinux: stricter parsing in mls_context_to_sid()

[Jann Horn]

mls_context_to_sid incorrectly accepted MLS context strings that are
followed by a dash and trailing garbage.

Before this change, the following command works:

# mount -t tmpfs -o 'context=system_u:object_r:tmp_t:s0-s0:c0-BLAH' \
none mount

After this change, it fails with the following error message in dmesg:

SELinux: security_context_str_to_sid(system_u:object_r:tmp_t:s0-s0:c0-BLAH)
failed for (dev tmpfs, type tmpfs) errno=-22

This is not an important bug; but it is a small quirk that was useful for
exploiting a vulnerability in fusermount.

This patch does not change the behavior when the policy does not have MLS
enabled.

Signed-off-by: Jann Horn <jannh@google.com>
---
 security/selinux/ss/mls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
index 39475fb455bc..2c73d612d2ee 100644
--- a/security/selinux/ss/mls.c
+++ b/security/selinux/ss/mls.c
@@ -344,7 +344,7 @@ int mls_context_to_sid(struct policydb *pol,
 					break;
 			}
 		}
-		if (delim == '-') {
+		if (delim == '-' && l == 0) {
 			/* Extract high sensitivity. */
 			scontextp = p;
 			while (*p && *p != ':')
-- 
2.18.0.597.ga71716f1ad-goog

Re: [PATCH] selinux: stricter parsing in mls_context_to_sid()

[Paul Moore]

On Fri, Aug 3, 2018 at 5:36 AM Jann Horn <jannh@google.com> wrote:
>
> mls_context_to_sid incorrectly accepted MLS context strings that are
> followed by a dash and trailing garbage.
>
> Before this change, the following command works:
>
> # mount -t tmpfs -o 'context=system_u:object_r:tmp_t:s0-s0:c0-BLAH' \
> none mount
>
> After this change, it fails with the following error message in dmesg:
>
> SELinux: security_context_str_to_sid(system_u:object_r:tmp_t:s0-s0:c0-BLAH)
> failed for (dev tmpfs, type tmpfs) errno=-22
>
> This is not an important bug; but it is a small quirk that was useful for
> exploiting a vulnerability in fusermount.
>
> This patch does not change the behavior when the policy does not have MLS
> enabled.
>
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
>  security/selinux/ss/mls.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Ooof.  mls_context_to_sid() isn't exactly the most elegant function is
it?  I suppose some of that is due to the label format, but it still
seems like we can do better.  However, before I jump into that let me
say that speaking strictly about your patch, yes, it does look correct
to me.

What I'm wonder is if we rework/reorder some of the parser to remove
some of the ordering specific (e.g. "l == 0") and reduce some
redundancy ... be patient with me for a moment ...

The while loops immediately following the "Extract low sensitivity"
and "Extract high sensitivity" comments are basically the same,
including NULL'ing the delimiter if necessary, the only difference is
the additional '-' delimiter in the low sensitivity search.

The only *legal* place for a '-' in the MLS portion of the label is to
separate the low/high portions.

What if we were to do a quick search for the low/high separator before
extracting the low sensitivity and stored low/high pointers for later
use?  e.g.

  rangep[0] = *scontext;
  rangep[1] = strchr(rangep[0], '-');
  if (rangep[1])
    rangep[1]++ = '\0';

... we could then move the "Extract X sensitivity" into the main for
loop as well remove all of the '-' special case parsing checks, e.g.

  for (l = 0; l < 2; l++) {

    scontextp = rangep[l];
    if (!scontextp)
      break;

    while (*p && *p != ':')
      p++;
    delim = *p;
    if (delim != '\0')
      *p++ = '\0';

    /* extract the level (use existing code) */

    /* extract the category set, if present (use existing code) */

    /* no need to worry about the '-' delimiter */

  }

I *believe* that should work.  I think.  Does that make sense?

Yes, I do understand that this likely isn't quite as performant as the
existing approach due to that initial strchr(), but I think the code
will be much cleaner and less fragile.  If we feel the need to claw
back some performance there are some other things in here would could
probably work on (e.g. imagine an ebitmap_set_range()).

> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 39475fb455bc..2c73d612d2ee 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -344,7 +344,7 @@ int mls_context_to_sid(struct policydb *pol,
>                                         break;
>                         }
>                 }
> -               if (delim == '-') {
> +               if (delim == '-' && l == 0) {
>                         /* Extract high sensitivity. */
>                         scontextp = p;
>                         while (*p && *p != ':')
> --
> 2.18.0.597.ga71716f1ad-goog
>

--
paul moore
www.paul-moore.com

Re: [PATCH] selinux: stricter parsing in mls_context_to_sid()

[Stephen Smalley]

On 08/03/2018 05:36 AM, Jann Horn wrote:
> mls_context_to_sid incorrectly accepted MLS context strings that are
> followed by a dash and trailing garbage.
> 
> Before this change, the following command works:
> 
> # mount -t tmpfs -o 'context=system_u:object_r:tmp_t:s0-s0:c0-BLAH' \
> none mount
> 
> After this change, it fails with the following error message in dmesg:
> 
> SELinux: security_context_str_to_sid(system_u:object_r:tmp_t:s0-s0:c0-BLAH)
> failed for (dev tmpfs, type tmpfs) errno=-22
> 
> This is not an important bug; but it is a small quirk that was useful for
> exploiting a vulnerability in fusermount.
> 
> This patch does not change the behavior when the policy does not have MLS
> enabled.
> 
> Signed-off-by: Jann Horn <jannh@google.com>

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

> ---
>   security/selinux/ss/mls.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c
> index 39475fb455bc..2c73d612d2ee 100644
> --- a/security/selinux/ss/mls.c
> +++ b/security/selinux/ss/mls.c
> @@ -344,7 +344,7 @@ int mls_context_to_sid(struct policydb *pol,
>   					break;
>   			}
>   		}
> -		if (delim == '-') {
> +		if (delim == '-' && l == 0) {
>   			/* Extract high sensitivity. */
>   			scontextp = p;
>   			while (*p && *p != ':')
> 

Re: [PATCH] selinux: stricter parsing in mls_context_to_sid()

[Jann Horn]

On Sat, Aug 4, 2018 at 2:01 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Aug 3, 2018 at 5:36 AM Jann Horn <jannh@google.com> wrote:
> >
> > mls_context_to_sid incorrectly accepted MLS context strings that are
> > followed by a dash and trailing garbage.
> >
> > Before this change, the following command works:
> >
> > # mount -t tmpfs -o 'context=system_u:object_r:tmp_t:s0-s0:c0-BLAH' \
> > none mount
> >
> > After this change, it fails with the following error message in dmesg:
> >
> > SELinux: security_context_str_to_sid(system_u:object_r:tmp_t:s0-s0:c0-BLAH)
> > failed for (dev tmpfs, type tmpfs) errno=-22
> >
> > This is not an important bug; but it is a small quirk that was useful for
> > exploiting a vulnerability in fusermount.
> >
> > This patch does not change the behavior when the policy does not have MLS
> > enabled.
> >
> > Signed-off-by: Jann Horn <jannh@google.com>
> > ---
> >  security/selinux/ss/mls.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Ooof.  mls_context_to_sid() isn't exactly the most elegant function is
> it?  I suppose some of that is due to the label format, but it still
> seems like we can do better.  However, before I jump into that let me
> say that speaking strictly about your patch, yes, it does look correct
> to me.
>
> What I'm wonder is if we rework/reorder some of the parser to remove
> some of the ordering specific (e.g. "l == 0") and reduce some
> redundancy ... be patient with me for a moment ...
>
> The while loops immediately following the "Extract low sensitivity"
> and "Extract high sensitivity" comments are basically the same,
> including NULL'ing the delimiter if necessary, the only difference is
> the additional '-' delimiter in the low sensitivity search.
>
> The only *legal* place for a '-' in the MLS portion of the label is to
> separate the low/high portions.
>
> What if we were to do a quick search for the low/high separator before
> extracting the low sensitivity and stored low/high pointers for later
> use?  e.g.
>
>   rangep[0] = *scontext;
>   rangep[1] = strchr(rangep[0], '-');
>   if (rangep[1])
>     rangep[1]++ = '\0';
>
> ... we could then move the "Extract X sensitivity" into the main for
> loop as well remove all of the '-' special case parsing checks, e.g.
>
>   for (l = 0; l < 2; l++) {
>
>     scontextp = rangep[l];
>     if (!scontextp)
>       break;
>
>     while (*p && *p != ':')
>       p++;
>     delim = *p;
>     if (delim != '\0')
>       *p++ = '\0';
>
>     /* extract the level (use existing code) */
>
>     /* extract the category set, if present (use existing code) */
>
>     /* no need to worry about the '-' delimiter */
>
>   }
>
> I *believe* that should work.  I think.  Does that make sense?

I'm fiddling with the code a bit now - your suggestion sounds good to
me, and I think there are a couple other small tweaks that can make
the code more readable. I hope I'll have a patch ready soon.