Re: [PATCH v3] Implement /proc/pid/kill
David Laight writes: > From: Daniel Colascione >> Sent: 31 October 2018 19:33 > ... >> You can't do it today with kill. The idea that keeping a open file >> descriptor to a /proc/pid or a file within it prevents PID reuse is >> widespread, but incorrect. > > Is there a real good reason why that shouldn't be the case? > ie Holding a reference on the 'struct pid' being enough to stop reuse. > > A patch to do that would be more generally useful. Holding an open file descriptor to /proc/pid is enough to prevent pid reuse problems. If a pid number is reused a new 'struct pid' is generated. There is not 'struct pid' reuse. So in solving this the kernel data structure you would need to hold onto is a 'struct pid'. It is just necessary to add an interface to sending signals to that 'struct pid' and not looking up the 'struct pid' again by number. Eric
Re: [PATCH v3] Implement /proc/pid/kill
David Laight writes: > From: Daniel Colascione >> Sent: 31 October 2018 19:33 > ... >> You can't do it today with kill. The idea that keeping a open file >> descriptor to a /proc/pid or a file within it prevents PID reuse is >> widespread, but incorrect. > > Is there a real good reason why that shouldn't be the case? > ie Holding a reference on the 'struct pid' being enough to stop reuse. > > A patch to do that would be more generally useful. Holding an open file descriptor to /proc/pid is enough to prevent pid reuse problems. If a pid number is reused a new 'struct pid' is generated. There is not 'struct pid' reuse. So in solving this the kernel data structure you would need to hold onto is a 'struct pid'. It is just necessary to add an interface to sending signals to that 'struct pid' and not looking up the 'struct pid' again by number. Eric
RE: [PATCH v3] Implement /proc/pid/kill
From: Daniel Colascione > Sent: 31 October 2018 19:33 ... > You can't do it today with kill. The idea that keeping a open file > descriptor to a /proc/pid or a file within it prevents PID reuse is > widespread, but incorrect. Is there a real good reason why that shouldn't be the case? ie Holding a reference on the 'struct pid' being enough to stop reuse. A patch to do that would be more generally useful. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
RE: [PATCH v3] Implement /proc/pid/kill
From: Daniel Colascione > Sent: 31 October 2018 19:33 ... > You can't do it today with kill. The idea that keeping a open file > descriptor to a /proc/pid or a file within it prevents PID reuse is > widespread, but incorrect. Is there a real good reason why that shouldn't be the case? ie Holding a reference on the 'struct pid' being enough to stop reuse. A patch to do that would be more generally useful. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 07:33:06PM +, Daniel Colascione wrote: > On Wed, Oct 31, 2018 at 6:17 PM, Tycho Andersen wrote: > > On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: > >> On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > >> > Why not just use an ioctl() like Jann suggested instead of this big > >> > security check? Then we avoid the whole setuid writer thing entirely, > >> > >> Don't you think a system call would be better than a new ioctl? > > > > We already have a kill() system call :) > > kill(2) is useless this purpose: it accepts a numeric PID, but we'd > need it to accept a process file descriptor instead. It's true that > the existing kill(1) binary might be the vehicle for using a > hypothetical new system call, but that's a separate matter. > > >> With either an ioctl or a new system call, though, the shell would > >> need a helper program to use the facility, whereas with the existing > >> approach, the shell can use the new facility without any additional > >> binaries. > > > > ...and a binary to use it! > > > > The nice thing about an ioctl is that it avoids this class of attacks > > entirely. > > Let's stop talking about adding an ioctl. Ioctls have problems with > namespacing of the request argument; it's not safe, in general, to > issue an ioctl against a file descriptor of an unknown type. So don't lose track of the fd type. I'm not sure I see this as a big problem. > You don't know how that FD will interpret your request code. The two > good options before us are a write(2) interface and a new system > call. I think both are defensible. But I don't see a good reason to > consider adding an ioctl instead of a system call. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1729911.html https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1729921.html maybe? :) > All of this is moot if the new comprehensive process interface that > comes out of LPC ends up being better anyway. +1, I think a way to do all of this sort of thing would be nice. > > either. Using this from the shell is still racy, because if I do > > something like: > > > > echo 9 > /proc/$pid/kill > > > > There's exactly the same race that there is with kill, that $pid might > > be something else. > > > Of course I could do some magic with bind mounts or > > my pwd or something to keep it alive, but I can already do that today > > with kill. > > You can't do it today with kill. The idea that keeping a open file > descriptor to a /proc/pid or a file within it prevents PID reuse is > widespread, but incorrect. Good to know :) Tycho
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 07:33:06PM +, Daniel Colascione wrote: > On Wed, Oct 31, 2018 at 6:17 PM, Tycho Andersen wrote: > > On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: > >> On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > >> > Why not just use an ioctl() like Jann suggested instead of this big > >> > security check? Then we avoid the whole setuid writer thing entirely, > >> > >> Don't you think a system call would be better than a new ioctl? > > > > We already have a kill() system call :) > > kill(2) is useless this purpose: it accepts a numeric PID, but we'd > need it to accept a process file descriptor instead. It's true that > the existing kill(1) binary might be the vehicle for using a > hypothetical new system call, but that's a separate matter. > > >> With either an ioctl or a new system call, though, the shell would > >> need a helper program to use the facility, whereas with the existing > >> approach, the shell can use the new facility without any additional > >> binaries. > > > > ...and a binary to use it! > > > > The nice thing about an ioctl is that it avoids this class of attacks > > entirely. > > Let's stop talking about adding an ioctl. Ioctls have problems with > namespacing of the request argument; it's not safe, in general, to > issue an ioctl against a file descriptor of an unknown type. So don't lose track of the fd type. I'm not sure I see this as a big problem. > You don't know how that FD will interpret your request code. The two > good options before us are a write(2) interface and a new system > call. I think both are defensible. But I don't see a good reason to > consider adding an ioctl instead of a system call. https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1729911.html https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1729921.html maybe? :) > All of this is moot if the new comprehensive process interface that > comes out of LPC ends up being better anyway. +1, I think a way to do all of this sort of thing would be nice. > > either. Using this from the shell is still racy, because if I do > > something like: > > > > echo 9 > /proc/$pid/kill > > > > There's exactly the same race that there is with kill, that $pid might > > be something else. > > > Of course I could do some magic with bind mounts or > > my pwd or something to keep it alive, but I can already do that today > > with kill. > > You can't do it today with kill. The idea that keeping a open file > descriptor to a /proc/pid or a file within it prevents PID reuse is > widespread, but incorrect. Good to know :) Tycho
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 6:17 PM, Tycho Andersen wrote: > On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: >> On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: >> > Why not just use an ioctl() like Jann suggested instead of this big >> > security check? Then we avoid the whole setuid writer thing entirely, >> >> Don't you think a system call would be better than a new ioctl? > > We already have a kill() system call :) kill(2) is useless this purpose: it accepts a numeric PID, but we'd need it to accept a process file descriptor instead. It's true that the existing kill(1) binary might be the vehicle for using a hypothetical new system call, but that's a separate matter. >> With either an ioctl or a new system call, though, the shell would >> need a helper program to use the facility, whereas with the existing >> approach, the shell can use the new facility without any additional >> binaries. > > ...and a binary to use it! > > The nice thing about an ioctl is that it avoids this class of attacks > entirely. Let's stop talking about adding an ioctl. Ioctls have problems with namespacing of the request argument; it's not safe, in general, to issue an ioctl against a file descriptor of an unknown type. You don't know how that FD will interpret your request code. The two good options before us are a write(2) interface and a new system call. I think both are defensible. But I don't see a good reason to consider adding an ioctl instead of a system call. >> > and we can pass the fd around if we want to. >> >> You can pass the FD around today --- specifically, you just pass the >> /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid >> directory FD acts as a process handle. (It's literally a reference to >> a struct pid.) Anyone who receives one of these process handle FDs and >> who wants to use the corresponding kill file can open the kill fd with >> openat(2). What you can't do is pass the /proc/pid/kill FD to another >> security context and use it, but when would you ever want to do that? > > Perhaps I don't have a good imagination, because it's not clear to me > when I'd ever use this from a shell instead of the kill binary, I'm not against a new system call per se; I'd just prefer a write(2) interface if we can get away with it. The trouble with a system call is that it would have to accept a /proc/pid file descriptor, and file descriptor management in the shell is awkward. I imagine the interface would look something like kill -f PATH, where PATH is a PATH to a /proc/pid directory. You'd be able to cd into /proc/$SOMETHING, inspect state, and then, if you wanted to kill the process, you'd run kill -f . 9 (or whatever signal number you want). It seems to be about as ergonomic as 'echo 9 > ./kill'. But a new system call means new kernel headers, coordination with procps, and bash, and every other shell that has a kill builtin. You could provide a different, non-kill binary, but then who'd distribute it? A new proc file, OTOH, would Just Work. I agree that a system call interface would avoid the need for the security check thing in the patch, but is avoiding this check worth the coordination flowing from adding a new system call? I don't know. All of this is moot if the new comprehensive process interface that comes out of LPC ends up being better anyway. > either. Using this from the shell is still racy, because if I do > something like: > > echo 9 > /proc/$pid/kill > > There's exactly the same race that there is with kill, that $pid might > be something else. > Of course I could do some magic with bind mounts or > my pwd or something to keep it alive, but I can already do that today > with kill. You can't do it today with kill. The idea that keeping a open file descriptor to a /proc/pid or a file within it prevents PID reuse is widespread, but incorrect.
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 6:17 PM, Tycho Andersen wrote: > On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: >> On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: >> > Why not just use an ioctl() like Jann suggested instead of this big >> > security check? Then we avoid the whole setuid writer thing entirely, >> >> Don't you think a system call would be better than a new ioctl? > > We already have a kill() system call :) kill(2) is useless this purpose: it accepts a numeric PID, but we'd need it to accept a process file descriptor instead. It's true that the existing kill(1) binary might be the vehicle for using a hypothetical new system call, but that's a separate matter. >> With either an ioctl or a new system call, though, the shell would >> need a helper program to use the facility, whereas with the existing >> approach, the shell can use the new facility without any additional >> binaries. > > ...and a binary to use it! > > The nice thing about an ioctl is that it avoids this class of attacks > entirely. Let's stop talking about adding an ioctl. Ioctls have problems with namespacing of the request argument; it's not safe, in general, to issue an ioctl against a file descriptor of an unknown type. You don't know how that FD will interpret your request code. The two good options before us are a write(2) interface and a new system call. I think both are defensible. But I don't see a good reason to consider adding an ioctl instead of a system call. >> > and we can pass the fd around if we want to. >> >> You can pass the FD around today --- specifically, you just pass the >> /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid >> directory FD acts as a process handle. (It's literally a reference to >> a struct pid.) Anyone who receives one of these process handle FDs and >> who wants to use the corresponding kill file can open the kill fd with >> openat(2). What you can't do is pass the /proc/pid/kill FD to another >> security context and use it, but when would you ever want to do that? > > Perhaps I don't have a good imagination, because it's not clear to me > when I'd ever use this from a shell instead of the kill binary, I'm not against a new system call per se; I'd just prefer a write(2) interface if we can get away with it. The trouble with a system call is that it would have to accept a /proc/pid file descriptor, and file descriptor management in the shell is awkward. I imagine the interface would look something like kill -f PATH, where PATH is a PATH to a /proc/pid directory. You'd be able to cd into /proc/$SOMETHING, inspect state, and then, if you wanted to kill the process, you'd run kill -f . 9 (or whatever signal number you want). It seems to be about as ergonomic as 'echo 9 > ./kill'. But a new system call means new kernel headers, coordination with procps, and bash, and every other shell that has a kill builtin. You could provide a different, non-kill binary, but then who'd distribute it? A new proc file, OTOH, would Just Work. I agree that a system call interface would avoid the need for the security check thing in the patch, but is avoiding this check worth the coordination flowing from adding a new system call? I don't know. All of this is moot if the new comprehensive process interface that comes out of LPC ends up being better anyway. > either. Using this from the shell is still racy, because if I do > something like: > > echo 9 > /proc/$pid/kill > > There's exactly the same race that there is with kill, that $pid might > be something else. > Of course I could do some magic with bind mounts or > my pwd or something to keep it alive, but I can already do that today > with kill. You can't do it today with kill. The idea that keeping a open file descriptor to a /proc/pid or a file within it prevents PID reuse is widespread, but incorrect.
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: > On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > > Why not just use an ioctl() like Jann suggested instead of this big > > security check? Then we avoid the whole setuid writer thing entirely, > > Don't you think a system call would be better than a new ioctl? We already have a kill() system call :) > With either an ioctl or a new system call, though, the shell would > need a helper program to use the facility, whereas with the existing > approach, the shell can use the new facility without any additional > binaries. ...and a binary to use it! The nice thing about an ioctl is that it avoids this class of attacks entirely. > > and we can pass the fd around if we want to. > > You can pass the FD around today --- specifically, you just pass the > /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid > directory FD acts as a process handle. (It's literally a reference to > a struct pid.) Anyone who receives one of these process handle FDs and > who wants to use the corresponding kill file can open the kill fd with > openat(2). What you can't do is pass the /proc/pid/kill FD to another > security context and use it, but when would you ever want to do that? Perhaps I don't have a good imagination, because it's not clear to me when I'd ever use this from a shell instead of the kill binary, either. Using this from the shell is still racy, because if I do something like: echo 9 > /proc/$pid/kill There's exactly the same race that there is with kill, that $pid might be something else. Of course I could do some magic with bind mounts or my pwd or something to keep it alive, but I can already do that today with kill. I can understand the desire to have a race free way to do this, but "it must use write(2)" seems a little unnecessary, given that the shell use case isn't particularly convincing to me. Tycho
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 06:00:49PM +, Daniel Colascione wrote: > On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > > Why not just use an ioctl() like Jann suggested instead of this big > > security check? Then we avoid the whole setuid writer thing entirely, > > Don't you think a system call would be better than a new ioctl? We already have a kill() system call :) > With either an ioctl or a new system call, though, the shell would > need a helper program to use the facility, whereas with the existing > approach, the shell can use the new facility without any additional > binaries. ...and a binary to use it! The nice thing about an ioctl is that it avoids this class of attacks entirely. > > and we can pass the fd around if we want to. > > You can pass the FD around today --- specifically, you just pass the > /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid > directory FD acts as a process handle. (It's literally a reference to > a struct pid.) Anyone who receives one of these process handle FDs and > who wants to use the corresponding kill file can open the kill fd with > openat(2). What you can't do is pass the /proc/pid/kill FD to another > security context and use it, but when would you ever want to do that? Perhaps I don't have a good imagination, because it's not clear to me when I'd ever use this from a shell instead of the kill binary, either. Using this from the shell is still racy, because if I do something like: echo 9 > /proc/$pid/kill There's exactly the same race that there is with kill, that $pid might be something else. Of course I could do some magic with bind mounts or my pwd or something to keep it alive, but I can already do that today with kill. I can understand the desire to have a race free way to do this, but "it must use write(2)" seems a little unnecessary, given that the shell use case isn't particularly convincing to me. Tycho
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > Why not just use an ioctl() like Jann suggested instead of this big > security check? Then we avoid the whole setuid writer thing entirely, Don't you think a system call would be better than a new ioctl? With either an ioctl or a new system call, though, the shell would need a helper program to use the facility, whereas with the existing approach, the shell can use the new facility without any additional binaries. > and we can pass the fd around if we want to. You can pass the FD around today --- specifically, you just pass the /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid directory FD acts as a process handle. (It's literally a reference to a struct pid.) Anyone who receives one of these process handle FDs and who wants to use the corresponding kill file can open the kill fd with openat(2). What you can't do is pass the /proc/pid/kill FD to another security context and use it, but when would you ever want to do that?
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen wrote: > Why not just use an ioctl() like Jann suggested instead of this big > security check? Then we avoid the whole setuid writer thing entirely, Don't you think a system call would be better than a new ioctl? With either an ioctl or a new system call, though, the shell would need a helper program to use the facility, whereas with the existing approach, the shell can use the new facility without any additional binaries. > and we can pass the fd around if we want to. You can pass the FD around today --- specifically, you just pass the /proc/pid directory FD, not the /proc/pid/kill FD. The /proc/pid directory FD acts as a process handle. (It's literally a reference to a struct pid.) Anyone who receives one of these process handle FDs and who wants to use the corresponding kill file can open the kill fd with openat(2). What you can't do is pass the /proc/pid/kill FD to another security context and use it, but when would you ever want to do that?
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 03:59:12PM +, 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 the write(2) to the kill file descriptor works only if it
> happens in the security context as the call to open(2), where
> "security context" is defined as the set of all ambient user IDs
> (effective uid, fs uid, real uid, and saved uid) as well as the
> presence of the CAP_KILL capability. This check prevents confused
> deputy attacks via, e.g., supplying a /proc/$(pidof httpd)/kill file
> descriptor as the standard output of setuid program and convincing
> that program to write a "9".
>
> 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
> ---
>
> Turns out that checking struct user isn't sufficient, since signal.c's
> permissions check also cares about effective UIDs. Let's be
> extra-paranoid and bail if _anything_ relevant in struct cred
> has changed.
>
> Also, as Joel suggested, switch from goto-return to direct return.
>
> fs/proc/base.c | 67 ++
> 1 file changed, 67 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..b0e7ded96af9 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -205,6 +205,72 @@ 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];
> + const struct cred *cur_cred;
> + const struct cred *open_cred;
> + bool security_changed;
> +
> + /* 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.
> + */
> + cur_cred = current_cred();
> + open_cred = file->f_cred;
> + security_changed =
> + cur_cred->user_ns != open_cred->user_ns ||
> + !uid_eq(cur_cred->euid, open_cred->euid) ||
> + !uid_eq(cur_cred->fsuid, open_cred->fsuid) ||
> + !uid_eq(cur_cred->suid, open_cred->suid) ||
> + !uid_eq(cur_cred->uid, open_cred->uid) ||
> + /* No audit: if we actually use the capability, we'll
> + * audit during the actual kill. Here, we're just
> + * checking whether our kill-FD has escaped its
> + * original security context and bailing if it has.
> + */
> + (security_capable_noaudit(cur_cred,
> + cur_cred->user_ns,
> + CAP_KILL)
> + != security_capable_noaudit(open_cred,
> + open_cred->user_ns,
> + CAP_KILL));
> + if (security_changed)
> + return -EPERM;
Why not just use an ioctl() like Jann suggested instead of this big
security check? Then we avoid the whole setuid writer thing entirely,
and we can pass the fd around if we want to.
Tycho
Re: [PATCH v3] Implement /proc/pid/kill
On Wed, Oct 31, 2018 at 03:59:12PM +, 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 the write(2) to the kill file descriptor works only if it
> happens in the security context as the call to open(2), where
> "security context" is defined as the set of all ambient user IDs
> (effective uid, fs uid, real uid, and saved uid) as well as the
> presence of the CAP_KILL capability. This check prevents confused
> deputy attacks via, e.g., supplying a /proc/$(pidof httpd)/kill file
> descriptor as the standard output of setuid program and convincing
> that program to write a "9".
>
> 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
> ---
>
> Turns out that checking struct user isn't sufficient, since signal.c's
> permissions check also cares about effective UIDs. Let's be
> extra-paranoid and bail if _anything_ relevant in struct cred
> has changed.
>
> Also, as Joel suggested, switch from goto-return to direct return.
>
> fs/proc/base.c | 67 ++
> 1 file changed, 67 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..b0e7ded96af9 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -205,6 +205,72 @@ 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];
> + const struct cred *cur_cred;
> + const struct cred *open_cred;
> + bool security_changed;
> +
> + /* 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.
> + */
> + cur_cred = current_cred();
> + open_cred = file->f_cred;
> + security_changed =
> + cur_cred->user_ns != open_cred->user_ns ||
> + !uid_eq(cur_cred->euid, open_cred->euid) ||
> + !uid_eq(cur_cred->fsuid, open_cred->fsuid) ||
> + !uid_eq(cur_cred->suid, open_cred->suid) ||
> + !uid_eq(cur_cred->uid, open_cred->uid) ||
> + /* No audit: if we actually use the capability, we'll
> + * audit during the actual kill. Here, we're just
> + * checking whether our kill-FD has escaped its
> + * original security context and bailing if it has.
> + */
> + (security_capable_noaudit(cur_cred,
> + cur_cred->user_ns,
> + CAP_KILL)
> + != security_capable_noaudit(open_cred,
> + open_cred->user_ns,
> + CAP_KILL));
> + if (security_changed)
> + return -EPERM;
Why not just use an ioctl() like Jann suggested instead of this big
security check? Then we avoid the whole setuid writer thing entirely,
and we can pass the fd around if we want to.
Tycho
