Message-ID: <ace9d273-3560-3631-33fa-7421a165b038@schaufler-ca.com>
Date:   Thu, 12 Aug 2021 15:38:07 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     casey.schaufler@...el.com, James Morris <jmorris@...ei.org>,
        linux-security-module@...r.kernel.org, selinux@...r.kernel.org,
        linux-audit@...hat.com, keescook@...omium.org,
        john.johansen@...onical.com, penguin-kernel@...ove.sakura.ne.jp,
        Stephen Smalley <sds@...ho.nsa.gov>,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v28 22/25] Audit: Add record for multiple process LSM
 attributes

On 8/12/2021 1:59 PM, Paul Moore wrote:
> On Wed, Jul 21, 2021 at 9:12 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 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"
>>         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                   | 16 +++++
>>  include/linux/security.h                | 16 ++++-
>>  include/net/netlabel.h                  |  2 +-
>>  include/net/scm.h                       |  2 +-
>>  include/net/xfrm.h                      | 13 +++-
>>  include/uapi/linux/audit.h              |  1 +
>>  kernel/audit.c                          | 90 +++++++++++++++++++------
>>  kernel/auditfilter.c                    |  5 +-
>>  kernel/auditsc.c                        | 27 ++++++--
>>  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_unlabeled.c       | 21 +++---
>>  net/netlabel/netlabel_user.c            | 14 ++--
>>  net/netlabel/netlabel_user.h            |  6 +-
>>  net/xfrm/xfrm_policy.c                  |  8 ++-
>>  net/xfrm/xfrm_state.c                   | 18 +++--
>>  security/integrity/ima/ima_api.c        |  6 +-
>>  security/integrity/integrity_audit.c    |  5 +-
>>  security/security.c                     | 46 ++++++++-----
>>  security/smack/smackfs.c                |  3 +-
>>  23 files changed, 221 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/android/binder.c b/drivers/android/binder.c
>> index 2c3a2348a144..3520caa0260c 100644
>> --- a/drivers/android/binder.c
>> +++ b/drivers/android/binder.c
>> @@ -2722,7 +2722,7 @@ static void binder_transaction(struct binder_proc *proc,
>>                  * case well anyway.
>>                  */
>>                 security_task_getsecid_obj(proc->tsk, &blob);
>> -               ret = security_secid_to_secctx(&blob, &lsmctx);
>> +               ret = security_secid_to_secctx(&blob, &lsmctx, LSMBLOB_DISPLAY);
>>                 if (ret) {
>>                         return_error = BR_FAILED_REPLY;
>>                         return_error_param = ret;
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 97cd7471e572..85eb87f6f92d 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -291,6 +291,7 @@ extern int  audit_alloc(struct task_struct *task);
>>  extern void __audit_free(struct task_struct *task);
>>  extern struct audit_context *audit_alloc_local(gfp_t gfpflags);
>>  extern void audit_free_context(struct audit_context *context);
>> +extern void audit_free_local(struct audit_context *context);
>>  extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
>>                                   unsigned long a2, unsigned long a3);
>>  extern void __audit_syscall_exit(int ret_success, long ret_value);
>> @@ -386,6 +387,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;
>> +}
> We don't want to do this, at least not as it is written above.  We
> shouldn't have a function which abstracts away the creation of a local
> audit_context.  Usage of a local audit_context should be explicit in
> the caller, and if the caller's execution context is ambiguous enough
> that it can require both a task_struct audit_context and a local
> audit_context then we need to handle that on a case-by-case basis.
> Hiding it like this is guaranteed to cause problems later.

OK. Please understand that *every case* where I've used audit_alloc_for_lsm()
is a case where I have *verified* that context may be NULL. I will make
the change.

> I probably did a poor job of explaining what a local context is during
> the last patchset; I'll try to do a better job here but also let me
> say this as clear as I can ... if the "current" task struct is valid
> for a given code path, *never* use a local audit context.

I probably did a poor job of demonstrating that I never use a local
context where there's a valid current context.

>   The local
> audit context is a hack that is made necessary by the fact that we
> have to audit things which happen outside the scope of an executing
> task, e.g. the netfilter audit hooks, it should *never* be used when
> there is a valid task_struct.

In the existing audit code a "current context" is only needed for
syscall events, so that's the only case where it's allocated. Would
you suggest that I track down the non-syscall events that include
subj= fields and add allocate a "current context" for them? I looked
into doing that, and it wouldn't be simple.

> It's the audit_context which helps bind multiple audit records into a
> single event, creating a new, "local" audit_context destroys that
> binding

... if there's a current context. There often isn't. That's why I'm
using a local context. There is not a "current" context available.

