Re: [PATCH] fanotify: fix permission model of unprivileged group

[Jan Kara <jack@suse.cz>]

On Mon 24-05-21 18:41:36, Matthew Bobrowski wrote:
> On Sat, May 22, 2021 at 12:19:16PM +0300, Amir Goldstein wrote:
> > Reporting event->pid should depend on the privileges of the user that
> > initialized the group, not the privileges of the user reading the
> > events.
> > 
> > Use an internal group flag FANOTIFY_UNPRIV to record the fact the the
> > group was initialized by an unprivileged user.
> > 
> > To be on the safe side, the premissions to setup filesystem and mount
> > marks now require that both the user that initialized the group and
> > the user setting up the mark have CAP_SYS_ADMIN.
> > 
> > Fixes: 7cea2a3c505e ("fanotify: support limited functionality for unprivileged users")
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> 
> Thanks for sending through this patch Amir!
> 
> In general, the patch looks good to me, however there's just a few
> nits below.
> 
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 71fefb30e015..7df6cba4a06d 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -424,11 +424,18 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group,
> >  	 * events generated by the listener process itself, without disclosing
> >  	 * the pids of other processes.
> >  	 */
> > -	if (!capable(CAP_SYS_ADMIN) &&
> > +	if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> >  	    task_tgid(current) != event->pid)
> >  		metadata.pid = 0;
> >  
> > -	if (path && path->mnt && path->dentry) {
> > +	/*
> > +	 * For now, we require fid mode for unprivileged listener, which does
> > +	 * record path events, but keep this check for safety in case we want
> > +	 * to allow unprivileged listener to get events with no fd and no fid
> > +	 * in the future.
> > +	 */
> 
> I think it's best if we keep clear of using first person in our
> comments throughout our code base. Maybe we could change this to:
> 
> * For now, fid mode is required for an unprivileged listener, which
>   does record path events. However, this check must be kept...

Actually, I have no problem with the first person in comments. It is a
standard "anonymous" language and IMO easy to understand as well. Also
frequently used in the kernel AFAICT. What problem do you see with the
first person? I'm well aware that unlike us you are a native speaker ;)

> > @@ -1305,11 +1320,13 @@ static int do_fanotify_mark(int fanotify_fd, unsigned int flags, __u64 mask,
> >  	group = f.file->private_data;
> >  
> >  	/*
> > -	 * An unprivileged user is not allowed to watch a mount point nor
> > -	 * a filesystem.
> > +	 * An unprivileged user is not allowed to setup mount point nor
>   	      		   	       	       	  	      	   ^
> 								   s
> > +	 * filesystem marks. It is not allowed to setup those marks for
> > +	 * a group that was initialized by an unprivileged user.
> 
> I think the second sentence would better read as:
> 
>        * This also includes setting up such marks by a group that was
>          intialized by an unprivileged user.

Yes, this is probably better.

> >  	show_fdinfo(m, f, fanotify_fdinfo);
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index bad41bcb25df..f277d1c4e6b8 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -51,6 +51,10 @@ extern struct ctl_table fanotify_table[]; /* for sysctl */
> >  #define FANOTIFY_INIT_FLAGS	(FANOTIFY_ADMIN_INIT_FLAGS | \
> >  				 FANOTIFY_USER_INIT_FLAGS)
> >  
> > +/* Internal flags */
> > +#define FANOTIFY_UNPRIV		0x80000000
> > +#define FANOTIFY_INTERNAL_FLAGS	(FANOTIFY_UNPRIV)
> 
> Should we be more distinct here i.e. FANOTIFY_INTERNAL_INIT_FLAGS?
> Just thinking about a possible case where there's some other internal
> fanotify flags that are used for something else?

Well, do we need to distinguish different uses of internal flags? I don't
think so...

								Honza

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