Messages in this thread Patch in this message | | | Date | Wed, 31 Oct 2018 08:05:48 -0700 | From | Joel Fernandes <> | Subject | Re: [PATCH v2] Implement /proc/pid/kill |
| |
On Wed, Oct 31, 2018 at 02:37:44PM +0000, Daniel Colascione wrote: > Add a simple proc-based kill interface. To use /proc/pid/kill, just > write the signal number in base-10 ASCII to the kill file of the > process to be killed: for example, 'echo 9 > /proc/$$/kill'. > > Semantically, /proc/pid/kill works like kill(2), except that the > process ID comes from the proc filesystem context instead of from an > explicit system call parameter. This way, it's possible to avoid races > between inspecting some aspect of a process and that process's PID > being reused for some other process. > > Note that only the real user ID that opened a /proc/pid/kill file can > write to it; other users get EPERM. This check prevents confused > deputy attacks via, e.g., standard output of setuid programs. > > With /proc/pid/kill, it's possible to write a proper race-free and > safe pkill(1). An approximation follows. A real program might use > openat(2), having opened a process's /proc/pid directory explicitly, > with the directory file descriptor serving as a sort of "process > handle". > > #!/bin/bash > set -euo pipefail > pat=$1 > for proc_status in /proc/*/status; do ( > cd $(dirname $proc_status) > readarray proc_argv -d'' < cmdline > if ((${#proc_argv[@]} > 0)) && > [[ ${proc_argv[0]} = *$pat* ]]; > then > echo 15 > kill > fi > ) || true; done > > Signed-off-by: Daniel Colascione <dancol@google.com> > --- > > Added a real-user-ID check to prevent confused deputy attacks. > > fs/proc/base.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/fs/proc/base.c b/fs/proc/base.c > index 7e9f07bf260d..74e494f24b28 100644 > --- a/fs/proc/base.c > +++ b/fs/proc/base.c > @@ -205,6 +205,56 @@ static int proc_root_link(struct dentry *dentry, struct path *path) > return result; > } > > +static ssize_t proc_pid_kill_write(struct file *file, > + const char __user *buf, > + size_t count, loff_t *ppos) > +{ > + ssize_t res; > + int sig; > + char buffer[4]; > + > + /* This check prevents a confused deputy attack in which an > + * unprivileged process opens /proc/victim/kill and convinces > + * a privileged process to write to that kill FD, effectively > + * performing a kill with the privileges of the unwitting > + * privileged process. Here, we just fail the kill operation > + * if someone calls write(2) with a real user ID that differs > + * from the one used to open the kill FD. > + */ > + res = -EPERM; > + if (file->f_cred->user != current_user()) > + goto out;
nit: You could get rid of the out label and just do direct returns. Will save a few lines and is more readable.
> + > + res = -EINVAL; > + if (*ppos != 0) > + goto out; > + > + res = -EINVAL; > + if (count > sizeof(buffer) - 1) > + goto out; > + > + res = -EFAULT; > + if (copy_from_user(buffer, buf, count)) > + goto out; > + > + buffer[count] = '\0';
I think you can just zero-initialize buffer with "= {};" and get rid of this line.
> + res = kstrtoint(strstrip(buffer), 10, &sig); > + if (res) > + goto out;
> + > + res = kill_pid(proc_pid(file_inode(file)), sig, 0); > + if (res) > + goto out; if (res) return res; Other than the security issues which I still think you're discussing, since we need this, I suggest to maintainers we take this in as an intermediate solution since we don't have anything close to it and this is a real issue, and the fix proposed is simple. So FWIW feel free to add my reviewed-by (with the above nits and security issues taken care off) on any future respins:
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
thanks,
- Joel
| |