[PATCH] new audit rule interface

[Steve Grubb]

On Tuesday 03 January 2006 18:05, Amy Griffis wrote:
> These message types use struct audit_rule, which doesn't support an
> additional buffer.

Ah, Ok.

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

In the last incarnation of file system audit code, we tried hard to validate  
the length before ever allocating any kernel memory of any kind. I was trying 
to trace through to see where the length is really checked to ensure the 
payload is legit.

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

In order to make sure people aren't playing games by abusing the audit system, 
I was thinking that you could do a check that the length isn't too big also. 
If you have a u32 for len, and someone passes -1, then you have a large 
positive number. To detect these problems, you have to check for a legal 
upper bounds. In this case, the longest string should be PATH_MAX. I guess 
the "*2" only applies to actual audit records rather than the file path for 
rules. 

> > > +/* 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;
> > > +
> > > +???????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

OK...right. So the < 0 part can be deleted.

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

OK, I thought the patch was complete. Nevermind.

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

I really need to see the consumer to finish evaluating the use of the 
interface. I just want to make sure there's no vulnerabilities that will give 
hackers a chance.

-Steve