Re: [PATCH v2] Implement /proc/pid/kill

[Joel Fernandes]

On Thu, Nov 01, 2018 at 04:33:53AM +1100, Aleksa Sarai wrote:
> On 2018-10-31, Joel Fernandes  wrote:
> > 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.
> 
> I would suggest we wait until after LPC to see what Christian's design
> is (given that, from what I've heard, it will help us solve more
> problems than just the kill(2) issue).
> 
> I am very skeptical of adding new procfs files as an "intermediate
> solution" (procfs is the most obvious example of an interface which is
> effectively written in stone). Especially if this would conflict with
> the idea Christian will propose -- as he said, there were proposals to
> do this in the past and they didn't get anywhere because of lack of
> discussion and brainstorming before posting patches.
> 

Fine with me, thanks.

Re: [PATCH v2] Implement /proc/pid/kill

[Joel Fernandes]

On Thu, Nov 01, 2018 at 04:33:53AM +1100, Aleksa Sarai wrote:
> On 2018-10-31, Joel Fernandes  wrote:
> > 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.
> 
> I would suggest we wait until after LPC to see what Christian's design
> is (given that, from what I've heard, it will help us solve more
> problems than just the kill(2) issue).
> 
> I am very skeptical of adding new procfs files as an "intermediate
> solution" (procfs is the most obvious example of an interface which is
> effectively written in stone). Especially if this would conflict with
> the idea Christian will propose -- as he said, there were proposals to
> do this in the past and they didn't get anywhere because of lack of
> discussion and brainstorming before posting patches.
> 

Fine with me, thanks.

Re: [PATCH v2] Implement /proc/pid/kill

[Aleksa Sarai]

On 2018-10-31, Joel Fernandes  wrote:
> 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.

I would suggest we wait until after LPC to see what Christian's design
is (given that, from what I've heard, it will help us solve more
problems than just the kill(2) issue).

I am very skeptical of adding new procfs files as an "intermediate
solution" (procfs is the most obvious example of an interface which is
effectively written in stone). Especially if this would conflict with
the idea Christian will propose -- as he said, there were proposals to
do this in the past and they didn't get anywhere because of lack of
discussion and brainstorming before posting patches.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH



signature.asc
Description: PGP signature

Re: [PATCH v2] Implement /proc/pid/kill

[Aleksa Sarai]

On 2018-10-31, Joel Fernandes  wrote:
> 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.

I would suggest we wait until after LPC to see what Christian's design
is (given that, from what I've heard, it will help us solve more
problems than just the kill(2) issue).

I am very skeptical of adding new procfs files as an "intermediate
solution" (procfs is the most obvious example of an interface which is
effectively written in stone). Especially if this would conflict with
the idea Christian will propose -- as he said, there were proposals to
do this in the past and they didn't get anywhere because of lack of
discussion and brainstorming before posting patches.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH



signature.asc
Description: PGP signature

Re: [PATCH v2] Implement /proc/pid/kill

[Joel Fernandes]

On Wed, Oct 31, 2018 at 02:37:44PM +, 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 
> ---
> 
> 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, );
> + 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) 

thanks,

- Joel

Re: [PATCH v2] Implement /proc/pid/kill

[Joel Fernandes]

On Wed, Oct 31, 2018 at 02:37:44PM +, 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 
> ---
> 
> 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, );
> + 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) 

thanks,

- Joel