>  as audit records created using that local audit_context have a
> different timestamp from those records created using the current
> task_struct's audit_context.

(Weeps) I only introduce a local context where there is no current
context available, so this is never a problem.

> Hopefully that makes sense?

Yes, it makes sense. Methinks you may believe that the current context
is available more regularly than it actually is.

I instrumented the audit event functions with:

	WARN_ONCE(audit_context, "%s has context\n", __func__);
	WARN_ONCE(!audit_context, "%s lacks context\n", __func__);

I only used local contexts where the 2nd WARN_ONCE was hit.


>>                                 /* Private API (for audit.c only) */
>>  extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
>>  extern void __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, umode_t mode);
>> @@ -560,6 +574,8 @@ extern int audit_signals;
>>  }
>>  static inline void audit_free_context(struct audit_context *context)
>>  { }
>> +static inline void audit_free_local(struct audit_context *context)
>> +{ }
>>  static inline int audit_alloc(struct task_struct *task)
>>  {
>>         return 0;
>> diff --git a/include/linux/security.h b/include/linux/security.h
>> index 3e9743118fb9..b3cf68cf2bd6 100644
>> --- a/include/linux/security.h
>> +++ b/include/linux/security.h
>> @@ -182,6 +182,8 @@ struct lsmblob {
>>  #define LSMBLOB_INVALID                -1      /* Not a valid LSM slot number */
>>  #define LSMBLOB_NEEDED         -2      /* Slot requested on initialization */
>>  #define LSMBLOB_NOT_NEEDED     -3      /* Slot not requested */
>> +#define LSMBLOB_DISPLAY                -4      /* Use the "display" slot */
>> +#define LSMBLOB_FIRST          -5      /* Use the default "display" slot */
>>
>>  /**
>>   * lsmblob_init - initialize an lsmblob structure
>> @@ -248,6 +250,15 @@ static inline u32 lsmblob_value(const struct lsmblob *blob)
>>         return 0;
>>  }
>>
>> +static inline bool lsm_multiple_contexts(void)
>> +{
>> +#ifdef CONFIG_SECURITY
>> +       return lsm_slot_to_name(1) != NULL;
>> +#else
>> +       return false;
>> +#endif
>> +}
>> +
>>  /* These functions are in security/commoncap.c */
>>  extern int cap_capable(const struct cred *cred, struct user_namespace *ns,
>>                        int cap, unsigned int opts);
>> @@ -578,7 +589,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>                          size_t size);
>>  int security_netlink_send(struct sock *sk, struct sk_buff *skb);
>>  int security_ismaclabel(const char *name);
>> -int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp);
>> +int security_secid_to_secctx(struct lsmblob *blob, struct lsmcontext *cp,
>> +                            int display);
>>  int security_secctx_to_secid(const char *secdata, u32 seclen,
>>                              struct lsmblob *blob);
>>  void security_release_secctx(struct lsmcontext *cp);
>> @@ -1433,7 +1445,7 @@ static inline int security_ismaclabel(const char *name)
>>  }
>>
>>  static inline int security_secid_to_secctx(struct lsmblob *blob,
>> -                                          struct lsmcontext *cp)
>> +                                          struct lsmcontext *cp, int display)
>>  {
>>         return -EOPNOTSUPP;
>>  }
>> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
>> index 73fc25b4042b..216cb1ffc8f0 100644
>> --- a/include/net/netlabel.h
>> +++ b/include/net/netlabel.h
>> @@ -97,7 +97,7 @@ struct calipso_doi;
>>
>>  /* NetLabel audit information */
>>  struct netlbl_audit {
>> -       u32 secid;
>> +       struct lsmblob lsmdata;
>>         kuid_t loginuid;
>>         unsigned int sessionid;
>>  };
> This chunk seems lost here, does it belong in another patch?

Probably. I am getting a touch of patch-rot showing up.


