Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM attributes

On 5/21/2021 1:19 PM, Paul Moore wrote: > On Thu, May 13, 2021 at 4:32 PM Casey Schaufler <casey(a)schaufler-ca.com&gt; wrote: >> Create a new audit record type to contain the subject information >> when there are multiple security modules that require such data. >> This record is linked with the same timestamp and serial number >> using the audit_alloc_local() mechanism. > The record is linked with the other associated records into a single > event, it doesn't matter if it gets the timestamp/serial from > audit_alloc_local() or an existing audit event, e.g. ongoing syscall. > >> The record is produced only in cases where there is more than one >> security module with a process "context". >> In cases where this record is produced the subj= fields of >> other records in the audit event will be set to "subj=?". >> >> An example of the MAC_TASK_CONTEXTS (1420) record is: >> >> type=UNKNOWN[1420] >> msg=audit(1600880931.832:113) >> subj_apparmor==unconfined > It should be just a single "=" in the line above. AppArmor provides the 2nd "=" as part of the subject context. What's here is correct. I won't argue that it won't case confusion or worse.

>> subj_smack=_ >> >> There will be a subj_$LSM= entry for each security module >> LSM that supports the secid_to_secctx and secctx_to_secid >> hooks. The BPF security module implements secid/secctx >> translation hooks, so it has to be considered to provide a >> secctx even though it may not actually do so. >> >> Signed-off-by: Casey Schaufler <casey(a)schaufler-ca.com&gt; >> To: paul(a)paul-moore.com >> To: linux-audit(a)redhat.com >> To: rgb(a)redhat.com >> Cc: netdev(a)vger.kernel.org >> --- >> drivers/android/binder.c | 2 +- >> include/linux/audit.h | 24 ++++++++ >> include/linux/security.h | 16 ++++- >> include/net/netlabel.h | 3 +- >> include/net/scm.h | 2 +- >> include/net/xfrm.h | 13 +++- >> include/uapi/linux/audit.h | 1 + >> kernel/audit.c | 80 ++++++++++++++++++------- >> kernel/audit.h | 3 + >> kernel/auditfilter.c | 6 +- >> kernel/auditsc.c | 75 ++++++++++++++++++++--- >> 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_domainhash.c | 4 +- >> net/netlabel/netlabel_unlabeled.c | 24 ++++---- >> net/netlabel/netlabel_user.c | 20 ++++--- >> net/netlabel/netlabel_user.h | 6 +- >> net/xfrm/xfrm_policy.c | 10 ++-- >> net/xfrm/xfrm_state.c | 20 ++++--- >> security/integrity/ima/ima_api.c | 7 ++- >> security/integrity/integrity_audit.c | 6 +- >> security/security.c | 46 +++++++++----- >> security/smack/smackfs.c | 3 +- >> 25 files changed, 274 insertions(+), 107 deletions(-) > ... > >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> index 97cd7471e572..229cd71fbf09 100644 >> --- a/include/linux/audit.h >> +++ b/include/linux/audit.h >> @@ -386,6 +395,19 @@ static inline void audit_ptrace(struct task_struct *t) >> __audit_ptrace(t); >> } >> >> +static inline struct audit_context *audit_alloc_for_lsm(gfp_t gfp) >> +{ >> + struct audit_context *context = audit_context(); >> + >> + if (context) >> + return context; >> + >> + if (lsm_multiple_contexts()) >> + return audit_alloc_local(gfp); >> + >> + return NULL; >> +} > See my other comments, but this seems wrong at face value. The > additional LSM record should happen as part of the existing audit log > functions. I'm good with that. But if you defer calling audit_alloc_local() until you know you need it you may be in a place where you can't associate the new context with the event. I think. I will have another go at it.

> I think I was distracted with the local context issue and I've lost > track of the details here, perhaps it's best to fix the local context > issue first (that should be a big change to this patch) and then we > can take another look. I really need to move forward. I'll give allocation of local contexts as necessary in audit_log_task_context() another shot.