Re: FAN_UNPRIVILEGED

[Jan Kara <jack@suse.cz>]

On Thu 01-10-20 16:08:50, Amir Goldstein wrote:
> On Thu, Oct 1, 2020 at 2:00 PM Jan Kara <jack@suse.cz> wrote:
> >
> > I'm sorry for late reply on this one...
> >
> > On Tue 15-09-20 11:33:41, Amir Goldstein wrote:
> > > On Tue, Sep 15, 2020 at 10:08 AM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Tue 15-09-20 01:27:43, Weiping Zhang wrote:
> > > > > Now the IN_OPEN event can report all open events for a file, but it can
> > > > > not distinguish if the file was opened for execute or read/write.
> > > > > This patch add a new event IN_OPEN_EXEC to support that. If user only
> > > > > want to monitor a file was opened for execute, they can pass a more
> > > > > precise event IN_OPEN_EXEC to inotify_add_watch.
> > > > >
> > > > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com>
> > > >
> > > > Thanks for the patch but what I'm missing is a justification for it. Is
> > > > there any application that cannot use fanotify that needs to distinguish
> > > > IN_OPEN and IN_OPEN_EXEC? The OPEN_EXEC notification is for rather
> > > > specialized purposes (e.g. audit) which are generally priviledged and need
> > > > to use fanotify anyway so I don't see this as an interesting feature for
> > > > inotify...
> > >
> > > That would be my queue to re- bring up FAN_UNPRIVILEGED [1].
> > > Last time this was discussed [2], FAN_UNPRIVILEGED did not have
> > > feature parity with inotify, but now it mostly does, short of (AFAIK):
> > > 1. Rename cookie (*)
> > > 2. System tunables for limits
> > >
> > > The question is - should I pursue it?
> >
> > So I think that at this point some form less priviledged fanotify use
> > starts to make sense. So let's discuss how it would look like... What comes
> > to my mind:
> >
> > 1) We'd need to make max_user_instances, max_user_watches, and
> > max_queued_events configurable similarly as for inotify. The first two
> > using ucounts so that the configuration is actually per-namespace as for
> > inotify.
> >
> > 2) I don't quite like the FAN_UNPRIVILEDGED flag. I'd rather see the checks
> > being done based on functionality requested in fanotify_init() /
> > fanotify_mark(). E.g. FAN_UNLIMITED_QUEUE or permission events will require
> > CAP_SYS_ADMIN, mount/sb marks will require CAP_DAC_READ_SEARCH, etc.
> > We should also consider which capability checks should be system-global and
> > which can be just user-namespace ones...
> 
> OK. That is not a problem to do.
> But FAN_UNPRIVILEDGED flag also impacts:
> 
>     An unprivileged event listener does not get an open file descriptor in
>     the event nor the process pid of another process.

Well, are these really sensitive that they should be forbidden? If we allow
only inode marks and given inode is opened in the context of process
reading the event, I don't see how fd would be any sensitive? And similarly
for pid I'd say...

> Obviously, I can check CAP_SYS_ADMIN on fanotify_init() and set the
> FAN_UNPRIVILEDGED flag as an internal flag.
> 
> The advantage of explicit FAN_UNPRIVILEDGED flag is that a privileged process
> can create an unprivileged listener and pass the fd to another process.
> Not a critical functionality at this point.

I'd prefer to keep the flag internal if you're convinced we need one - but
I'm not yet convinced we need even internal FAN_UNPRIVILEDGED flag because
I don't think this will end up being a yes/no thing. I imagine that
depending on exact process capabilities, different kinds of fanotify
functionality will be allowed as I outlined in 2). So we'll be checking
against current process capabilities at the time of action and not against
some internal fanotify flag...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR