quoted

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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.