Re: [PATCH ghak57 V1] selinux: format all invalid context as untrusted
On Fri, Jun 14, 2019 at 4:05 AM Ondrej Mosnacek wrote:
> On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs 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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH ghak57 V1] selinux: format all invalid context as untrusted
On Thu, Jun 13, 2019 at 2:43 PM Richard Guy Briggs 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
Re: [PATCH ghak57 V1] selinux: format all invalid context as untrusted
On Thu, Jun 13, 2019 at 8:43 PM Richard Guy Briggs 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
> ---
> 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...@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)
