Re: [Non-DoD Source] Re: [RFC PATCH v2] fanotify, inotify, dnotify, security: add security hook for fs notifications

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Jul 23, 2019 at 7:17 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
>
> On 7/18/19 12:16 PM, Amir Goldstein wrote:
> > On Thu, Jul 18, 2019 at 5:31 PM Aaron Goidel <acgoide@tycho.nsa.gov> wrote:
> >>
> >> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> >> index a90bb19dcfa2..9e3137badb6b 100644
> >> --- a/fs/notify/fanotify/fanotify_user.c
> >> +++ b/fs/notify/fanotify/fanotify_user.c
> >> @@ -528,9 +528,10 @@ static const struct file_operations fanotify_fops = {
> >>   };
> >>
> >>   static int fanotify_find_path(int dfd, const char __user *filename,
> >> -                             struct path *path, unsigned int flags)
> >> +                             struct path *path, unsigned int flags, __u64 mask)
> >>   {
> >>          int ret;
> >> +       unsigned int mark_type;
> >>
> >>          pr_debug("%s: dfd=%d filename=%p flags=%x\n", __func__,
> >>                   dfd, filename, flags);
> >> @@ -567,8 +568,30 @@ static int fanotify_find_path(int dfd, const char __user *filename,
> >>
> >>          /* you can only watch an inode if you have read permissions on it */
> >>          ret = inode_permission(path->dentry->d_inode, MAY_READ);
> >> +       if (ret) {
> >> +               path_put(path);
> >> +               goto out;
> >> +       }
> >> +
> >> +       switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> >> +       case FAN_MARK_MOUNT:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> >> +               break;
> >> +       case FAN_MARK_FILESYSTEM:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_SB;
> >> +               break;
> >> +       case FAN_MARK_INODE:
> >> +               mark_type = FSNOTIFY_OBJ_TYPE_INODE;
> >> +               break;
> >> +       default:
> >> +               ret = -EINVAL;
> >> +               goto out;
> >> +       }
> >> +
> >> +       ret = security_inode_notify(path->dentry->d_inode, mask, mark_type);
> >
> > If you prefer 3 hooks security_{inode,mount,sb}_notify()
> > please place them in fanotify_add_{inode,mount,sb}_mark().
> >
> > If you prefer single hook with path argument, please pass path
> > down to fanotify_add_mark() and call security_path_notify() from there,
> > where you already have the object type argument.
> >
> I'm not clear on why you want me to move the hook call down to
> fanotify_add_mark(). I'd prefer to keep it adjacent to the existing
> inode_permission() call so that all the security checking occurs from
> one place.

Fine.

> Moving it down requires adding a path arg to that entire call
> chain, even though it wouldn't otherwise be needed.

That doesn't matter.

> And that raises the
> question of whether to continue passing the mnt_sb, mnt, or inode
> separately or just extract all those from the path inside of
> fanotify_add_*_mark().

You lost me. The major issue I have is with passing @inode argument
to hook for adding a mount watch. Makes no sense to me as @inode
may be accessed from any mount and without passing @path to hook
this information is lost.

>
> It also seems to destroy the parallelism with fanotify_remove_*_mark().

I don't know what that means.

> I also don't see any real benefit in splitting into three separate
> hooks, especially as some security modules will want the path or inode
> even for the mount or superblock cases, since they may have no relevant
> security information for vfsmounts or superblocks.

OK. that is an argument for single hook with @path argument.
That is fine by me.

Thanks,
Amir.