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> 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>
> 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.
<snip>
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.
--
paul moore
www.paul-moore.com