[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]