[PATCH] new audit rule interface

[Steve Grubb]

On Wednesday 21 December 2005 19:08, Amy Griffis wrote:

Thanks Amy for sending this. We at least have the beginnings of the new file 
system audit patch to chew on. :)   I only have a couple issues listed below. 
They basically revolve around length checking of the netlink packet.

>         case AUDIT_ADD:
>         case AUDIT_DEL:
> -               if (nlh->nlmsg_len < sizeof(struct audit_rule))
> +               if (nlmsg_len(nlh) < sizeof(struct audit_rule))

This checks the lower size bound, is there a check for the upper bound? Or 
that the strings after buf[0] are in fact correctly sized? Does all the 
lengths + struct add up to be equal to the payload length?

>                         return -EINVAL;
>                 /* fallthrough */
>         case AUDIT_LIST:
>                 err = audit_receive_filter(nlh->nlmsg_type,
> NETLINK_CB(skb).pid, -                                          uid, seq,
> data, loginuid); +                                          uid, seq, data,
> nlmsg_len(nlh), +                                          loginuid);
> +               break;
> +       case AUDIT_ADD_RULE:
> +       case AUDIT_DEL_RULE:
> +               if (nlmsg_len(nlh) < sizeof(struct audit_rule_xprt))

Same.

> +                       return -EINVAL;
> +               /* fallthrough */
> +       case AUDIT_LIST_RULES:
> +               err = audit_receive_filter(nlh->nlmsg_type,
> NETLINK_CB(skb).pid, +                                          uid, seq,
> data, nlmsg_len(nlh), +                                          loginuid);
>                 break;
>         case AUDIT_SIGNAL_INFO:
>                 sig_data.uid = audit_sig_uid;

