VFS hooks analysis (pass 1)

[Timothy R. Chavez]

On Friday 26 August 2005 17:13, Amy Griffis wrote:
> Hello,

Hey Amy, great work.  Well organized.  Thanks.  Comments below.

> 
> I've been investigating audit's needs for VFS hook placement, to
> determine whether audit would require additional events or additional
> information about events, other than what is currently provided by
> Inotify.  
> 
> I've written up my findings in hopes that some of you who may be more
> familiar with the situation than I can correct my mistakes, and that
> we can all be on the same page with regard to audit's needs.
> 
> At this point, I haven't addressed the issue of race conditions.  For
> the purpose of the first pass, I make the assumption that the current
> fsnotify hook placement is sufficient.  I'll address the possibility
> of race conditions in a second pass.
> 
> Background:
> 
> The purpose of the VFS hooks is to capture information about an
> object's identity.  In this implementation, identity means inode and
> name.  Identity information is needed for filtering and for log record
> detail.
> 
> To narrow the field, I limited the set of syscalls to those which are
> defined by CAPP as security-relevant and are also filesystem-related.
> I believe this is the correct list:
> 
>     sys_access
>     sys_chdir
>     sys_chmod
>     sys_chown
>     sys_creat
>     sys_execve
>     sys_fchmod
>     sys_fchown
>     sys_fremovexattr
>     sys_fsetxattr
>     sys_lchown
>     sys_link
>     sys_lremovexattr
>     sys_lsetxattr
>     sys_mkdir
>     sys_mknod
>     sys_mount
>     sys_open
>     sys_removexattr
>     sys_rename
>     sys_rmdir
>     sys_setxattr
>     sys_swapon
>     sys_symlink
>     sys_truncate
>     sys_unlink
>     sys_utime(s)
> 
> Available Object Identity Info:
> 
> The upstream audit code uses getname() and path_lookup() hooks to
> collect object identity information during syscall processing.  This
> is sufficient for the following syscalls:
> 
>     sys_access
>     sys_chdir
>     sys_chmod
>     sys_chown
>     sys_execve
>     sys_lchown
>     sys_link
>     sys_lremovexattr
>     sys_lsetxattr
>     sys_removexattr
>     sys_setxattr
>     sys_swapon
>     sys_truncate
>     sys_utime(s)
> 

Here's my thinking.  It'd be nice to have a complete set of Inotify hooks
that map to specific Inotify events (IN_*).  Thus, even though the above
syscalls may be sufficiently covered by the hook placements in the 
getname() and path_lookup() functions, I think we should split them out
into seperate Inotify hooks.  This gives us the ability to consistently audit
a file system object based on a set of events (which is ammenable to 
Steve's log size reduction campaign :-)).  This also makes the overall
API cleaner and more coherent.

The administrator would then be able to do something like this,

auditctl -w /this/path/watch_this_inode -E create,open,close,write

> Lacking Object Identity Info:
> 
> The information audit needs isn't available when path_lookup is called
> with LOOKUP_PARENT, or when getname/path_lookup are not called at all.
> In these situations, audit needs more VFS hooks to get the necessary
> information.
> 
> Attribute Changes:
> 
> The getname/path_lookup hooks are never called for these syscalls:
> 
>     sys_fchmod
>     sys_fchown
>     sys_fremovexattr
>     sys_fsetxattr
> 
> The fsnotify_change() and fsnotify_xattr() hooks capture the relevant
> dentry, so the dentry's name & inode could be passed to the event
> callback.
> 
> Mounts:
> 
> For sys_mount, audit does not currently capture the pathname for the
> device.  This is because getname is not called before path_lookup.  We
> could modify audit_inode() to save the name from path_lookup for
> "nameless" inodes.  Note the FIXME in audit_inode().
> 
> The rest of the syscalls are mostly lacking information due to
> path_lookup being called with LOOKUP_PARENT.
> 
> Created Objects:
> 
>     sys_creat
>     sys_mkdir
>     sys_mknod
>     sys_open
> 
> The fsnotify_create() and fsnotify_mkdir() hooks have the same
> placement as audit_notify_watch(), but audit needs the dentry's inode.
> The fsnotify hooks could be modified to capture the dentry, instead of
> just the dentry->d_name.name.
> 
> Removed Objects:
> 
>     sys_rmdir
>     sys_unlink 
> 
> The fsnotify_inoderemove() hook provides the inode, but see note below
> on capturing attempted removals.
> 
> Symlinks:
> 
>     sys_symlink
> 
> The inodes for the target path and symlink are not available from the
> getname/path_lookup hoooks.  The PATH records do not indicate which
> name is the symlink.  Maybe this isn't necessary since you can tell
> from the syscall args.
> 
> As previously mentioned, the fsnotify_create() hook could be modified
> to capture the dentry instead of the dentry->d_name.name.  This would
> provide audit with the symlink's inode.
> 
> I haven't found a good way to capture the target inode.  We don't do
> it in the current implementation, which means we don't log events when
> someone makes a symlink to a watched inode or pathname.  It seems like
> we should, though.  
> 
> Renames:
> 
>     sys_rename
> 
> The inode for the relevant object is not available from the
> getname/path_lookup hooks.  There is also no way to indicate the old
> name versus the new name, other than by the syscall args.
> 
> The fsnotify_move() hook could be modified to capture the old and new
> dentrys, instead of just the names, which would provide audit with the
> inode (twice).  Inotify provides separate to/from events.
> 
> Capturing Unsuccessful Attempts:
> 
> CAPP dictates that audit must capture unsuccessful attempts from open,
> rename, truncate, and unlink.  The open and truncate calls are a
> non-issue, the inode is obtained from the path_lookup hook.
> 
> The rename and unlink calls present the problem.  Since the inode
> wouldn't be captured unless the rename/remove was successful, an
> inode-based filter wouldn't catch unsuccessful attempts.
> 
> Summary:
> 
> To summarize, I haven't found a situation requiring the current
> permission() and exec_permission_lite() hooks.

Good.  I'm of the opinion that we should stay away from catch-all hooks.
Especially since we now have the benefit of Inotify and its prolific use
of file system hooks.

> 
> The issues seem to be with symlinks and logging unsuccessful
> rename/remove attempts.
> 
> I can think of three possible solutions for the latter:
> 
>     (1) Require filters based on filenames to capture unsuccessful
>         attempts.  This seems undesirable.
> 
>     (2) Have an audit-specific hook in may_delete(), as is currently
>         done, but have it tie in to inode-based filters as well.
> 
>     (3) Request an additional Inotify event that would not be included
>         in IN_ALL_EVENTS, so wouldn't impact Inotify's other users.
> 
> I think 2 or 3 are our best options, with the answer likely depending
> on what Inotify is willing to add.  It is probably doable to add
> audit-specific hooks if they are few and if Inotify isn't interested.
> 
> I do not yet have a solution for capturing the target inode from a
> symlink operation.  However since it's not currently done, maybe it
> isn't a hard requirement?
> 
> I'd appreciate any comments on my analysis.
> 
> Thanks,
> Amy

Thank you, this is really quite helpful.

-tim