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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOQ4uxh4+SDCF7HHwSxGFx01ZyJ43VSLhLM2dDFY2AQ0HkkuvA@mail.gmail.com>
Date:   Fri, 9 Aug 2019 19:43:09 +0300
From:   Amir Goldstein <amir73il@...il.com>
To:     Aaron Goidel <acgoide@...ho.nsa.gov>
Cc:     Paul Moore <paul@...l-moore.com>, selinux@...r.kernel.org,
        LSM List <linux-security-module@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        David Howells <dhowells@...hat.com>, Jan Kara <jack@...e.cz>,
        James Morris <jmorris@...ei.org>,
        Stephen Smalley <sds@...ho.nsa.gov>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [Non-DoD Source] Re: [PATCH] fanotify, inotify, dnotify,
 security: add security hook for fs notifications

> >>> +       switch (flags & FANOTIFY_MARK_TYPE_BITS) {
> >>> +       case FAN_MARK_MOUNT:
> >>> +               obj_type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> >>> +               break;
> >>> +       case FAN_MARK_FILESYSTEM:
> >>> +               obj_type = FSNOTIFY_OBJ_TYPE_SB;
> >>> +               break;
> >>> +       case FAN_MARK_INODE:
> >>> +               obj_type = FSNOTIFY_OBJ_TYPE_INODE;
> >>> +               break;
> >>> +       default:
> >>> +               ret = -EINVAL;
> >>> +               goto out;
> >>> +       }
> >
> > Sorry, I just can't stand this extra switch statement here.
> > Please initialize obj_type at the very first switch statement in
> > do_fanotify_mark() and pass it to fanotify_find_path().
> > Preferably also make it a helper that returns either
> > valid obj_type or <0 for error.
> >
> >
> I have no problem moving the initialization of obj_type up one level to
> do_fanotify_mark(). I don't think that a helper is necessary at this
> juncture as this logic seems to only exist in one place. Should this
> change, then there would be merit to having a separate function.

Ok.

> >>> +
> >>> +       ret = security_path_notify(path, mask, obj_type);
> >>>          if (ret)
> >>>                  path_put(path);
> >
> > It is probably best to mask out FANOTIFY_EVENT_FLAGS
> > when calling the hook. Although FAN_EVENT_ON_CHILD
> > and FAN_ONDIR do map to corresponding FS_ constants,
> > the security hooks from dnotify and inotify do not pass these
> > flags, so the security module cannot use them as reliable
> > information, so it will have to assume that they have been
> > requested anyway.
> >
> > Alternatively, make sure that dnotify/inotify security hooks
> > always set these two flags, by fixing up and using the
> > dnotify/inotify arg_to_mask conversion helpers before calling
> > the security hook.
> >
> I think that at this point either approach you mentioned makes just as
> much sense. If it's all the same to you, Amir, I'll just change the
> caller in fanotify to include (mask) & ~(FANOTIFY_EVENT_FLAGS)

On second look, let's go with (mask & ALL_FSNOTIFY_EVENTS)
It seems simpler and more appropriate way to convert to FS_ flags.

[...]
> >>>
> >>> -       ret = inotify_find_inode(pathname, &path, flags);
> >>> +       ret = inotify_find_inode(pathname, &path, flags, mask);
> >
> > Please use (mask & IN_ALL_EVENTS) for converting to common FS_ flags
> > or use the inotify_arg_to_mask() conversion helper, which contains more
> > details irrelevant for the security hook.
> > Otherwise mask may contain flags like IN_MASK_CREATE, which mean
> > different things on different backends and the security module cannot tell
> > the difference.
> >
> > Also note that at this point, before inotify_arg_to_mask(), the mask does
> > not yet contain FS_EVENT_ON_CHILD, which could be interesting for
> > the security hook (fanotify users can opt-in with FAN_EVENT_ON_CHILD).
> > Not a big deal though as security hook can assume the worse
> > (that events on child are requested).
> >
> I'll use (mask & IN_ALL_EVENTS).

OK.

>
> I can implement the changes in the ways I mentioned above. I don't see a
> need for anything more in the cases you brought up since none of them
> change the logic of the hook itself or would make a substantive
> difference to the operation of the hook given its current implementation.
>

Agree. If more flags are needed for LSMs they could be added later.

Thanks,
Amir.