Re: [PATCH] new audit rule interface
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