On Wed, Feb 24, 2021 at 12:52 PM Jan Kara [off-list ref] wrote:

quoted

On Tue 23-02-21 19:16:40, Amir Goldstein wrote:

quoted

On Fri, Feb 19, 2021 at 6:16 PM Amir Goldstein [off-list ref] wrote:

quoted

On Tue, Feb 16, 2021 at 8:12 PM Amir Goldstein [off-list ref] wrote:

quoted

On Tue, Feb 16, 2021 at 7:01 PM Jan Kara [off-list ref] wrote:

quoted

On Sun 24-01-21 20:42:04, Amir Goldstein wrote:

quoted

Add limited support for unprivileged fanotify event listener.
An unprivileged event listener does not get an open file descriptor in
the event nor the process pid of another process.  An unprivileged event
listener cannot request permission events, cannot set mount/filesystem
marks and cannot request unlimited queue/marks.

This enables the limited functionality similar to inotify when watching a
set of files and directories for OPEN/ACCESS/MODIFY/CLOSE events, without
requiring SYS_CAP_ADMIN privileges.

The FAN_REPORT_DFID_NAME init flag, provide a method for an unprivileged
event listener watching a set of directories (with FAN_EVENT_ON_CHILD)
to monitor all changes inside those directories.

This typically requires that the listener keeps a map of watched directory
fid to dirfd (O_PATH), where fid is obtained with name_to_handle_at()
before starting to watch for changes.

When getting an event, the reported fid of the parent should be resolved
to dirfd and fstatsat(2) with dirfd and name should be used to query the
state of the filesystem entry.

Note that even though events do not report the event creator pid,
fanotify does not merge similar events on the same object that were
generated by different processes. This is aligned with exiting behavior
when generating processes are outside of the listener pidns (which
results in reporting 0 pid to listener).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks mostly good to me. Just two questions:

a) Remind me please, why did we decide pid isn't safe to report to
unpriviledged listeners?

Just because the information that process X modified file Y is not an
information that user can generally obtain without extra capabilities(?)
I can add a flag FAN_REPORT_OWN_PID to make that behavior
explicit and then we can relax reporting pids later.

FYI a patch for flag FAN_REPORT_SELF_PID is pushed to branch
fanotify_unpriv.

The UAPI feels a bit awkward with this flag, but that is the easiest way
to start without worrying about disclosing pids.

I guess we can require that unprivileged listener has pid 1 in its own
pid ns. The outcome is similar to FAN_REPORT_SELF_PID, except
it can also get pids of its children which is probably fine.

Jan,

WRT your comment in github:
"So maybe we can just require that this flag is already set by userspace
instead of silently setting it? Like:

if (!(flags & FAN_REPORT_SELF_PID)) return -EPERM;

I'd say that variant is more futureproof and the difference for user
is minimal."

I started with this approach and then I wrote the tests and imagined
the man page
requiring this flag would be a bit awkward, so I changed it to auto-enable.

I am not strongly against the more implicit flag requirement, but in
favor of the
auto-enable approach I would like to argue that with current fanotify you CAN
get zero pid in event, so think about it this way:
If a listener is started in (or moved into) its own pid ns, it will
get zero pid in all
events (other than those generated by itself and its own children).

With the proposed change, the same applies also if the listener is started
without CAP_SYS_ADMIN.

As a matter of fact, we do not need the flag at all, we can determine whether
or not to report pid according to capabilities of the event reader at
event read time.
And we can check for one of:
- CAP_SYS_ADMIN
- CAP_SYS_PACCT
- CAP_SYS_PTRACE

Do you prefer this flag-less approach?

Well, I don't have strong opinion what we should do internally either. The
flag seems OK to me. The biggest question is whether we should expose the
FAN_REPORT_SELF_PID flag to userspace or not. If we would not require
explicit flag for unpriv users, I see little reason to expose that flag at
all.

IMO the only listeners that actually care about event->pid are permission
event listeners. I think that FAN_CLASS_NOTIF listeners do not care
about it. The only thing that *I* ever used event->pid for is to ignore events
from self pid.

My question is, do you mind if we start with this code:

@@ -419,6 +419,14 @@ static ssize_t copy_event_to_user(struct

fsnotify_group *group,
        metadata.reserved = 0;
        metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
        metadata.pid = pid_vnr(event->pid);
+
+        /*
+         * For an unprivileged listener, event->pid can be used to identify the
+         * events generated by the listener process itself, without disclosing
+         * the pids of other processes.
+         */
+        if (!capable(CAP_SYS_ADMIN) &&
+            task_tgid(current) != event->pid)
+                metadata.pid = 0;

No need for any visible or invisible user flags.