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.
> >> 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
