| Message ID | 53af233d05da5e07d75d122878387288a10276df.1560447640.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ghak57,V1] selinux: format all invalid context as untrusted | expand |
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.
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.
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.
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;
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(-)