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;
MIME-Version: 1.0
In-Reply-To: <20181031181717.GD2180@cisco>
References: <20181029221037.87724-1-dancol@google.com> <20181031155912.45088-1-dancol@google.com>
 <20181031175448.GC2180@cisco> <CAKOZuevrFxWLY1J1DVPNGaEy8UbkD1r_M9T9FQTOtSrqp-G0qw@mail.gmail.com>
 <20181031181717.GD2180@cisco>
From:   Daniel Colascione <dancol@google.com>
Date:   Wed, 31 Oct 2018 19:33:06 +0000
Message-ID: <CAKOZuetdSK8jhD1snja8p4AQTNPrLOugQrVn642RCH8S4QBGFg@mail.gmail.com>
Subject: Re: [PATCH v3] Implement /proc/pid/kill
To:     Tycho Andersen <tycho@tycho.ws>
Cc:     linux-kernel <linux-kernel@vger.kernel.org>,
        Tim Murray <timmurray@google.com>,
        Joel Fernandes <joelaf@google.com>,
        Suren Baghdasaryan <surenb@google.com>,
        Aleksa Sarai <cyphar@cyphar.com>,
        Christian Brauner <christian.brauner@canonical.com>,
        "Eric W. Biederman" <ebiederm@xmission.com>,
        Kees Cook <keescook@chromium.org>,
        Oleg Nesterov <oleg@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 31, 2018 at 6:17 PM, Tycho Andersen <tycho@tycho.ws> wrote:
> On Wed, Oct 31, 2018 at 06:00:49PM +0000, Daniel Colascione wrote:
>> On Wed, Oct 31, 2018 at 5:54 PM, Tycho Andersen <tycho@tycho.ws> 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.