[RFC][PATCH] (#6) filesystem auditing

[Timothy R. Chavez]

On Tuesday 15 March 2005 03:55 pm, Steve Grubb wrote:
> On Monday 14 March 2005 18:14, Timothy R. Chavez wrote:
> > This patch has enough changes in it to be called patch #6
>
> I just noticed this in audit.h:
>
> struct audit_watch {
>        int     namelen;
>        int     fklen;
>
> Shouldn't these be pinned down to a byte size? Like __u32 or __u16? It
> seems safer to me when you consider userspace to kernel packets and whether
> the kernel is 64 bit and userspace 32 bit.
>
> But more interesting...what if a -1 was sent for fklen?
>
> +       if (req->fklen) {
> +               ret = -ENOMEM;
> +               filterkey = kmalloc(req->fklen, GFP_KERNEL);
>
> Kaboom...

I went ahead and just made then __u32.

>
> Also a nit, you have a structure audit_watch and a function audit_watch.
>
> In audit.c in the audit_receive_msg  function,
> @@ -413,6 +416,12 @@ static int audit_receive_msg(struct sk_b
>                 err = -EOPNOTSUPP;
>  #endif
>                 break;
> +       case AUDIT_WATCH_INS:
> +       case AUDIT_WATCH_REM:
> +               err = audit_receive_watch(nlh->nlmsg_type,
> +                                         NETLINK_CB(skb).pid,
> +                                         uid, seq, data);
> +               break;
>
> Shouldn't there be some checking of the packet like so (may not be 100%
> right, but you should see what I mean):
>
>  if (nlh->nlmsg_len != sizeof(struct audit_watch))
>          return -EINVAL

I'll put something like this in here.
>
> before sending it into audit_receive_watch?  And then shouldn't some
> reality checks be done like making sure no file name is greater than
> MAX_PATH and the filterkey is reasonable size before using them in
> audit_insert_watch?

Well I suppose it depends on what you mean by "use" -- There are reality 
checks in there, when attempting to create the watch here:

static struct audit_watch *audit_create_watch(const char *name,
                                              const char *filterkey,
                                              __u32 perms)
{
        struct audit_watch *err = NULL;
        struct audit_watch *watch = NULL;

        err = ERR_PTR(-EINVAL);
        if (!name || strlen(name) + 1 > PATH_MAX)
                goto audit_create_watch_fail;

        if (filterkey && strlen(filterkey) + 1 > AUDIT_FILTERKEY_MAX)
                goto audit_create_watch_fail;

        if (perms > 15)
                goto audit_create_watch_fail;
	.....
}

In the beginning of audit_insert_watch(), I'm merely bringing them from user 
space to kernel space.  Then when I goto create the watchlist entry (wentry) 
that will hold the watch, if I fail upon creating the watch, I drop out.

However, I suppose I should do all the user space->kernel space transitioning 
in in audit_receive_watch() for both audit_insert_watch() and 
audit_remove_watch().  That seems to make more sense.

>
> Also, how do you list the watches? I was looking in userspace code and only
> see insert and remove, no listing.
>

Yeah this is the infamous feature that's missing that needs to be added :)

> That's what I see for now...
>

Thanks.  The patch is mostly done.  It'll be out tommorow.  Much of the 
feedback has been incorporated both by you and Stephen.  I've got a couple 
more things to add/change.  I've also added a couple of my own things -- 
mostly what I hope to be better locking around audit_insert/remove_watch()

> -Steve
>
> --
> Linux-audit mailing list
> Linux-audit at redhat.com
> http://www.redhat.com/mailman/listinfo/linux-audit

-tim