Re: [PATCH v30 25/28] Audit: Add record for multiple task security contexts

[Paul Moore <paul@paul-moore.com>]

On Tue, Nov 23, 2021 at 9:11 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> Create a new audit record AUDIT_MAC_TASK_CONTEXTS.
> An example of the MAC_TASK_CONTEXTS (1420) record is:
>
>     type=UNKNOWN[1420]
>     msg=audit(1600880931.832:113)
>     subj_apparmor="=unconfined"
>     subj_smack="_"
>
> When an audit event includes a AUDIT_MAC_TASK_CONTEXTS record
> the "subj=" field in other records in the event will be "subj=?".
> A AUDIT_MAC_TASK_CONTEXTS record is supplied when the system has
> multiple security modules that may make access decisions based
> on a subject security context.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  include/linux/security.h   |  9 ++++++
>  include/uapi/linux/audit.h |  1 +
>  kernel/audit.c             | 66 ++++++++++++++++++++++++++++++++------
>  3 files changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 763dca314c00..b98545d2ae04 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -231,6 +231,15 @@ static inline bool lsmblob_equal(struct lsmblob *bloba, struct lsmblob *blobb)
>  extern int lsm_name_to_slot(char *name);
>  extern const char *lsm_slot_to_name(int slot);
>
> +static inline bool lsm_multiple_contexts(void)
> +{
> +#ifdef CONFIG_SECURITY
> +       return lsm_slot_to_name(1) != NULL;
> +#else
> +       return false;
> +#endif
> +}
> +
>  /**
>   * lsmblob_value - find the first non-zero value in an lsmblob structure.
>   * @blob: Pointer to the data
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index 9176a095fefc..86ad3da4f0d4 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -143,6 +143,7 @@
>  #define AUDIT_MAC_UNLBL_STCDEL 1417    /* NetLabel: del a static label */
>  #define AUDIT_MAC_CALIPSO_ADD  1418    /* NetLabel: add CALIPSO DOI entry */
>  #define AUDIT_MAC_CALIPSO_DEL  1419    /* NetLabel: del CALIPSO DOI entry */
> +#define AUDIT_MAC_TASK_CONTEXTS        1420    /* Multiple LSM task contexts */
>
>  #define AUDIT_FIRST_KERN_ANOM_MSG   1700
>  #define AUDIT_LAST_KERN_ANOM_MSG    1799
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 2b22498d3532..6c93545a14f3 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -197,6 +197,9 @@ static struct audit_ctl_mutex {
>  struct audit_context_entry {
>         struct list_head        list;
>         int                     type;   /* Audit record type */
> +       union {
> +               struct lsmblob  mac_task_context;

The name "mac_task_context" is awfully long, why not something like
"lsm_subjs" or similar?

> +       };
>  };
>
>  /* The audit_buffer is used when formatting an audit record.  The caller
> @@ -2139,6 +2142,21 @@ void audit_log_key(struct audit_buffer *ab, char *key)
>                 audit_log_format(ab, "(null)");
>  }
>
> +static int audit_add_aux_task(struct audit_buffer *ab, struct lsmblob *blob)
> +{
> +       struct audit_context_entry *ace;
> +
> +       ace = kzalloc(sizeof(*ace), GFP_KERNEL);

Instead of using GFP_KERNEL I would recommend using ab->gfp_mask to
ensure we don't run into allocation problems depending on the calling
context.

> +       if (!ace)
> +               return -ENOMEM;
> +
> +       INIT_LIST_HEAD(&ace->list);
> +       ace->type = AUDIT_MAC_TASK_CONTEXTS;

I would suggest one of the following:

A) Add a "type" parameter to the function and use it here so that this
function truly generic.
B) Leave the ace->type assignment alone and change the function name
to audit_add_mac_task_contexts().
C) Fold this entire function into audit_log_task_context().

Of the three, I think choice B makes the least amount of sense; if
this is a dedicated AUDIT_MAC_TASK_CONTEXTS function then it should
probably just live inside audit_log_task_context() (choice C).
Choosing between A and C is really a matter of deciding if you are
going to use this function elsewhere, and it doesn't appear that you
do so in this patchset.  Sure, other patchsets might make use of this
someday (or not), but if they do they can also extract it back out
into a separate function (if needed).

> +       ace->mac_task_context = *blob;
> +       list_add(&ace->list, &ab->aux_records);
> +       return 0;
> +}
> +
>  int audit_log_task_context(struct audit_buffer *ab)
>  {
>         int error;
> @@ -2149,16 +2167,22 @@ int audit_log_task_context(struct audit_buffer *ab)
>         if (!lsmblob_is_set(&blob))
>                 return 0;
>
> -       error = security_secid_to_secctx(&blob, &context, LSMBLOB_FIRST);
> -       if (error) {
> -               if (error != -EINVAL)
> -                       goto error_path;
> +       if (!lsm_multiple_contexts()) {
> +               error = security_secid_to_secctx(&blob, &context,
> +                                                LSMBLOB_FIRST);
> +               if (error) {
> +                       if (error != -EINVAL)
> +                               goto error_path;
> +                       return 0;
> +               }
> +               audit_log_format(ab, " subj=%s", context.context);
> +               security_release_secctx(&context);
>                 return 0;
>         }
> -
> -       audit_log_format(ab, " subj=%s", context.context);
> -       security_release_secctx(&context);
> -       return 0;
> +       audit_log_format(ab, " subj=?");
> +       error = audit_add_aux_task(ab, &blob);
> +       if (!error)
> +               return 0;

This is very bikeshed-y, but from a style perspective I would prefer
to see something like this:

  if (!lsm_multiple_contexts()) {
    /* existing, single LSM code */
  } else {
    /* new, multiple LSM code */
  }
  return error;

