[PATCH] new audit rule interface

[Amy Griffis]

Hi Steve,

Thanks for reviewing.

On Tue, Jan 03, 2006 at 03:46:42PM -0500, Steve Grubb wrote:
> 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?

These message types use struct audit_rule, which doesn't support an
additional buffer.

> 
> > ????????????????????????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.

The netlink packet length is passed along through
audit_receive_filter() to audit_xprt_to_entry(), where it can be
referenced while processing the buffer.  audit_xprt_to_entry() keeps
track of the bytes in the buffer yet to be consumed and provides this
value to audit_unpack_string(), which both checks that sufficient
space remains, and updates the 'remaining' value accordingly.  I see
audit_unpack_string() as being called by this patch's consumers in the
per-field-type switch statement in audit_xprt_to_entry().

> 
> > +???????????????????????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. 

Correct.  I intended for this to be called by consumers from the
switch in audit_xprt_to_entry().  I clearly didn't provide enough
comments. :-/

> Is there any chance that len or remaining could be negative? If not,
> lets make them unsigned.

I'll do that, thanks.

> 
> > +???????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.

I'm confused about the significance of the value PATH_MAX*2 and how
that relates here.  Could you clarify?

> 
> > +/* 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.

I thought it was okay to specify a rule with no fields, i.e.

    auditctl -a entry,always -S swapon

> 
> > +
> > +???????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.

I'm not sure what you mean.  I don't see a variable 'sb' and this is
code that was removed.

> 
> > -???????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?

Nothing right now, but it should be used by a consumer adding code to
the switch statement.

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

This should be called by a consumer from the switch in
audit_krule_to_xprt().

> 
> > +{
> > +???????int len = strlen(str);
> 
> Why use int? size_t is natural size.

Sure, will fix.

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