vnode filter uses /proc/PID/fd# to lookup filename mappings. This is suboptimal as the process won't always have permissions to access /proc

[arr2036]

This main issue here is that despite its name, inotify uses paths instead of inodes to identify files and directories to watch for events on.

When a vnode filter knote is added, the fd is resolved to the path name using linux_fd_to_path.

/*
 * Given a file descriptor, return the path to the file it refers to.
 */
int
linux_fd_to_path(char *buf, size_t bufsz, int fd)
{
    char path[1024];    //TODO: Maxpathlen, etc.

    if (snprintf(&path[0], sizeof(path), "/proc/%d/fd/%d", getpid(), fd) < 0)
        return (-1);

    memset(buf, 0, bufsz);
    return (readlink(path, buf, bufsz));
}

For files, this really does seem to be the only way to get the filename, but for directories, we can use fdopendir() -> readdir() -> (struct dirent)-> d_name, which uses internal kernel interfaces and means we don't have to access proc.

As the majority of the time vnode filters are used to watch directories, it's probably worth adding this to linux_fd_to_path() using fstat() st_mode & S_IFDIR to identify directories.

This will have a negative performance impact, but as the resolution is only done once when the knote is created, and not on every event, I think this is likely acceptable.

[mheily]

I don't see how this would work. The d_name field from readdir() gives you the filename, but how do you know the full path to the directory? The inotify_add_watch() function requires the full path, not just the filename.

…

On Tue, Apr 2, 2019 at 11:26 AM Arran Cudbard-Bell ***@***.***> wrote: This main issue here is that despite its name, inotify uses paths instead of inodes to identify files and directories to watch for events on. When a vnode filter knote is added, the fd is resolved to the path name using linux_fd_to_path. /* * Given a file descriptor, return the path to the file it refers to. */ int linux_fd_to_path(char *buf, size_t bufsz, int fd) { char path[1024]; //TODO: Maxpathlen, etc. if (snprintf(&path[0], sizeof(path), "/proc/%d/fd/%d", getpid(), fd) < 0) return (-1); memset(buf, 0, bufsz); return (readlink(path, buf, bufsz)); } For files, this really does seem to be the only way to get the filename, but for directories, we can use fdopendir() -> readdir() -> (struct dirent)-> d_name, which uses internal kernel interfaces and means we don't have to access proc. As the majority of the time vnode filters are used to watch directories, it's probably worth adding this to linux_fd_to_path() using fstat() st_mode & S_IFDIR to identify directories. This will have a negative performance impact, but as the resolution is only done once when the knote is created, and not on every event, I think this is likely acceptable. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#62>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGUw4IUlmdEW7U5CbIZYBD9VY7YywBFkks5vc3acgaJpZM4cYVdS> .

[arr2036]

I didn't realise that, it wasn't clear from the man pages. I guess the buffer size of 256 bytes should have been a hint.

[arr2036]

Two other options:

• When libkqueue is initialised see if uid == 0. This would be done before the application drops root privileges. libkqueue would then communicate with the forked process using a pipe, and use that process to access /proc. As seteuid/gid would only affect the original process, not the child process, the child would retain access to /proc. sendmsg and recvmsg would need to be used to perform FD transfer between the two processes. If we still retained access to /proc later, we could avoid the overhead messaging the child process, and just do resolution in the main process.
• Spawn persistent child processes, one for each thread, then use fchdir and getcwd in the child process to do FD to directory resolution. As with the first option, communication would be with a pipe. This would only work for directories.

[mheily]

What about adding a function to the API that lets the application associate the path with the fd? Something like: kqueue_set_vnode_path(kqfd, ident, path) And then storing this in struct knote? Then libkqueue could skip the /proc lookup entirely, if this optional path is provided

…

On Wed, Apr 3, 2019, 10:28 AM Arran Cudbard-Bell ***@***.***> wrote: Two other options: - When libkqueue is initialised see if uid == 0. This would be done before the application drops root privileges. libkqueue would then communicate with the forked process using a pipe, and use that process to access /proc. As seteuid/gid would only affect the original process, not the child process, the child would retain access to /proc. sendmsg and recvmsg would need to be used to perform FD transfer between the two processes. If we still retained access to /proc later, we could avoid the overhead messaging the child process, and just do resolution in the main process. - Spawn persistent child processes, one for each thread, then use fchdir and getcwd in the child process to do FD to directory resolution. As with the first option, communication would be with a pipe. This would only work for directories. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#62 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGUw4FH-OIHjrEq1_Dno1iJxWz2FPSB3ks5vdLqLgaJpZM4cYVdS> .

[arr2036]

Yes we could do. I just wanted as much as possible to avoid needing extra calls in the application, it really diminishes the value of libkqueue.

Currently looking to see if there's a way of accessing the /proc that doesn't require PR_SET_DUMPABLE.

[mheily]

Another option is doing open() of /proc during libkqueue_init(), saving a copy of the descriptor, and then changing readlink() to readlinkat() to use the saved copy. I'm also not exactly clear on the symptoms of the permissions issue. What combination of syscalls are needed to reproduce the problem?

…

On Thu, Apr 4, 2019, 2:03 PM Arran Cudbard-Bell ***@***.***> wrote: Yeah we could do. I just wanted as much as possible to avoid needing extra calls in the application, it really diminishes the value of libkqueue. Currently looking to see if there's a way of accessing the /proc that doesn't require PR_SET_DUMPABLE. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#62 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGUw4Ld37wt3EKdXDiQIkPfMmPoMP7KYks5vdj5_gaJpZM4cYVdS> .

[arr2036]

Excellent idea! I'll see about implementing that.

The scenario here is when the process starts as root and then calls seteuid to change to another user with fewer privileges. It's a pretty common thing for daemons to do, which is why I wanted to try and get decent fix.

Unless PR_SET_DUMPABLE is set by the application, /proc will only be owned by root, so libkqueue will be unable to access it after seteuid is called.

[mheily]

I still don't understand how this causes a problem. The mode of /proc is 0555, so what does it matter that root owns it? I assume that after setuid() is called, the owner of /proc/$pid is changed to match the effective uid.. is that the case?

Would you be willing to write a unit test that reproduces the problem? Or can you provide the source code to the application that is exhibiting the issue?

[arr2036]

Yes, definitely. I'm working on second hand information, another contributor actually reported the issue and developed a fix within our application, so it may not be an issue with accessing the /proc directory, but maybe an issue accessing something else in that file system.

[arr2036]

Though I don't believe /proc/$pid is changed to match the effective UID unless PR_SET_DUMPABLE is set.

man 5 proc

The files inside each /proc/[pid] directory are normally owned
by the effective user and effective group ID of the process.
However, as a security measure, the ownership is made
root:root if the process's "dumpable" attribute is set to a
value other than 1.

[mheily]

In that case, you might want to open() /proc/self/fd instead of /proc, prior to seteuid()

…

On Fri, Apr 5, 2019, 6:53 PM Arran Cudbard-Bell ***@***.***> wrote: Though I don't believe /proc/$pid is changed to match the effective UID unless PR_SET_DUMPABLE is set. man 5 proc The files inside each /proc/[pid] directory are normally owned by the effective user and effective group ID of the process. However, as a security measure, the ownership is made root:root if the process's "dumpable" attribute is set to a value other than 1. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#62 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGUw4F8A04p5oJDOp0QcraAs-NpmGhbIks5vd9PggaJpZM4cYVdS> .