IMO it makes it a bit more clear that each code path is equally likely
and neither is an exception.

>  error_path:
>         audit_panic("error in audit_log_task_context");
> @@ -2419,9 +2443,12 @@ void audit_log_end(struct audit_buffer *ab)
>         struct audit_context_entry *entry;
>         struct audit_context mcontext;
>         struct audit_context *mctx;
> +       struct lsmcontext lcontext;
>         struct audit_buffer *mab;
>         struct list_head *l;
>         struct list_head *n;
> +       int rc;
> +       int i;
>
>         if (!ab)
>                 return;
> @@ -2448,7 +2475,28 @@ void audit_log_end(struct audit_buffer *ab)
>                         continue;
>                 }
>                 switch (entry->type) {
> -               /* Don't know of any quite yet. */
> +               case AUDIT_MAC_TASK_CONTEXTS:
> +                       for (i = 0; i < LSMBLOB_ENTRIES; i++) {
> +                               if (entry->mac_task_context.secid[i] == 0)
> +                                       continue;
> +                               rc = security_secid_to_secctx(
> +                                               &entry->mac_task_context,
> +                                               &lcontext, i);
> +                               if (rc) {
> +                                       if (rc != -EINVAL)
> +                                               audit_panic("error in audit_log_end");
> +                                       audit_log_format(mab, "%ssubj_%s=\"?\"",

I don't believe you need the quotes around the question mark here, you
should be able to treat it just like it was "subj=?".

> +                                                        i ? " " : "",
> +                                                        lsm_slot_to_name(i));
> +                               } else {
> +                                       audit_log_format(mab, "%ssubj_%s=\"%s\"",

Same as above.

> +                                                        i ? " " : "",
> +                                                        lsm_slot_to_name(i),
> +                                                        lcontext.context);
> +                                       security_release_secctx(&lcontext);
> +                               }
> +                       }
> +                       break;
>                 default:
>                         audit_panic("Unknown type in audit_log_end");
>                         break;
> --
> 2.31.1

--
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit