Re: [RFC][PATCH] (#6) filesystem auditing
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(a)redhat.com
http://www.redhat.com/mailman/listinfo/linux-audit
-tim