Re: [PATCH v15 20/23] Audit: Add subj_LSM fields when necessary

On 3/6/2020 6:24 PM, Paul Moore wrote: > On Fri, Feb 21, 2020 at 7:06 PM Casey Schaufler <casey(a)schaufler-ca.com&gt; wrote: >> Add record entries to identify subject data for all of the >> security modules when there is more than one. >> >> Acked-by: Stephen Smalley <sds(a)tycho.nsa.gov&gt; >> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; >> Cc: netdev(a)vger.kernel.org >> Cc: linux-audit(a)redhat.com >> --- >> drivers/android/binder.c | 2 +- >> include/linux/audit.h | 1 + >> include/linux/security.h | 9 ++++- >> include/net/scm.h | 3 +- >> kernel/audit.c | 40 ++++++++++++++++++- >> kernel/audit_fsnotify.c | 1 + >> kernel/auditfilter.c | 1 + >> kernel/auditsc.c | 10 +++-- >> net/ipv4/ip_sockglue.c | 2 +- >> net/netfilter/nf_conntrack_netlink.c | 4 +- >> net/netfilter/nf_conntrack_standalone.c | 2 +- >> net/netfilter/nfnetlink_queue.c | 2 +- >> net/netlabel/netlabel_unlabeled.c | 11 ++++-- >> net/netlabel/netlabel_user.c | 2 +- >> net/xfrm/xfrm_policy.c | 2 + >> net/xfrm/xfrm_state.c | 2 + >> security/integrity/ima/ima_api.c | 1 + >> security/integrity/integrity_audit.c | 1 + >> security/security.c | 51 +++++++++++++++++++++++-- >> 19 files changed, 124 insertions(+), 23 deletions(-) > ... > >> diff --git a/kernel/audit.c b/kernel/audit.c >> index a25097cfe623..c3a1d8d2d33c 100644 >> --- a/kernel/audit.c >> +++ b/kernel/audit.c >> @@ -2054,6 +2061,33 @@ void audit_log_key(struct audit_buffer *ab, char *key) >> audit_log_format(ab, "(null)"); >> } >> >> +void audit_log_task_lsms(struct audit_buffer *ab) >> +{ >> + int i; >> + const char *lsm; >> + struct lsmblob blob; >> + struct lsmcontext context; >> + >> + /* >> + * Don't do anything unless there is more than one LSM >> + * with a security context to report. >> + */ >> + if (security_lsm_slot_name(1) == NULL) >> + return; >> + >> + security_task_getsecid(current, &blob); >> + >> + for (i = 0; i < LSMBLOB_ENTRIES; i++) { >> + lsm = security_lsm_slot_name(i); >> + if (lsm == NULL) >> + break; >> + if (security_secid_to_secctx(&blob, &context, i)) >> + continue; >> + audit_log_format(ab, " subj_%s=%s", lsm, context.context); >> + security_release_secctx(&context); >> + } >> +} >> + >> int audit_log_task_context(struct audit_buffer *ab) >> { >> int error; >> @@ -2064,7 +2098,7 @@ int audit_log_task_context(struct audit_buffer *ab) >> if (!lsmblob_is_set(&blob)) >> return 0; >> >> - error = security_secid_to_secctx(&blob, &context); >> + error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST); >> if (error) { >> if (error != -EINVAL) >> goto error_path; > Sorry, please disregard my previous ACK. :( > We should treat "subj=" similar to how we treat "obj="; if there is > more than one LSM loaded the "subj=" should be set to "?" with the > "subj_XXX=" set to the appropriate label for the named LSM. This > patch looks like it is always using LSMBLOB_FIRST and not "?" when > multiple LSMs are present. I'm fine with that, although I could see someone suggesting that would constitute breaking backward compatibility.