On 2021-05-21 22:20, Paul Moore wrote: > On Fri, May 21, 2021 at 6:05 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > > On 5/21/2021 1:19 PM, Paul Moore wrote: > > > On Thu, May 13, 2021 at 4:32 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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. > > Oh, wow, okay. That needs to change at some point but I agree it's > out of scope for this patchset. In the meantime I might suggest using > something other than AppArmor as an example here. Similar but not identical situation to: BUG: INTEGRITY_POLICY_RULE violates audit message format · Issue #113 · linux-audit/audit-kernel https://github.com/linux-audit/audit-kernel/issues/113 > > >> 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@xxxxxxxxxxxxxxxx> > > >> To: paul@xxxxxxxxxxxxxx > > >> To: linux-audit@xxxxxxxxxx > > >> To: rgb@xxxxxxxxxx > > >> Cc: netdev@xxxxxxxxxxxxxxx > > >> --- > > >> 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 can't think of a case where you would ever not know if you need to > allocate a local context at the start. If you are unsure, get in > touch and we can work it out. > > > > 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. > > I appreciate the desire to move forward, and while I can't speak for > everyone, I'll do my best to work with you to find a good solution. > If you get stuck or aren't sure you know how to reach me :) > > As a start, I might suggest looking at some of the recent audit > container ID patchsets from Richard; while they have had some issues, > they should serve as a basic example of what we mean when we talk > about "local contexts" and how they should be used. > > -- > paul moore > www.paul-moore.com > > -- > Linux-audit mailing list > Linux-audit@xxxxxxxxxx > https://listman.redhat.com/mailman/listinfo/linux-audit - RGB -- Richard Guy Briggs <rgb@xxxxxxxxxx> Sr. S/W Engineer, Kernel Security, Base Operating Systems Remote, Ottawa, Red Hat Canada IRC: rgb, SunRaycer Voice: +1.647.777.2635, Internal: (81) 32635
