Re: [RFC][PATCH] (#6) filesystem auditing
On Wednesday 16 March 2005 18:50, Timothy R. Chavez wrote:
I went ahead and just made then __u32.
Good.
> if (nlh->nlmsg_len != sizeof(struct audit_watch))
> return -EINVAL
I'll put something like this in here.
Good.
> 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:
What I mean by use is allocating memory. fklen is completely unchecked before
allocating memory. It should only be used if the length is < PATH_MAX (now
that its a __u32). Just imagine the user is up to no good and wanting to take
the system down. I know they got to be root at this point, but one day that
requirement may change and the kernel needs to be able to withstand abuse and
not keel over.
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.
That's too late to check the lengths because memory has already been
allocated. Also, does the name the user passed in have a terminating 0 on or
before the PATH_MAX? Is the basename <= NAME_MAX ? Does filterkey have a
terminating 0 on or before AUDIT_FILTERKEY_MAX ? Are watch perms reasonable
(perms <= WATCH_MAY_READ|WATCH_MAY_WRITE|WATCH_MAY_EXEC ? Is the name passed
to the kernel containing relative notation?
Be paranoid. Right now, the only program that puts rules into the kernel is
auditctl. I've added these checks and will release a new audit package as
soon as we have a kernel that we can use it with. But somebody else may
decide they can do it better and send into the kernel unchecked data. There's
no reason someone can't hack their own utility. The kernel should check the
validity of the data before accepting it for use.
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.
I suppose they could share a few common paranoid checks.
-Steve