Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Daniel Colascione <dancol@google.com>
Cc:     linux-kernel@vger.kernel.org, timmurray@google.com,
        joelaf@google.com, surenb@google.com,
        Kees Cook <keescook@chromium.org>,
        Christian Brauner <christian.brauner@canonical.com>,
        Oleg Nesterov <oleg@redhat.com>
References: <20181029221037.87724-1-dancol@google.com>
Date:   Tue, 30 Oct 2018 23:44:50 -0500
In-Reply-To: <20181029221037.87724-1-dancol@google.com> (Daniel Colascione's
        message of "Mon, 29 Oct 2018 22:10:37 +0000")
Message-ID: <87bm7a3et9.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [RFC PATCH] Implement /proc/pid/kill
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Daniel Colascione <dancol@google.com> writes:

> 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.
>
> 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
>

In general this looks good.

Unfortunately the permission checks are are subject to a serious
problem.  Even if I don't have permission to kill a process I quite
likely will be allowed to open the file.

Then I just need to find a setuid or setcap executable will write to
stdout or stderr a number.  Then I have killed something I should not
have the privileges to kill.

At a bare minimum you need to perform the permission check using the
credentials of the opener of the file.    Which means refactoring
kill_pid so that you can perform the permission check for killing the
application during open.  Given that process credentials can change
completely during exec you also need to rule out a change in process
credentials making it so that the original opener of the file would not
be able to kill the process as it is now.

But overall this looks quite reasaonble.

Eric

> Signed-off-by: Daniel Colascione <dancol@google.com>
> ---
>  fs/proc/base.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 7e9f07bf260d..923d62b21e67 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -205,6 +205,44 @@ 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];
> +
> +	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';
> +	res = kstrtoint(strstrip(buffer), 10, &sig);
> +	if (res)
> +		goto out;
> +
> +	res = kill_pid(proc_pid(file_inode(file)), sig, 0);
> +	if (res)
> +		goto out;
> +	res = count;
> +out:
> +	return res;
> +
> +}
> +
> +static const struct file_operations proc_pid_kill_ops = {
> +	.write	= proc_pid_kill_write,
> +};
> +
>  static ssize_t get_mm_cmdline(struct mm_struct *mm, char __user *buf,
>  			      size_t count, loff_t *ppos)
>  {
> @@ -2935,6 +2973,7 @@ static const struct pid_entry tgid_base_stuff[] = {
>  #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
>  	ONE("syscall",    S_IRUSR, proc_pid_syscall),
>  #endif
> +	REG("kill",       S_IRUGO | S_IWUGO, proc_pid_kill_ops),
>  	REG("cmdline",    S_IRUGO, proc_pid_cmdline_ops),
>  	ONE("stat",       S_IRUGO, proc_tgid_stat),
>  	ONE("statm",      S_IRUGO, proc_pid_statm),