On Fri, May 21, 2021 at 6:05 PM Casey Schaufler [off-list ref] wrote:

On 5/21/2021 1:19 PM, Paul Moore wrote:

quoted

On Thu, May 13, 2021 at 4:32 PM Casey Schaufler [off-list ref] wrote:

quoted

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.

quoted

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.

quoted

quoted

        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@schaufler-ca.com>
To: paul@paul-moore.com
To: linux-audit@redhat.com
To: rgb@redhat.com
Cc: netdev@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(-)

...

quoted

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.

quoted

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