Re: [PATCH ghak57 V1] selinux: format all invalid context as untrusted

[Paul Moore]

On Fri, Jun 14, 2019 at 4:05 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs <rgb@xxxxxxxxxx> wrote:

...

> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index cc043bc8fd4c..817576802f7d 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -1588,6 +1588,8 @@ static int compute_sid_handle_invalid_context(
> >         struct policydb *policydb = &state->ss->policydb;
> >         char *s = NULL, *t = NULL, *n = NULL;
> >         u32 slen, tlen, nlen;
> > +       struct audit_buffer *ab;
> > +       size_t audit_size;
> >
> >         if (context_struct_to_string(policydb, scontext, &s, &slen))
> >                 goto out;
> > @@ -1595,12 +1597,22 @@ static int compute_sid_handle_invalid_context(
> >                 goto out;
> >         if (context_struct_to_string(policydb, newcontext, &n, &nlen))
> >                 goto out;
> > -       audit_log(audit_context(), GFP_ATOMIC, AUDIT_SELINUX_ERR,
> > -                 "op=security_compute_sid invalid_context=%s"
> > -                 " scontext=%s"
> > -                 " tcontext=%s"
> > -                 " tclass=%s",
> > -                 n, s, t, sym_name(policydb, SYM_CLASSES, tclass-1));
> > +       /* We strip a nul only if it is at the end, otherwise the
> > +        * context contains a nul and we should audit that */
> > +       if (n) {
> > +               if (n[nlen - 1] == '\0')
> > +                       audit_size = nlen - 1;
> > +               else
> > +                       audit_size = nlen;
> > +       } else {
> > +               audit_size = 0;
> > +       }
>
> If you reasonably assume that (n == NULL) implies (nlen == 0), then
> you can simplify this down to:
>
>     audit_size = nlen;
>     if (nlen && n[nlen - 1] == '\0')
>         audit_size--;
>
> (or similar), see my recent patch to log *rawcon as untrusted [2].
> That is IMHO faster to parse. But I see you copied it from
> selinux_inode_setxattr(), where it is like this...

You could likely simplify this even further by getting rid of
audit_size and just using nlen; there is no reason why we need to
preserve the original nlen value in this function.  Also, keep in mind
that if you are hitting that chunk of code, and not jumping to "out"
due to a context_struct_to_string() error, then you should have a
properly formatted SELinux label, it just happens to be invalid for
the currently loaded policy.  Something like the following should be
safe:

  if (n[nlen - 1] == '\0')
    nlen--;
  audit_log_start(...);
  audit_log_format("... invalid_context=");
  audit_log_n_untrustedstring(n, nlen);
  audit_log_format(...);
  audit_log_end(...);

Also, to be honest, the string you get back from
context_struct_to_string() is always going to be NUL-terminated so you
could simplify this further:

  audit_log_start(...);
  audit_log_format("... invalid_context=");
  /* no need to record the NUL with untrusted strings */
  audit_log_n_untrustedstring(n, nlen - 1);
  audit_log_format(...);
  audit_log_end(...);

> I'm not sure if it
> is worth changing this patch / consolidating the style across all
> places that do this / creating a helper function...

If anyone is going to look into that, it should be done in a separate patch.

-- 
paul moore
www.paul-moore.com