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

[Daniel Colascione <dancol@google.com>]

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.