Re: [PATCH 5/5] fanotify: Add pidfd info record support to the fanotify API

[Amir Goldstein <amir73il@gmail.com>]

> > > > > > > > > You also forgot to add CAP_SYS_ADMIN check before pidfd_create()
> > > > > > > > > (even though fanotify_init() does check for that).
> > > > > > > >
> > > > > > > > I didn't really understand the need for this check here given that the
> > > > > > > > administrative bits are already being checked for in fanotify_init()
> > > > > > > > i.e. FAN_REPORT_PIDFD can never be set for an unprivileged listener;
> > > > > > > > thus never walking any of the pidfd_mode paths. Is this just a defense
> > > > > > > > in depth approach here, or is it something else that I'm missing?
> > > > > > > >
> > > > > > >
> > > > > > > We want to be extra careful not to create privilege escalations,
> > > > > > > so even if the fanotify fd is leaked or intentionally passed to a less
> > > > > > > privileged user, it cannot get an open pidfd.
> > > > > > >
> > > > > > > IOW, it is *much* easier to be defensive in this case than to prove
> > > > > > > that the change cannot introduce any privilege escalations.
> > > > > >
> > > > > > I have no problems with being more defensive (it's certainly better than
> > > > > > being too lax) but does it really make sence here? I mean if CAP_SYS_ADMIN
> > > > > > task opens O_RDWR /etc/passwd and then passes this fd to unpriviledged
> > > > > > process, that process is also free to update all the passwords.
> > > > > > Traditionally permission checks in Unix are performed on open and then who
> > > > > > has fd can do whatever that fd allows... I've tried to follow similar
> > > > > > philosophy with fanotify as well and e.g. open happening as a result of
> > > > > > fanotify path events does not check permissions either.
> > > > > >
> > > > >
> > > > > Agreed.
> > > > >
> > > > > However, because we had this issue with no explicit FAN_REPORT_PID
> > > > > we added the CAP_SYS_ADMIN check for reporting event->pid as next
> > > > > best thing. So now that becomes weird if priv process created fanotify fd
> > > > > and passes it to unpriv process, then unpriv process gets events with
> > > > > pidfd but without event->pid.
> > > > >
> > > > > We can change the code to:
> > > > >
> > > > >         if (!capable(CAP_SYS_ADMIN) && !pidfd_mode &&
> > > > >             task_tgid(current) != event->pid)
> > > > >                 metadata.pid = 0;
> > > > >
> > > > > So the case I decscribed above ends up reporting both pidfd
> > > > > and event->pid to unpriv user, but that is a bit inconsistent...
> > > >
> > > > Oh, now I see where you are coming from :) Thanks for explanation. And
> > > > remind me please, cannot we just have internal FAN_REPORT_PID flag that
> > > > gets set on notification group when priviledged process creates it and then
> > > > test for that instead of CAP_SYS_ADMIN in copy_event_to_user()? It is
> > > > mostly equivalent but I guess more in the spirit of how fanotify
> > > > traditionally does things. Also FAN_REPORT_PIDFD could then behave in the
> > > > same way...
> > >
> > > Yes, we can. In fact, we should call the internal flag FANOTIFY_UNPRIV
> > > as it described the situation better than FAN_REPORT_PID.
> > > This happens to be how I implemented it in the initial RFC [1].
> > >
> > > It's not easy to follow our entire discussion on this thread, but I think
> > > we can resurrect the FANOTIFY_UNPRIV internal flag and use it
> > > in this case instead of CAP_SYS_ADMIN.
> >
> > I think at that time we were discussing how to handle opening of fds and
> > we decided to not depend on FANOTIFY_UNPRIV and then I didn't see a value
> > of that flag because I forgot about pids... Anyway now I agree to go for
> > that flag. :)
>
> Resurrection of this flag SGTM! However, it also sounds like we need
> to land that series before this PIDFD series or simply incorporate the
> UNPRIV flag into this one.
>
> Will chat with Amir to get this done.

Let me post this patch as a fix patch to unprivileged group.

Thanks,
Amir.