>> diff --git a/include/net/scm.h b/include/net/scm.h
>> index b77a52f93389..f4d567d4885e 100644
>> --- a/include/net/scm.h
>> +++ b/include/net/scm.h
>> @@ -101,7 +101,7 @@ static inline void scm_passec(struct socket *sock, struct msghdr *msg, struct sc
>>                  * and the infrastructure will know which it is.
>>                  */
>>                 lsmblob_init(&lb, scm->secid);
>> -               err = security_secid_to_secctx(&lb, &context);
>> +               err = security_secid_to_secctx(&lb, &context, LSMBLOB_DISPLAY);
> Misplaced code change?

Same here.


>>                 if (!err) {
>>                         put_cmsg(msg, SOL_SOCKET, SCM_SECURITY, context.len,
>> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
>> index cbff7c2a9724..a10fa01f7bf4 100644
>> --- a/include/net/xfrm.h
>> +++ b/include/net/xfrm.h
>> @@ -660,13 +660,22 @@ struct xfrm_spi_skb_cb {
>>  #define XFRM_SPI_SKB_CB(__skb) ((struct xfrm_spi_skb_cb *)&((__skb)->cb[0]))
>>
>>  #ifdef CONFIG_AUDITSYSCALL
>> -static inline struct audit_buffer *xfrm_audit_start(const char *op)
>> +static inline struct audit_buffer *xfrm_audit_start(const char *op,
>> +                                                   struct audit_context **lac)
>>  {
>> +       struct audit_context *context;
>>         struct audit_buffer *audit_buf = NULL;
>>
>>         if (audit_enabled == AUDIT_OFF)
>>                 return NULL;
>> -       audit_buf = audit_log_start(audit_context(), GFP_ATOMIC,
>> +       context = audit_context();
>> +       if (lac != NULL) {
>> +               if (lsm_multiple_contexts() && context == NULL)
>> +                       context = audit_alloc_local(GFP_ATOMIC);
>> +               *lac = context;
>> +       }
>> +
>> +       audit_buf = audit_log_start(context, GFP_ATOMIC,
>>                                     AUDIT_MAC_IPSEC_EVENT);
>>         if (audit_buf == NULL)
>>                 return NULL;
> Related to the other comments around local audit_contexts, we don't
> want to do this; use the existing audit_context, @lac in this case, so
> that this audit record is bound to the other associated records into a
> single audit event (all have the same timestamp).

Hmm. This is clearly a problem. Looks like a change came in
that I didn't see.

>> diff --git a/kernel/audit.c b/kernel/audit.c
>> index 841123390d41..cba63789a164 100644
>> --- a/kernel/audit.c
>> +++ b/kernel/audit.c
>> @@ -386,10 +386,12 @@ void audit_log_lost(const char *message)
>>  static int audit_log_config_change(char *function_name, u32 new, u32 old,
>>                                    int allow_changes)
>>  {
>> +       struct audit_context *context;
>>         struct audit_buffer *ab;
>>         int rc = 0;
>>
>> -       ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_CONFIG_CHANGE);
>> +       context = audit_alloc_for_lsm(GFP_KERNEL);
>> +       ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONFIG_CHANGE);
> We shouldn't need to do this for the reasons discussed up near the top
> of this email (and elsewhere as well).

Here and elsewhere, I only put audit_alloc_for_lsm() in because there
are cases where audit_context() returns NULL. Really.

>
> I'm going to refrain from commenting on the other uses of
> audit_alloc_for_lsm() in this patch unless there is something unique
> to the code path, so if you don't see me comment about a use of
> audit_alloc_for_lsm() you can assume it should be removed and the
> existing audit_context used instead.
>
>> @@ -399,6 +401,7 @@ static int audit_log_config_change(char *function_name, u32 new, u32 old,
>>                 allow_changes = 0; /* Something weird, deny request */
>>         audit_log_format(ab, " res=%d", allow_changes);
>>         audit_log_end(ab);
>> +       audit_free_local(context);
> See my comment directly above regarding usage of
> audit_alloc_for_lsm(), it obviously applies here too.
>
>> @@ -2128,6 +2136,36 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>>                 audit_log_format(ab, "(null)");
>>  }
>>
>> +static void audit_log_lsm(struct audit_context *context, struct lsmblob *blob)
> See my note below about moving this into audit_log_task_context(),

Either works for me. This seemed consistent with the rest of the audit
code, but I'm happy to change it if you like that better.

>  but
> if we really need to keep this as a separate function, let's consider
> changing the name to something which indicates that it logs the LSM
> data as *subject* fields.  How about audit_log_lsm_subj()?
>
>> +{
>> +       struct audit_buffer *ab;
>> +       struct lsmcontext lsmdata;
>> +       bool sep = false;
>> +       int error;
>> +       int i;
>> +
>> +       ab = audit_log_start(context, GFP_ATOMIC, AUDIT_MAC_TASK_CONTEXTS);
>> +       if (!ab)
>> +               return; /* audit_panic or being filtered */
>> +
>> +       for (i = 0; i < LSMBLOB_ENTRIES; i++) {
>> +               if (blob->secid[i] == 0)
>> +                       continue;
>> +               error = security_secid_to_secctx(blob, &lsmdata, i);
>> +               if (error && error != -EINVAL) {
>> +                       audit_panic("error in audit_log_lsm");
>> +                       return;
>> +               }
>> +
>> +               audit_log_format(ab, "%ssubj_%s=\"%s\"", sep ? " " : "",
>> +                                lsm_slot_to_name(i), lsmdata.context);
>> +               sep = true;
> Since @i starts at zero, you can get rid of @sep by replacing it with @i:
>
>   audit_log_format(ab, ..., (i ? " " : ""), ...);

Clever.

>
>> +               security_release_secctx(&lsmdata);
>> +       }
>> +       audit_log_end(ab);
>> +}
>> @@ -2138,7 +2176,18 @@ int audit_log_task_context(struct audit_buffer *ab)
>>         if (!lsmblob_is_set(&blob))
>>                 return 0;
>>
>> -       error = security_secid_to_secctx(&blob, &context);
>> +       /*
>> +        * If there is more than one security module that has a
>> +        * subject "context" it's necessary to put the subject data
>> +        * into a separate record to maintain compatibility.
>> +        */
>> +       if (lsm_multiple_contexts()) {
>> +               audit_log_format(ab, " subj=?");
>> +               audit_log_lsm(ab->ctx, &blob);
>> +               return 0;
>> +       }
>> +
>> +       error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> Instead of the lsm_multiple_contexts() case bailing on the rest of the
> function with a return inside the if-block, let's made the code a bit
> more robust by organizing it like this:

Sure, why not?

>
>   int audit_log_task_context(ab)
>   {
>      /* common stuff at the start */
>
>      if (lsm_multiple_contexts()) {
>        /* multi-LSM stuff */
>      } else {
>        /* single LSM stuff */
>      }
>
>      /* common stuff at the end */
>   }
>
> ... it also may make sense to just move the body of audit_log_lsm()
> into audit_log_task_context() and do away with audit_log_lsm()
> entirely; is it ever going to be called from anywhere else?
>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index 0e58a3ab56f5..01fdcbf468c0 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -993,12 +993,11 @@ struct audit_context *audit_alloc_local(gfp_t gfpflags)
>>         context = audit_alloc_context(AUDIT_STATE_BUILD, gfpflags);
>>         if (!context) {
>>                 audit_log_lost("out of memory in audit_alloc_local");
>> -               goto out;
>> +               return NULL;
>>         }
>>         context->serial = audit_serial();
>>         ktime_get_coarse_real_ts64(&context->ctime);
>>         context->local = true;
>> -out:
>>         return context;
>>  }
> This chunk should be moved to 21/25 when audit_alloc_local() is first defined.

True. I was trying to minimize the change to the original audit_alloc_local()
patch on the assumption that it was coming in for other reasons, but that hasn't
happened.


>> @@ -1019,6 +1018,13 @@ void audit_free_context(struct audit_context *context)
>>  }
>>  EXPORT_SYMBOL(audit_free_context);
>>
>> +void audit_free_local(struct audit_context *context)
>> +{
>> +       if (context && context->local)
>> +               audit_free_context(context);
>> +}
>> +EXPORT_SYMBOL(audit_free_local);
> If this is strictly necessary, and I don't think it is, it should also
> be moved to patch 21/25 with the original definition of a local
> audit_context.  However,  there really should be no reason why we have
> to distinguish between a proper and local audtit_context when it comes
> to free'ing the memory, just call audit_free_context() in both cases.
>
>> @@ -1036,7 +1042,7 @@ static int audit_log_pid_context(struct audit_context *context, pid_t pid,
>>                          from_kuid(&init_user_ns, auid),
>>                          from_kuid(&init_user_ns, uid), sessionid);
>>         if (lsmblob_is_set(blob)) {
>> -               if (security_secid_to_secctx(blob, &lsmctx)) {
>> +               if (security_secid_to_secctx(blob, &lsmctx, LSMBLOB_FIRST)) {
> Misplaced code change?
>
> Actually, there are a lot of these below, I'm not going to comment on
> all of them as I think you get the idea ... and I very well may be
> wrong so I'll save you all of my wrongness in that case :)
>
>> diff --git a/security/security.c b/security/security.c
>> index cb359e185d1a..5d7fd982f84a 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2309,7 +2309,7 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
>>                 hlist_for_each_entry(hp, &security_hook_heads.setprocattr,
>>                                      list) {
>>                         rc = hp->hook.setprocattr(name, value, size);
>> -                       if (rc < 0)
>> +                       if (rc < 0 && rc != -EINVAL)
>>                                 return rc;
>>                 }
> This really looks misplaced ... ?

Yeah, you're not the first person to notice these bits of patch-rot.
I have some clean-up to do.

Thank you for the review.

Powered by blists - more mailing lists