[PATCH] context based audit filtering (take 3)

[Dustin Kirkland]

On 2/21/06, Darrel Goeddel <dgoeddel at trustedcs.com> wrote:
> The updated version of Dustin's patch I referred to is below.

Wow, Darrel, thanks for updating my patch.  That's definitely above
and beyond my expectations--I figured I would be reworking it to match
your API changes.

More comments inline, but one big, general one...  We need to make
sure that we test this with SELinux compiled out of the kernel, as
well as with SELinux disabled on a running system.  Obviously the
functionality won't be there, but we need to make sure that we handle
those situations gracefully before pushing upstream past our git tree.

> - Add the se_str char * to the audit_field.  This stores a copy of the string
>   target that the rule is based on.  This is needed for the audit_compare_rule
>   addition (below).  It will also be used in the callback for policy reloads
>   (also below).  This is managed along with the se_rule field.  This also gets
>   rid of a memory leak where the unpacked string was not being freed.

Good catch on the mem leak.

> - printk a warning and ignore invalid selinux rules (but still hang on to them
>   so they may be activated with a later policy reload).

Interesting...  Is this the recommended approach by the SELinux folks?

> - Change audit_compare_rule to compare the string values of the selinux rules
>   to see if the rule exists.  Previously, it thought that any selinux filters
>   based on equal length string were equal.
> - Change audit_krule_to_data to put in the string target of the rule for the
>   selinux filters.  This will hopefully allow the display of the type, level,
>   etc. when dumping rules via AUDIT_LIST_RULES (I'll test tommorrow).

Ah yes, these were unfinished pieces of my original patch.  Thanks for
polishing it off.
Amy noted both of these and I was saving them to-do as soon as your
API was updated.

> - Add a selinux callback for re-initializing the se_rule field when there is
>   a policy reload.  THIS NEEDS WORK - It doesn't obey proper locking yet, but
>   it is functional.  I need to get my head around the locking of the audit
>   structures a little better - I'll also take suggestions ;)

Outside of my expertise.  Deferring.

> - Obtain the sid of the task in audit_filter_rules instead of the callers
>   obtaining it and passing it in.
>
>
> diff --git a/kernel/audit.h b/kernel/audit.h
> index eb33354..3ffc70a 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -59,9 +59,11 @@ struct audit_watch {
>  };
>
>  struct audit_field {
> -       u32                     type;
> -       u32                     val;
> -       u32                     op;
> +       u32                             type;
> +       u32                             val;
> +       u32                             op;
> +       char                            *se_str;
> +       struct selinux_audit_rule       *se_rule;
>  };
>
>  struct audit_krule {
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index 3712295..306e941 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -25,6 +25,7 @@
>  #include <linux/fs.h>
>  #include <linux/namei.h>
>  #include <linux/netlink.h>
> +#include <linux/selinux.h>
>  #include "audit.h"
>
>  /* There are three lists of rules -- one to search at task creation
> @@ -50,6 +51,12 @@ static inline void audit_free_watch(stru
>
>  static inline void audit_free_rule(struct audit_entry *e)
>  {
> +       int i;
> +       for (i = 0; i < e->rule.field_count; i++) {
> +               struct audit_field *f = &e->rule.fields[i];
> +               kfree(f->se_str);

The memory leak fix.  Thanks.

> +               selinux_audit_rule_free(f->se_rule);
> +       }
>         kfree(e->rule.fields);
>         kfree(e);
>  }

<snip>

>  }
> +
> +static int selinux_callback(void)

Is this function name descriptive enough?  At the least, a little
documentation might help explain what this is all about ;-)

> +{
> +       struct audit_entry *entry;
> +       int i, j, err = 0;
> +
> +       /* TODO: add proper locking. */
> +       for (i = 0; i < AUDIT_NR_FILTERS; i++) {
> +               list_for_each_entry(entry, &audit_filter_list[i], list) {
> +                       for (j = 0; j < entry->rule.field_count; j++) {
> +                               struct audit_field *f = &entry->rule.fields[j];
> +                               switch (f->type) {
> +                               case AUDIT_SE_USER:
> +                               case AUDIT_SE_ROLE:
> +                               case AUDIT_SE_TYPE:
> +                               case AUDIT_SE_SEN:
> +                               case AUDIT_SE_CLR:
> +                                       selinux_audit_rule_free(f->se_rule);
> +                                       err = selinux_audit_rule_init(f->type,
> +                                                f->op, f->se_str, &f->se_rule);
> +                                       if (err == -EINVAL) {
> +                                               printk(KERN_WARNING "selinux audit rule for item %s is invalid\n", f->se_str);
> +                                               err = 0;

Is this desired behavior?

<snip>

> @@ -258,6 +262,18 @@ static int audit_filter_rules(struct tas
>                         if (ctx)
>                                 result = audit_comparator(ctx->loginuid, f->op, f->val);
>                         break;
> +               case AUDIT_SE_USER:
> +               case AUDIT_SE_ROLE:
> +               case AUDIT_SE_TYPE:
> +               case AUDIT_SE_SEN:
> +               case AUDIT_SE_CLR:
> +                       /* NOTE: this may return negative values indicating
> +                          a temporary error.  We simply treat this as a
> +                          match for now to avoid losing information that
> +                          may be wanted. */

Can we get clarification from Klaus that this meets LSPP/RBACPP
requirements?  I would think that this conservative approach would be
acceptable.

> +                       result = selinux_audit_rule_match(sid, f->type, f->op,
> +                                                         f->se_rule);
> +                       break;
>                 case AUDIT_ARG0:
>                 case AUDIT_ARG1:
>                 case AUDIT_ARG2:
>