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

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

[Richard Guy Briggs]

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

Signed-off-by: Richard Guy Briggs <rgb@redhat.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;
+	}
+	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;
+				}
+				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

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

[Ondrej Mosnacek]

On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs <rgb@redhat.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@redhat.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.

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@redhat.com> wrote:
> On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs <rgb@redhat.com> 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

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

[Paul Moore]

On Thu, Jun 13, 2019 at 2:43 PM Richard Guy Briggs <rgb@redhat.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

It would be good to see a list of all the places we are using the
"invalid_context" field and some discussion about if those labels are
really "trusted" or "untrusted".  In both the
compute_sid_handle_invalid_context() and security_sid_mls_copy() cases
below it would appear that the labels can be considered "trusted",
even if they are invalid.  I understand your concern about logging
consistency with the "invalid_context" field, but without some further
discussion it is hard to accept this patch as-is.

-- 
paul moore
www.paul-moore.com