> right); diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index a3a3275..1fd4b2a 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -40,52 +40,253 @@ struct list_head audit_filter_list[AUDIT
>  #endif
>  };
>  
> -/* Copy rule from user-space to kernel-space.  Called from
> - * audit_add_rule during AUDIT_ADD. */
> -static inline int audit_copy_rule(struct audit_rule *d, struct audit_rule
> *s) +/* Unpack a filter field's string representation from user-space
> + * buffer. */
> +static inline char *audit_unpack_string(void **bufp, int *remaining, int
> len) {

I couldn't find callers for this. Is there any chance that len or remaining 
could be negative? If not, lets make them unsigned.

> +       char *str;
> +
> +       if (!*bufp || (len > *remaining))
> +               return ERR_PTR(-EINVAL);

len should always be <= PATH_MAX*2. That must be checked for as well or there 
could be wrapping.

> +/* Common user-space to kernel rule translation. */
> +static struct audit_entry *audit_to_entry_common(struct audit_rule *rule)
> +{
> +       unsigned listnr;
> +       struct audit_entry *entry;
> +       int i, err;
> +
> +       err = -EINVAL;
> +       listnr = rule->flags & ~AUDIT_FILTER_PREPEND;
> +       switch(listnr) {
> +       default:
> +               goto exit_err;
> +       case AUDIT_FILTER_USER:
> +       case AUDIT_FILTER_TYPE:
> +#ifdef CONFIG_AUDITSYSCALL
> +       case AUDIT_FILTER_ENTRY:
> +       case AUDIT_FILTER_EXIT:
> +       case AUDIT_FILTER_TASK:
> +#endif
> +               ;
> +       }
> +       if (rule->action != AUDIT_NEVER && rule->action != AUDIT_POSSIBLE
> && +           rule->action != AUDIT_ALWAYS)
> +               goto exit_err;
> +       if (rule->field_count < 0 || rule->field_count > AUDIT_MAX_FIELDS)
> +               goto exit_err;

field_count is u32 & cannot be less than 0. It should be checked to make sure 
its not 0, though.

> +
> +       err = -ENOMEM;
> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               goto exit_err;
> +       memset(&entry->rule, 0, sizeof(struct audit_krule));
> +
> +       entry->rule.flags = rule->flags & AUDIT_FILTER_PREPEND;
> +       entry->rule.listnr = listnr;
> +       entry->rule.action = rule->action;
> +       entry->rule.field_count = rule->field_count;
> +
> +       for (i = 0; i < AUDIT_BITMASK_SIZE; i++)
> +               entry->rule.mask[i] = rule->mask[i];
> +
> +       return entry;
> +
> +exit_err:
> +       return ERR_PTR(err);
> +}
> +
> +/* Translate struct audit_rule to kernel's rule respresentation.
> + * Exists for backward compatibility with userspace. */
> +static struct audit_entry *audit_rule_to_entry(struct audit_rule *rule)
> +{
> +       struct audit_entry *entry;
> +       int err = 0;
>         int i;
>  
> -       if (s->action != AUDIT_NEVER
> -           && s->action != AUDIT_POSSIBLE
> -           && s->action != AUDIT_ALWAYS)
> -               return -1;
> -       if (s->field_count < 0 || s->field_count > AUDIT_MAX_FIELDS)
> -               return -1;

Same as above. sb == 0 rather than < 0.

> -       if ((s->flags & ~AUDIT_FILTER_PREPEND) >= AUDIT_NR_FILTERS)
> -               return -1;
> -
> -       d->flags        = s->flags;
> -       d->action       = s->action;
> -       d->field_count  = s->field_count;
> -       for (i = 0; i < d->field_count; i++) {
> -               d->fields[i] = s->fields[i];
> -               d->values[i] = s->values[i];
> +       entry = audit_to_entry_common(rule);
> +       if (IS_ERR(entry))
> +               goto exit_nofree;
> +
> +       for (i = 0; i < rule->field_count; i++) {
> +               struct audit_field *f = &entry->rule.fields[i];
> +
> +               if (rule->fields[i] & AUDIT_UNUSED_BITS) {
> +                       err = -EINVAL;
> +                       goto exit_free;
> +               }
> +
> +               f->op = rule->fields[i] & (AUDIT_NEGATE|AUDIT_OPERATORS);
> +               f->type = rule->fields[i] &
> (~AUDIT_NEGATE|AUDIT_OPERATORS); +               f->val = rule->values[i];
> +
> +               entry->rule.vers_ops = (f->op & AUDIT_OPERATORS) ? 2 : 1;
> +               if (f->op & AUDIT_NEGATE)
> +                       f->op |= AUDIT_NOT_EQUAL;
> +               else if (!(f->op & AUDIT_OPERATORS))
> +                       f->op |= AUDIT_EQUAL;
> +               f->op &= ~AUDIT_NEGATE;
>         }
> -       for (i = 0; i < AUDIT_BITMASK_SIZE; i++) d->mask[i] = s->mask[i];
> -       return 0;
> +
> +exit_nofree:
> +       return entry;
> +
> +exit_free:
> +       kfree(entry);
> +       return ERR_PTR(err);
>  }
>  
> -/* Check to see if two rules are identical.  It is called from
> - * audit_add_rule during AUDIT_ADD and
> - * audit_del_rule during AUDIT_DEL. */
> -static int audit_compare_rule(struct audit_rule *a, struct audit_rule *b)
> +/* Translate struct audit_rule_xprt to kernel's rule respresentation. */
> +static struct audit_entry *audit_xprt_to_entry(struct audit_rule_xprt
> *xprt, +                                              int dlen)
>  {
> +       int err = 0;
> +       struct audit_entry *entry;
> +       void *bufp;
> +       /* int remaining = dlen - sizeof(struct audit_rule_xprt); */
>         int i;
>  
> -       if (a->flags != b->flags)
> -               return 1;
> +       entry = audit_to_entry_common((struct audit_rule *)xprt);
> +       if (IS_ERR(entry))
> +               goto exit_nofree;
> +
> +       bufp = xprt->buf;

What uses bufp?

> +/* Pack a filter field's string representation into data block. */
> +static inline int audit_pack_string(void **bufp, char *str)

What calls this?

> +{
> +       int len = strlen(str);

Why use int? size_t is natural size.

> +       memcpy(*bufp, str, len);
> +       *bufp += len;
> +
> +       return len;
> +}
> +


-Steve