Message-ID: <CAFqZXNvTAj_MhgbUB0kbQwF+gDQTTO5jXPagQfW9qwfHEzc1iQ@mail.gmail.com>
Date:   Fri, 14 Jun 2019 10:04:54 +0200
From:   Ondrej Mosnacek <omosnace@...hat.com>
To:     Richard Guy Briggs <rgb@...hat.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        SElinux list <selinux@...r.kernel.org>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...hat.com>, Steve Grubb <sgrubb@...hat.com>
Subject: Re: [PATCH ghak57 V1] selinux: format all invalid context as untrusted

On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> All instances of one field type should be encoded in the same way.
> Since some invalid_context fields can contain untrusted strings, encode
> all instances of this field the same way.
>
> Please see github issue
> https://github.com/linux-audit/audit-kernel/issues/57

I would add at least a short explanation ("The userspace tools expects
all fields of the same name to be logged consistently.") directly to
the commit message. Funny that Paul warned about this on the SELinux
ML just yesterday :) [1]

>
> Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> ---
> Passes audit-testsuite.
>
>  security/selinux/ss/services.c | 48 +++++++++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 10 deletions(-)
>
> 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... I'm not sure if it
is worth changing this patch / consolidating the style across all
places that do this / creating a helper function...

> +       ab = audit_log_start(audit_context(), GFP_ATOMIC, AUDIT_SELINUX_ERR);
> +       audit_log_format(ab, "op=security_compute_sid invalid_context=");
> +       audit_log_n_untrustedstring(ab, n, audit_size);
> +       audit_log_format(ab, " scontext=%s tcontext=%s tclass=%s",
> +                        s, t, sym_name(policydb, SYM_CLASSES, tclass-1));
> +       audit_log_end(ab);
>  out:
>         kfree(s);
>         kfree(t);
> @@ -3007,10 +3019,26 @@ int security_sid_mls_copy(struct selinux_state *state,
>                 if (rc) {
>                         if (!context_struct_to_string(policydb, &newcon, &s,
>                                                       &len)) {
> -                               audit_log(audit_context(),
> -                                         GFP_ATOMIC, AUDIT_SELINUX_ERR,
> -                                         "op=security_sid_mls_copy "
> -                                         "invalid_context=%s", s);
> +                               struct audit_buffer *ab;
> +                               size_t audit_size;
> +
> +                               /* We strip a nul only if it is at the
> +                                * end, otherwise the context contains a
> +                                * nul and we should audit that */
> +                               if (s) {
> +                                       if (s[len - 1] == '\0')
> +                                               audit_size = len - 1;
> +                                       else
> +                                               audit_size = len;
> +                               } else {
> +                                       audit_size = 0;
> +                               }

analogically here ^^

[1] https://lore.kernel.org/selinux/CAHC9VhRwKjp9qwqMB3p7intdpxFT1oYVOUKmoEcwZcN1VAC-UA@mail.gmail.com/T/#m1515af36ca98dceddfb6c9f795e1dfa2ac6e8a1b
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git/commit/?h=stable-5.2&id=aff7ed4851680d0d28ad9f52cd2f99213e1371b2


> +                               ab = audit_log_start(audit_context(),
> +                                                    GFP_ATOMIC,
> +                                                    AUDIT_SELINUX_ERR);
> +                               audit_log_format(ab, "op=security_sid_mls_copy invalid_context=");
> +                               audit_log_n_untrustedstring(ab, s, audit_size);
> +                               audit_log_end(ab);
>                                 kfree(s);
>                         }
>                         goto out_unlock;
> --
> 1.8.3.1
>

--
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.

Powered by blists - more mailing lists