Message-ID: <20210522125859.GF447005@madcap2.tricolour.ca>
Date:   Sat, 22 May 2021 08:58:59 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Casey Schaufler <casey@...aufler-ca.com>,
        john.johansen@...onical.com, selinux@...r.kernel.org,
        netdev@...r.kernel.org, James Morris <jmorris@...ei.org>,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-audit@...hat.com,
        casey.schaufler@...el.com, Stephen Smalley <sds@...ho.nsa.gov>
Subject: Re: [PATCH v26 22/25] Audit: Add new record for multiple process LSM
 attributes

On 2021-05-21 22:20, Paul Moore wrote:
> On Fri, May 21, 2021 at 6:05 PM Casey Schaufler <casey@...aufler-ca.com> wrote:
> > On 5/21/2021 1:19 PM, Paul Moore wrote:
> > > On Thu, May 13, 2021 at 4:32 PM Casey Schaufler <casey@...aufler-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.
> 
> 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@...aufler-ca.com>
> > >> To: paul@...l-moore.com
> > >> To: linux-audit@...hat.com
> > >> To: rgb@...hat.com
> > >> Cc: netdev@...r.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 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@...hat.com
> https://listman.redhat.com/mailman/listinfo/linux-audit

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
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

Powered by blists - more mailing lists