* Re: [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 21:23 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Aleksa Sarai, Andy Lutomirski, Randy Dunlap,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZueuWzu2GuJ-w3yb01P9uyO1WniKG+i=BUsweVdA-KgEjhw@mail.gmail.com>
On Sun, Nov 18, 2018 at 12:54:10PM -0800, Daniel Colascione wrote:
> On Sun, Nov 18, 2018 at 12:43 PM, Christian Brauner
> <christian@brauner.io> wrote:
> > On Sun, Nov 18, 2018 at 01:28:41PM -0700, Andy Lutomirski wrote:
> >>
> >>
> >> > On Nov 18, 2018, at 12:44 PM, Daniel Colascione <dancol@google.com> wrote:
> >> >
> >>
> >> >
> >> > That is, I'm proposing an API that looks like this:
> >> >
> >> > int process_kill(int procfs_dfd, int signo, const union sigval value)
> >> >
> >> > If, later, process_kill were to *also* accept process-capability FDs,
> >> > nothing would break.
> >>
> >> Except that this makes it ambiguous to the caller as to whether their current creds are considered. So it would need to be a different syscall or at least a flag. Otherwise a lot of those nice theoretical properties go away.
> >
> > I can add a flag argument
> > int process_signal(int procfs_dfd, int signo, siginfo_t *info, int flags)
> > The way I see it process_signal() should be equivalent to kill(pid, signal) for now.
> > That is siginfo_t is cleared and set to:
> >
> > info.si_signo = sig;
> > info.si_errno = 0;
> > info.si_code = SI_USER;
> > info.si_pid = task_tgid_vnr(current);
> > info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
>
> That makes sense. I just don't want to get into a situation where
> callers feel that they *have* to use the PID-based APIs to send a
> signal because process_kill doesn't offer some bit of functionality.
Yeah.
>
> Are you imagining something like requiring info t be NULL unless flags
> contains some "I have a siginfo_t" value?
Well, I was actually thinking about something like:
/**
* sys_process_signal - send a signal to a process trough a process file descriptor
* @fd: the file descriptor of the process
* @sig: signal to be sent
* @info: the signal info
* @flags: future flags to be passed
*/
SYSCALL_DEFINE4(process_signal, int, fd, int, sig, siginfo_t __user *, info,
int, flags)
{
struct pid *pid;
struct fd *f;
kernel_siginfo_t kinfo;
/* Do not allow users to pass garbage. */
if (flags)
return -EINVAL;
int ret = __copy_siginfo_from_user(sig, &kinfo, info);
if (unlikely(ret))
return ret;
/* For now, enforce that caller's creds are used. */
kinfo.si_pid = task_tgid_vnr(current);
kinfo.si_uid = from_kuid_munged(current_user_ns(), current_uid());
if (signal_impersonates_kernel(kinfo))
return -EPERM;
f = fdget(fd);
if (!f.file)
return -EBADF;
pid = f.file->private_data;
if (!pid)
return -EBADF;
return kill_pid_info(sig, kinfo, pid);
}
>
> BTW: passing SI_USER to rt_sigqueueinfo *should* as long as the
> passed-in si_pid and si_uid match what the kernel would set them to in
> the kill(2) case. The whole point of SI_USER is that the recipient
> knows that it can trust the origin information embedded in the
> siginfo_t in the signal handler. If the kernel verifies that a signal
> sender isn't actually lying, why not let people send SI_USER with
> rt_sigqueueinfo?
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 20:54 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Aleksa Sarai, Andy Lutomirski, Randy Dunlap,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <20181118204317.seaztq7fqmysucns@brauner.io>
On Sun, Nov 18, 2018 at 12:43 PM, Christian Brauner
<christian@brauner.io> wrote:
> On Sun, Nov 18, 2018 at 01:28:41PM -0700, Andy Lutomirski wrote:
>>
>>
>> > On Nov 18, 2018, at 12:44 PM, Daniel Colascione <dancol@google.com> wrote:
>> >
>>
>> >
>> > That is, I'm proposing an API that looks like this:
>> >
>> > int process_kill(int procfs_dfd, int signo, const union sigval value)
>> >
>> > If, later, process_kill were to *also* accept process-capability FDs,
>> > nothing would break.
>>
>> Except that this makes it ambiguous to the caller as to whether their current creds are considered. So it would need to be a different syscall or at least a flag. Otherwise a lot of those nice theoretical properties go away.
>
> I can add a flag argument
> int process_signal(int procfs_dfd, int signo, siginfo_t *info, int flags)
> The way I see it process_signal() should be equivalent to kill(pid, signal) for now.
> That is siginfo_t is cleared and set to:
>
> info.si_signo = sig;
> info.si_errno = 0;
> info.si_code = SI_USER;
> info.si_pid = task_tgid_vnr(current);
> info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
That makes sense. I just don't want to get into a situation where
callers feel that they *have* to use the PID-based APIs to send a
signal because process_kill doesn't offer some bit of functionality.
Are you imagining something like requiring info t be NULL unless flags
contains some "I have a siginfo_t" value?
BTW: passing SI_USER to rt_sigqueueinfo *should* as long as the
passed-in si_pid and si_uid match what the kernel would set them to in
the kill(2) case. The whole point of SI_USER is that the recipient
knows that it can trust the origin information embedded in the
siginfo_t in the signal handler. If the kernel verifies that a signal
sender isn't actually lying, why not let people send SI_USER with
rt_sigqueueinfo?
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 20:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Aleksa Sarai, Andy Lutomirski, Randy Dunlap,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <608F2959-800D-46EE-A7CD-8C972ACD2F02@amacapital.net>
On Sun, Nov 18, 2018 at 01:28:41PM -0700, Andy Lutomirski wrote:
>
>
> > On Nov 18, 2018, at 12:44 PM, Daniel Colascione <dancol@google.com> wrote:
> >
>
> >
> > That is, I'm proposing an API that looks like this:
> >
> > int process_kill(int procfs_dfd, int signo, const union sigval value)
> >
> > If, later, process_kill were to *also* accept process-capability FDs,
> > nothing would break.
>
> Except that this makes it ambiguous to the caller as to whether their current creds are considered. So it would need to be a different syscall or at least a flag. Otherwise a lot of those nice theoretical properties go away.
I can add a flag argument
int process_signal(int procfs_dfd, int signo, siginfo_t *info, int flags)
The way I see it process_signal() should be equivalent to kill(pid, signal) for now.
That is siginfo_t is cleared and set to:
info.si_signo = sig;
info.si_errno = 0;
info.si_code = SI_USER;
info.si_pid = task_tgid_vnr(current);
info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
>
> > Yes, that's what I have in mind. A siginfo_t is small enough that we
> > could just store it as a blob allocated off the procfs inode or
> > something like that without bothering with a shmfs file. You'd be able
> > to read(2) the exit status as many times as you wanted.
>
> I think that, if the syscall in question is read(2), then it should work *once* per struct file. Otherwise running cat on the file would behave very oddly.
>
> Read and poll have the same problem as write: we can’t check caps in read or poll either.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 20:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Aleksa Sarai, Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <608F2959-800D-46EE-A7CD-8C972ACD2F02@amacapital.net>
On Sun, Nov 18, 2018 at 12:28 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> That is, I'm proposing an API that looks like this:
>>
>> int process_kill(int procfs_dfd, int signo, const union sigval value)
>>
>> If, later, process_kill were to *also* accept process-capability FDs,
>> nothing would break.
>
> Except that this makes it ambiguous to the caller as to whether their current creds are considered. So it would need to be a different syscall or at least a flag. Otherwise a lot of those nice theoretical properties go away.
Sure. A flag might make for better ergonomics.
>> Yes, that's what I have in mind. A siginfo_t is small enough that we
>> could just store it as a blob allocated off the procfs inode or
>> something like that without bothering with a shmfs file. You'd be able
>> to read(2) the exit status as many times as you wanted.
>
> I think that, if the syscall in question is read(2), then it should work *once* per struct file. Otherwise running cat on the file would behave very oddly.
Why? The file pointer would work normally.
> Read and poll have the same problem as write: we can’t check caps in read or poll either.
Why not? Reading /proc/pid/stat does an access check today and
conditionally replaces the exit status with zero.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 20:28 UTC (permalink / raw)
To: Daniel Colascione
Cc: Aleksa Sarai, Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZuetfqvn1uVqKJ=16iEzG4g449YOjC_tLM60eKBSkv9u+bQ@mail.gmail.com>
> On Nov 18, 2018, at 12:44 PM, Daniel Colascione <dancol@google.com> wrote:
>
>
> That is, I'm proposing an API that looks like this:
>
> int process_kill(int procfs_dfd, int signo, const union sigval value)
>
> If, later, process_kill were to *also* accept process-capability FDs,
> nothing would break.
Except that this makes it ambiguous to the caller as to whether their current creds are considered. So it would need to be a different syscall or at least a flag. Otherwise a lot of those nice theoretical properties go away.
> Yes, that's what I have in mind. A siginfo_t is small enough that we
> could just store it as a blob allocated off the procfs inode or
> something like that without bothering with a shmfs file. You'd be able
> to read(2) the exit status as many times as you wanted.
I think that, if the syscall in question is read(2), then it should work *once* per struct file. Otherwise running cat on the file would behave very oddly.
Read and poll have the same problem as write: we can’t check caps in read or poll either.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 20:21 UTC (permalink / raw)
To: Christian Brauner
Cc: Aleksa Sarai, Andy Lutomirski, Randy Dunlap, Eric W. Biederman,
LKML, Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Al Viro, Linux FS Devel, Linux API, Tim Murray, Kees Cook,
Jan Engelhardt
In-Reply-To: <20181118201514.c5ujdjav6ccodyff@brauner.io>
On Sun, Nov 18, 2018 at 12:15 PM, Christian Brauner
<christian@brauner.io> wrote:
>> That is, I'm proposing an API that looks like this:
>>
>> int process_kill(int procfs_dfd, int signo, const union sigval value)
>
> I've started a second tree with process_signal(int procpid_dfd, int sig)
Thanks.
> instead of an ioctl(). Why do you want sigval too?
API completeness. The sigqueue interface is a superset of kill, and I
don't want process_kill to do less than any PID-based kill. Maybe
taking a siginfo_t, like rt_sigqueueinfo does, would be even better in
that respect, come to think of it.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 20:15 UTC (permalink / raw)
To: Daniel Colascione
Cc: Aleksa Sarai, Andy Lutomirski, Randy Dunlap, Eric W. Biederman,
LKML, Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Al Viro, Linux FS Devel, Linux API, Tim Murray, Kees Cook,
Jan Engelhardt
In-Reply-To: <CAKOZuetfqvn1uVqKJ=16iEzG4g449YOjC_tLM60eKBSkv9u+bQ@mail.gmail.com>
On Sun, Nov 18, 2018 at 11:44:19AM -0800, Daniel Colascione wrote:
> On Sun, Nov 18, 2018 at 11:05 AM, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2018-11-18, Daniel Colascione <dancol@google.com> wrote:
> >> > Here's my point: if we're really going to make a new API to manipulate
> >> > processes by their fd, I think we should have at least a decent idea
> >> > of how that API will get extended in the future. Right now, we have
> >> > an extremely awkward situation where opening an fd in /proc requires
> >> > certain capabilities or uids, and using those fds often also checks
> >> > current's capabilities, and the target process may have changed its
> >> > own security context, including gaining privilege via SUID, SGID, or
> >> > LSM transition rules in the mean time. This has been a huge source of
> >> > security bugs. It would be nice to have a model for future APIs that
> >> > avoids these problems.
> >> >
> >> > And I didn't say in my proposal that a process's identity should
> >> > fundamentally change when it calls execve(). I'm suggesting that
> >> > certain operations that could cause a process to gain privilege or
> >> > otherwise require greater permission to introspect (mainly execve)
> >> > could be handled by invalidating the new process management fds.
> >> > Sure, if init re-execs itself, it's still PID 1, but that doesn't
> >> > necessarily mean that:
> >> >
> >> > fd = process_open_management_fd(1);
> >> > [init reexecs]
> >> > process_do_something(fd);
> >> >
> >> > needs to work.
> >>
> >> PID 1 is a bad example here, because it doesn't get recycled. Other
> >> PIDs do. The snippet you gave *does* need to work, in general, because
> >> if exec invalidates the handle, and you need to reopen by PID to
> >> re-establish your right to do something with the process, that process
> >> may in fact have died between the invalidation and your reopen, and
> >> your reopened FD may refer to some other random process.
> >
> > I imagine the error would be -EPERM rather than -ESRCH in this case,
> > which would be incredibly trivial for userspace to differentiate
> > between.
>
> Why would userspace necessarily see EPERM? The PID might get recycled
> into a different random process that the caller has the ability to
> affect.
>
> > If you wish to re-open the path that is also trivial by
> > re-opening through /proc/self/fd/$fd -- which will re-do any permission
> > checks and will guarantee that you are re-opening the same 'struct file'
> > and thus the same 'struct pid'.
>
> When you reopen via /proc/self/fd, you get a new struct file
> referencing the existing inode, not the same struct file. A new
> reference to the old struct file would just be dup.
>
> Anyway: what other API requires, for correct operation, occasional
> reopening through /proc/self/fd? It's cumbersome, and it doesn't add
> anything. If we invalidate process handles on execve, and processes
> are legally allowed to re-exec themselves for arbitrary reasons at any
> time, that's tantamount to saying that handles might become invalid at
> any time and that all callers must be prepared to go through the
> reopen-and-retry path before any operation.
>
> Why are we making them do that? So that a process can have an open FD
> that represents a process-operation capability? Which capability does
> the open FD represent?
>
> I think when you and Andy must be talking about is an API that looks like this:
>
> int open_process_operation_handle(int procfs_dirfd, int capability_bitmask)
>
> capability_bitmask would have bits like
>
> PROCESS_CAPABILITY_KILL --- send a signal
> PROCESS_CAPABILITY_PTRACE --- attach to a process
> PROCESS_CAPABILITY_READ_EXIT_STATUS --- what it says on the tin
> PROCESS_CAPABILITY_READ_CMDLINE --- etc.
>
> Then you'd have system calls like
>
> int process_kill(int process_capability_fd, int signo, const union sigval data)
> int process_ptrace_attach(int process_capability_fd)
> int process_wait_for_exit(int process_capability_fd, siginfo_t* exit_info)
>
> that worked on these capability bits. If a process execs or does
> something else to change its security capabilities, operations on
> these capability FDs would fail with ESTALE or something and callers
> would have to re-acquire their capabilities.
>
> This approach works fine. It has some nice theoretical properties, and
> could allow for things like nicer privilege separation for debuggers.
> I wouldn't mind something like this getting into the kernel.
>
> I just don't think this model is necessary right now. I want a small
> change from what we have today, one likely to actually make it into
> the tree. And bypassing the capability FDs and just allowing callers
> to operate directly on process *identity* FDs, using privileges in
> effect at the time of all, is at least no worse than what we have now.
>
> That is, I'm proposing an API that looks like this:
>
> int process_kill(int procfs_dfd, int signo, const union sigval value)
I've started a second tree with process_signal(int procpid_dfd, int sig)
instead of an ioctl(). Why do you want sigval too?
>
> If, later, process_kill were to *also* accept process-capability FDs,
> nothing would break.
>
> >> The only way around this problem is to have two separate FDs --- one
> >> to represent process identity, which *must* be continuous across
> >> execve, and the other to represent some specific capability, some
> >> ability to do something to that process. It's reasonable to invalidate
> >> capability after execve, but it's not reasonable to invalidate
> >> identity. In concrete terms, I don't see a big advantage to this
> >> separation, and I think a single identity FD combined with
> >> per-operation capability checks is sufficient. And much simpler.
> >
> > I think that the error separation above would trivially allow user-space
> > to know whether the identity or capability of a process being monitored
> > has changed.
> >
> > Currently, all operations on a '/proc/$pid' which you've previously
> > opened and has died will give you -ESRCH.
>
> Not the case. Zombies have died, but profs operations work fine on zombies.
>
> >> > Similarly, it seems like
> >> > it's probably safe to be able to open an fd that lets you watch the
> >> > exit status of a process, have the process call setresuid(), and still
> >> > see the exit status.
> >>
> >> Is it? That's an open question.
> >
> > Well, if we consider wait4(2) it seems that this is already the case.
> > If you fork+exec a setuid binary you can definitely see its exit code.
>
> Only if you're the parent. Otherwise, you can't see the process exit
> status unless you pass a ptrace access check and consult
> /proc/pid/stat after the process dies, but before the zombie
> disappears. Random unrelated and unprivileged processes can't see exit
> statuses from distant parts of the system.
>
> >> > My POLLERR hack, aside from being ugly,
> >> > avoids this particular issue because it merely lets you wait for
> >> > something you already could have observed using readdir().
> >>
> >> Yes. I mentioned this same issue-punting as the motivation behind
> >> exithand, initially, just reading EOF on exit.
> >
> > One question I have about EOF-on-exit is that if we wish to extend it to
> > allow providing the exit status (which is something we discussed in the
> > original thread), how will multiple-readers be handled in such a
> > scenario?
> > Would we be storing the exit status or siginfo in the
> > equivalent of a locked memfd?
>
> Yes, that's what I have in mind. A siginfo_t is small enough that we
> could just store it as a blob allocated off the procfs inode or
> something like that without bothering with a shmfs file. You'd be able
> to read(2) the exit status as many times as you wanted.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 19:44 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <20181118190504.ixglsqbn6mxkcdzu@yavin>
On Sun, Nov 18, 2018 at 11:05 AM, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-11-18, Daniel Colascione <dancol@google.com> wrote:
>> > Here's my point: if we're really going to make a new API to manipulate
>> > processes by their fd, I think we should have at least a decent idea
>> > of how that API will get extended in the future. Right now, we have
>> > an extremely awkward situation where opening an fd in /proc requires
>> > certain capabilities or uids, and using those fds often also checks
>> > current's capabilities, and the target process may have changed its
>> > own security context, including gaining privilege via SUID, SGID, or
>> > LSM transition rules in the mean time. This has been a huge source of
>> > security bugs. It would be nice to have a model for future APIs that
>> > avoids these problems.
>> >
>> > And I didn't say in my proposal that a process's identity should
>> > fundamentally change when it calls execve(). I'm suggesting that
>> > certain operations that could cause a process to gain privilege or
>> > otherwise require greater permission to introspect (mainly execve)
>> > could be handled by invalidating the new process management fds.
>> > Sure, if init re-execs itself, it's still PID 1, but that doesn't
>> > necessarily mean that:
>> >
>> > fd = process_open_management_fd(1);
>> > [init reexecs]
>> > process_do_something(fd);
>> >
>> > needs to work.
>>
>> PID 1 is a bad example here, because it doesn't get recycled. Other
>> PIDs do. The snippet you gave *does* need to work, in general, because
>> if exec invalidates the handle, and you need to reopen by PID to
>> re-establish your right to do something with the process, that process
>> may in fact have died between the invalidation and your reopen, and
>> your reopened FD may refer to some other random process.
>
> I imagine the error would be -EPERM rather than -ESRCH in this case,
> which would be incredibly trivial for userspace to differentiate
> between.
Why would userspace necessarily see EPERM? The PID might get recycled
into a different random process that the caller has the ability to
affect.
> If you wish to re-open the path that is also trivial by
> re-opening through /proc/self/fd/$fd -- which will re-do any permission
> checks and will guarantee that you are re-opening the same 'struct file'
> and thus the same 'struct pid'.
When you reopen via /proc/self/fd, you get a new struct file
referencing the existing inode, not the same struct file. A new
reference to the old struct file would just be dup.
Anyway: what other API requires, for correct operation, occasional
reopening through /proc/self/fd? It's cumbersome, and it doesn't add
anything. If we invalidate process handles on execve, and processes
are legally allowed to re-exec themselves for arbitrary reasons at any
time, that's tantamount to saying that handles might become invalid at
any time and that all callers must be prepared to go through the
reopen-and-retry path before any operation.
Why are we making them do that? So that a process can have an open FD
that represents a process-operation capability? Which capability does
the open FD represent?
I think when you and Andy must be talking about is an API that looks like this:
int open_process_operation_handle(int procfs_dirfd, int capability_bitmask)
capability_bitmask would have bits like
PROCESS_CAPABILITY_KILL --- send a signal
PROCESS_CAPABILITY_PTRACE --- attach to a process
PROCESS_CAPABILITY_READ_EXIT_STATUS --- what it says on the tin
PROCESS_CAPABILITY_READ_CMDLINE --- etc.
Then you'd have system calls like
int process_kill(int process_capability_fd, int signo, const union sigval data)
int process_ptrace_attach(int process_capability_fd)
int process_wait_for_exit(int process_capability_fd, siginfo_t* exit_info)
that worked on these capability bits. If a process execs or does
something else to change its security capabilities, operations on
these capability FDs would fail with ESTALE or something and callers
would have to re-acquire their capabilities.
This approach works fine. It has some nice theoretical properties, and
could allow for things like nicer privilege separation for debuggers.
I wouldn't mind something like this getting into the kernel.
I just don't think this model is necessary right now. I want a small
change from what we have today, one likely to actually make it into
the tree. And bypassing the capability FDs and just allowing callers
to operate directly on process *identity* FDs, using privileges in
effect at the time of all, is at least no worse than what we have now.
That is, I'm proposing an API that looks like this:
int process_kill(int procfs_dfd, int signo, const union sigval value)
If, later, process_kill were to *also* accept process-capability FDs,
nothing would break.
>> The only way around this problem is to have two separate FDs --- one
>> to represent process identity, which *must* be continuous across
>> execve, and the other to represent some specific capability, some
>> ability to do something to that process. It's reasonable to invalidate
>> capability after execve, but it's not reasonable to invalidate
>> identity. In concrete terms, I don't see a big advantage to this
>> separation, and I think a single identity FD combined with
>> per-operation capability checks is sufficient. And much simpler.
>
> I think that the error separation above would trivially allow user-space
> to know whether the identity or capability of a process being monitored
> has changed.
>
> Currently, all operations on a '/proc/$pid' which you've previously
> opened and has died will give you -ESRCH.
Not the case. Zombies have died, but profs operations work fine on zombies.
>> > Similarly, it seems like
>> > it's probably safe to be able to open an fd that lets you watch the
>> > exit status of a process, have the process call setresuid(), and still
>> > see the exit status.
>>
>> Is it? That's an open question.
>
> Well, if we consider wait4(2) it seems that this is already the case.
> If you fork+exec a setuid binary you can definitely see its exit code.
Only if you're the parent. Otherwise, you can't see the process exit
status unless you pass a ptrace access check and consult
/proc/pid/stat after the process dies, but before the zombie
disappears. Random unrelated and unprivileged processes can't see exit
statuses from distant parts of the system.
>> > My POLLERR hack, aside from being ugly,
>> > avoids this particular issue because it merely lets you wait for
>> > something you already could have observed using readdir().
>>
>> Yes. I mentioned this same issue-punting as the motivation behind
>> exithand, initially, just reading EOF on exit.
>
> One question I have about EOF-on-exit is that if we wish to extend it to
> allow providing the exit status (which is something we discussed in the
> original thread), how will multiple-readers be handled in such a
> scenario?
> Would we be storing the exit status or siginfo in the
> equivalent of a locked memfd?
Yes, that's what I have in mind. A siginfo_t is small enough that we
could just store it as a blob allocated off the procfs inode or
something like that without bothering with a shmfs file. You'd be able
to read(2) the exit status as many times as you wanted.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 19:24 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, David Howells
In-Reply-To: <CAKOZueto1CmUSWFKjOkazcDcZH6CiXXCr4U=ghewLh8GjE2-=A@mail.gmail.com>
On Sun, Nov 18, 2018 at 10:07:31AM -0800, Daniel Colascione wrote:
> On Sun, Nov 18, 2018 at 9:41 AM, Christian Brauner <christian@brauner.io> wrote:
> > On Sun, Nov 18, 2018 at 07:38:09AM -0800, Andy Lutomirski wrote:
> >> On Sun, Nov 18, 2018 at 5:59 AM Daniel Colascione <dancol@google.com> wrote:
> >> >
> >> > I had been led to believe that the proposal would be a comprehensive
> >> > process API, not an ioctl basically equivalent to my previous patch.
> >> > If you had a more comprehensive proposal, please just share it on LKML
> >> > instead of limiting the discussion to those able to attend these
> >> > various conferences. If there's some determined opposition to a
> >> > general new process API, this opposition needs a fair and full airing,
> >> > as not everyone can attend these conferences.
> >> >
> >> > On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> >> > > With this patch an open() call on /proc/<pid> will give userspace a handle
> >> > > to struct pid of the process associated with /proc/<pid>. This allows to
> >> > > maintain a stable handle on a process.
> >> > > I have been discussing various approaches extensively during technical
> >> > > conferences this year culminating in a long argument with Eric at Linux
> >> > > Plumbers. The general consensus was that having a handle on a process
> >> > > will be something that is very simple and easy to maintain
> >> >
> >> > ioctls are the opposite of "easy to maintain". Their
> >> > file-descriptor-specific behavior makes it difficult to use the things
> >> > safely. If you want to take this approach, please make a new system
> >> > call. An ioctl is just a system call with a very strange spelling and
> >> > unfortunate collision semantics.
> >> >
> >> > > with the
> >> > > option of being extensible via a more advanced api if the need arises.
> >> >
> >> > The need *has* arisen; see my exithand patch.
> >> >
> >> > > I
> >> > > believe that this patch is the most simple, dumb, and therefore
> >> > > maintainable solution.
> >> > >
> >> > > The need for this has arisen in order to reliably kill a process without
> >> > > running into issues of the pid being recycled as has been described in the
> >> > > rejected patch [1].
> >> >
> >> > That patch was not "rejected". It was tabled pending the more
> >> > comprehensive process API proposal that was supposed to have emerged.
> >> > This patch is just another variant of the sort of approach we
> >> > discussed on that patch's thread here. As I mentioned on that thread,
> >> > the right approach option is a new system call, not an ioctl.
> >> >
> >> > To fulfill the need described in that patchset a new
> >> > > ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> >> > > process via a file descriptor:
> >> > >
> >> > > int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> >> > > ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> >> > > close(fd);
> >> > >
> >> > > Note, the stable handle will allow us to carefully extend this feature in
> >> > > the future.
> >> >
> >> > We still need the ability to synchronously wait on a process's death,
> >> > as in my patch set. I will be refreshing that patch set.
> >>
> >> I fully agree that a more comprehensive, less expensive API for
> >> managing processes would be nice. But I also think that this patch
> >> (using the directory fd and ioctl) is better from a security
> >> perspective than using a new file in /proc.
> >>
> >> I have an old patch to make proc directory fds pollable:
> >>
> >> https://lore.kernel.org/patchwork/patch/345098/
> >>
> >> That patch plus the one in this thread might make a nice addition to
> >> the kernel even if we expect something much better to come along
> >> later.
> >
> > I agree. Eric's point was to make the first implementation of this as
> > simple as possible that's why this patch is intentionally almost
> > trivial. And I like it for its simplicity.
> >
> > I had a more comprehensive API proposal of which open(/proc/<pid>) was a
> > part. I didn't send out alongside this patch as Eric clearly prefered to
> > only have the /proc/<pid> part. Here is the full proposal as I intended
> > to originally send it out:
>
> Thanks.
>
> > The gist is to have file descriptors for processes which is obviously not a new
> > idea. This has been done before in other OSes and it has been tried before in
> > Linux [2], [3] (Thanks to Kees for pointing out these patches.). So I want to
> > make it very clear that I'm not laying claim to this being my or even a novel
> > idea in any way. However, I want to diverge from previous approaches with my
> > suggestion. (Though I can't be sure that there's not something similar in other
> > OSes already.)
>
> Windows works basically as you describe. You can create a process is
> suspended state, configure it however you want, then let it run.
> CreateProcess (and even moreso, NtCreateProcess) also provide a rich
> (and *extensible*) interface for pre-creation process configuration.
>
> >> One of the main motivations for having procfds is to have a race-free way of
> > configuring, starting, polling, and killing a process. Basically, a process
> > lifecycle api if you want to think about it that way. The api should also be
> > easily extendable in the future to avoid running into the limitations we
> > currently see with the clone*() syscall(s) again.
> >
> > One of the crucial points of the api is to *separate the configuration
> > of a process through a procfd from actually creating the process*.
> > This is a crucial property expressed in the open*() system calls. First, get a
> > stable handle on an object then allow for ways to configure it. As such the
> > procfd api shares the same insight with Al's and David's new mount api.
> > (Fwiw, Andy also pointed out similarities with posix_spawn().)
> > What I envisioned was to have the following syscalls (multiple name suggestions):
> >
> > 1. int process_open / proc_open / procopen
> > 2. int process_config / proc_config / procconfig or ioctl()-based
> > 3. int process_info / proc_info / procinfo or ioctl()-based
> > 4. int process_manage / proc_manage / procmanage or ioctl()-based
>
> The API you've proposed seems fine to me, although I'd either 1)
> consolidate operations further into one system call, or 2) separate
> the different management operations into more and different system
> calls that can be audited independently. The grouping you've proposed
> seems to have the worst aspects of API splitting and API multiplexing.
> But I have no objection to it in spirit.
>
> That said, while I do want to fix process configuration and startup
> generally, I want to fix specific holes in the existing API surface
> first. The two patches I've already sent do that, and this work
> shouldn't wait on an ideal larger process-API overhaul that may or may
> not arrive. Based on previous history, I suspect that an API of the
> scope you're proposing would take years to overcome all LKML
> objections and land. I don't want to wait that long when we can make
> smaller fixes that would not conflict with the general architecture.
>
> The original patch on this thread is half of the right fix. While I
> think we should use a system call instead of an ioctl, and while I
> have some specific implementation critiques (which I described in a
> different message), it's the right general sort of thing. We should
> merge it.
Thanks. I agree. Note, I don't care if it's an ioctl() or not. I'm happy
to instead add a syscall process_signal() alongside this patchset. What
do people prefer?
>
> Next, I want to merge my exithand proposal, or something like it. It's
> likewise a simple change that, in a minimal way, addresses a
> longstanding API deficiency. I'm very strongly against the
> POLLERR-on-directory variant of the idea.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Aleksa Sarai @ 2018-11-18 19:05 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Al Viro, Linux FS Devel, Linux API,
Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZuevRq-igh06zS_nsGG400zXrKFCtajpEG9-xgU2+Rtb2Pw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4901 bytes --]
On 2018-11-18, Daniel Colascione <dancol@google.com> wrote:
> > Here's my point: if we're really going to make a new API to manipulate
> > processes by their fd, I think we should have at least a decent idea
> > of how that API will get extended in the future. Right now, we have
> > an extremely awkward situation where opening an fd in /proc requires
> > certain capabilities or uids, and using those fds often also checks
> > current's capabilities, and the target process may have changed its
> > own security context, including gaining privilege via SUID, SGID, or
> > LSM transition rules in the mean time. This has been a huge source of
> > security bugs. It would be nice to have a model for future APIs that
> > avoids these problems.
> >
> > And I didn't say in my proposal that a process's identity should
> > fundamentally change when it calls execve(). I'm suggesting that
> > certain operations that could cause a process to gain privilege or
> > otherwise require greater permission to introspect (mainly execve)
> > could be handled by invalidating the new process management fds.
> > Sure, if init re-execs itself, it's still PID 1, but that doesn't
> > necessarily mean that:
> >
> > fd = process_open_management_fd(1);
> > [init reexecs]
> > process_do_something(fd);
> >
> > needs to work.
>
> PID 1 is a bad example here, because it doesn't get recycled. Other
> PIDs do. The snippet you gave *does* need to work, in general, because
> if exec invalidates the handle, and you need to reopen by PID to
> re-establish your right to do something with the process, that process
> may in fact have died between the invalidation and your reopen, and
> your reopened FD may refer to some other random process.
I imagine the error would be -EPERM rather than -ESRCH in this case,
which would be incredibly trivial for userspace to differentiate
between. If you wish to re-open the path that is also trivial by
re-opening through /proc/self/fd/$fd -- which will re-do any permission
checks and will guarantee that you are re-opening the same 'struct file'
and thus the same 'struct pid'.
> The only way around this problem is to have two separate FDs --- one
> to represent process identity, which *must* be continuous across
> execve, and the other to represent some specific capability, some
> ability to do something to that process. It's reasonable to invalidate
> capability after execve, but it's not reasonable to invalidate
> identity. In concrete terms, I don't see a big advantage to this
> separation, and I think a single identity FD combined with
> per-operation capability checks is sufficient. And much simpler.
I think that the error separation above would trivially allow user-space
to know whether the identity or capability of a process being monitored
has changed.
Currently, all operations on a '/proc/$pid' which you've previously
opened and has died will give you -ESRCH. So the above separation I
mentioned is entirely consistent with how users are using '/proc/$pid'
to check for PID death today.
> > I think you're overstating your case. To a pretty good approximation,
> > setresuid() allows the caller to remove elements from the set {ruid,
> > suid, euid}, unless the caller has CAP_SETUID. If you could ptrace a
> > process before it calls setresuid(), you might as well be able to
> > ptrace() it after, since you could have just ptraced it and made it
> > call setresuid() while still ptracing it.
>
> What about a child that execs a setuid binary?
Yeah, for this reason I think that using -EPERM on operations that we
think are not reasonable to allow possibly-less-privileged processes to
do -- probably the most reasonable choice would be ptrace_may_access().
> > Similarly, it seems like
> > it's probably safe to be able to open an fd that lets you watch the
> > exit status of a process, have the process call setresuid(), and still
> > see the exit status.
>
> Is it? That's an open question.
Well, if we consider wait4(2) it seems that this is already the case.
If you fork+exec a setuid binary you can definitely see its exit code.
> > My POLLERR hack, aside from being ugly,
> > avoids this particular issue because it merely lets you wait for
> > something you already could have observed using readdir().
>
> Yes. I mentioned this same issue-punting as the motivation behind
> exithand, initially, just reading EOF on exit.
One question I have about EOF-on-exit is that if we wish to extend it to
allow providing the exit status (which is something we discussed in the
original thread), how will multiple-readers be handled in such a
scenario? Would we be storing the exit status or siginfo in the
equivalent of a locked memfd?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 18:43 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Randy Dunlap, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, Jan Engelhardt
In-Reply-To: <CALCETrVscRwQG55-j1SKc2TmSb1-=5861804ojUuviNzdyDOrA@mail.gmail.com>
On Sun, Nov 18, 2018 at 10:28 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 9:51 AM Daniel Colascione <dancol@google.com> wrote:
>>
>> > I'm not entirely sure that ship has sailed. In the kernel, we already
>> > have a bit of a distinction between a pid (and tid, etc -- I'm
>> > referring to struct pid) and a task. If we make a new
>> > process-management API, we could put a distinction like this into the
>> > API.
>>
>> It would be a disaster to have different APIs give callers a different
>> idea of process identity over its lifetime. The precedent is
>> well-established that execve and setreuid do not change a process's
>> identity. Invaliding some identifiers but not others in response to
>> supposedly-internal things a process might do under rare circumstances
>> is creating a bug machine..
>
> Here's my point: if we're really going to make a new API to manipulate
> processes by their fd, I think we should have at least a decent idea
> of how that API will get extended in the future. Right now, we have
> an extremely awkward situation where opening an fd in /proc requires
> certain capabilities or uids, and using those fds often also checks
> current's capabilities, and the target process may have changed its
> own security context, including gaining privilege via SUID, SGID, or
> LSM transition rules in the mean time. This has been a huge source of
> security bugs. It would be nice to have a model for future APIs that
> avoids these problems.
>
> And I didn't say in my proposal that a process's identity should
> fundamentally change when it calls execve(). I'm suggesting that
> certain operations that could cause a process to gain privilege or
> otherwise require greater permission to introspect (mainly execve)
> could be handled by invalidating the new process management fds.
> Sure, if init re-execs itself, it's still PID 1, but that doesn't
> necessarily mean that:
>
> fd = process_open_management_fd(1);
> [init reexecs]
> process_do_something(fd);
>
> needs to work.
PID 1 is a bad example here, because it doesn't get recycled. Other
PIDs do. The snippet you gave *does* need to work, in general, because
if exec invalidates the handle, and you need to reopen by PID to
re-establish your right to do something with the process, that process
may in fact have died between the invalidation and your reopen, and
your reopened FD may refer to some other random process.
The only way around this problem is to have two separate FDs --- one
to represent process identity, which *must* be continuous across
execve, and the other to represent some specific capability, some
ability to do something to that process. It's reasonable to invalidate
capability after execve, but it's not reasonable to invalidate
identity. In concrete terms, I don't see a big advantage to this
separation, and I think a single identity FD combined with
per-operation capability checks is sufficient. And much simpler.
>> > setresuid() has no effect
>> > here -- if you have W access to the process and the process calls
>> > setresuid(), you still have W access.
>>
>> Now you've created a situation in which an operation that security
>> policy previously blocked now becomes possible, invaliding previous
>> designs based on the old security invariant. That's the definition of
>> introducing a security hole.
>
> I think you're overstating your case. To a pretty good approximation,
> setresuid() allows the caller to remove elements from the set {ruid,
> suid, euid}, unless the caller has CAP_SETUID. If you could ptrace a
> process before it calls setresuid(), you might as well be able to
> ptrace() it after, since you could have just ptraced it and made it
> call setresuid() while still ptracing it.
What about a child that execs a setuid binary?
> Similarly, it seems like
> it's probably safe to be able to open an fd that lets you watch the
> exit status of a process, have the process call setresuid(), and still
> see the exit status.
Is it? That's an open question.
>
> Regardless of how you feel about these issues, if you're going to add
> an API by which you open an fd, wait for a process to exit, and read
> the exit status, you need to define the conditions under which you may
> open the fd and under which you may read the exit status once you have
> the fd. There are probably multiple valid answers, but the question
> still needs to be answered.
Yes. That's the point I made in that previous message of mine that I referenced.
> My POLLERR hack, aside from being ugly,
> avoids this particular issue because it merely lets you wait for
> something you already could have observed using readdir().
Yes. I mentioned this same issue-punting as the motivation behind
exithand, initially, just reading EOF on exit.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 18:31 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, David Howells
In-Reply-To: <CALCETrUb8MSPPBLLz5Md9Fzaq3Kd9HtG8R5JFwWuy21jOuzbUA@mail.gmail.com>
On Sun, Nov 18, 2018 at 10:15 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 10:07 AM Daniel Colascione <dancol@google.com> wrote:
>> Next, I want to merge my exithand proposal, or something like it. It's
>> likewise a simple change that, in a minimal way, addresses a
>> longstanding API deficiency. I'm very strongly against the
>> POLLERR-on-directory variant of the idea.
>
> Can you explain why you don't like POLLERR-on-a-directory? I'm not
> saying that POLLERR-on-a-directory is fantastic, but I'd like to
> understand what your objection is.
I've written my objections in more detail at [1]. Basically,
1) Nothing else today works by polling on directory file descriptors.
2) POLLERR is wrong because POLLERR indicates, well, an error, but
since we want notifications upon either a transition to a zombie *or*
actual death, and /proc/pid operations work perfectly well on zombie
processes, there's no error to report, and reporting POLLERR would be
a weird kind of lie. POLLHUP might be less awkward here.
3) POLLERR is a single bit of information. I want exit status as well,
or at least a logical path from whatever we add to some kind of exit
status reporting. You can get exit status from a zombie with openat on
/proc/pid/stat, but what if the process fully dies by the time you get
around to reading its exit status? Should we synthesize a
/proc/pid/stat? It seems simpler to be able to just read(2) the exit
information from some FD.
4) Event monitoring frameworks generally treat POLLERR as some kind of
black sheep. Most people think in terms of bits indicating reading and
writing. I want something that can cleanly integrate into existing
wait mechanisms.
5) Poll events are *hints* that some other operation probably won't
block if attempted. Using poll as the primary way of communicating a
bit of information instead of an attempt-IO-now hint feels awkward.
6) A POLLERR interface can't be used by the shell without some kind of helper.
What *advantage* does a POLLERR interface have? That you don't have to
openat a separate file? That's a trivial operation in profs,
especially compared to the machinery of process shutdown.
I'm not particularly tied to a proc file; if we're adding a system
call interface for killing a process by its procfs dfd, we can add one
for returning an eventfd-like FD representing that process's status.
It's unfortunate that the process handle FD also happens to be a
directory FD; if it were a standalone object type, we could just use
POLLIN more naturally.
[1] https://lore.kernel.org/lkml/CAKOZueszfoSM0pxhmuFLOuPmJqSfYXxgutstyCgqxAyoUi4h3w@mail.gmail.com/
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 18:28 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZuevVk_aH_2TuiNcmxgMa+gHXMBXz6Uu5a6TDjoxjhaE36g@mail.gmail.com>
On Sun, Nov 18, 2018 at 9:51 AM Daniel Colascione <dancol@google.com> wrote:
>
> > I'm not entirely sure that ship has sailed. In the kernel, we already
> > have a bit of a distinction between a pid (and tid, etc -- I'm
> > referring to struct pid) and a task. If we make a new
> > process-management API, we could put a distinction like this into the
> > API.
>
> It would be a disaster to have different APIs give callers a different
> idea of process identity over its lifetime. The precedent is
> well-established that execve and setreuid do not change a process's
> identity. Invaliding some identifiers but not others in response to
> supposedly-internal things a process might do under rare circumstances
> is creating a bug machine..
Here's my point: if we're really going to make a new API to manipulate
processes by their fd, I think we should have at least a decent idea
of how that API will get extended in the future. Right now, we have
an extremely awkward situation where opening an fd in /proc requires
certain capabilities or uids, and using those fds often also checks
current's capabilities, and the target process may have changed its
own security context, including gaining privilege via SUID, SGID, or
LSM transition rules in the mean time. This has been a huge source of
security bugs. It would be nice to have a model for future APIs that
avoids these problems.
And I didn't say in my proposal that a process's identity should
fundamentally change when it calls execve(). I'm suggesting that
certain operations that could cause a process to gain privilege or
otherwise require greater permission to introspect (mainly execve)
could be handled by invalidating the new process management fds.
Sure, if init re-execs itself, it's still PID 1, but that doesn't
necessarily mean that:
fd = process_open_management_fd(1);
[init reexecs]
process_do_something(fd);
needs to work.
>
> > setresuid() has no effect
> > here -- if you have W access to the process and the process calls
> > setresuid(), you still have W access.
>
> Now you've created a situation in which an operation that security
> policy previously blocked now becomes possible, invaliding previous
> designs based on the old security invariant. That's the definition of
> introducing a security hole.
I think you're overstating your case. To a pretty good approximation,
setresuid() allows the caller to remove elements from the set {ruid,
suid, euid}, unless the caller has CAP_SETUID. If you could ptrace a
process before it calls setresuid(), you might as well be able to
ptrace() it after, since you could have just ptraced it and made it
call setresuid() while still ptracing it. Similarly, it seems like
it's probably safe to be able to open an fd that lets you watch the
exit status of a process, have the process call setresuid(), and still
see the exit status.
Regardless of how you feel about these issues, if you're going to add
an API by which you open an fd, wait for a process to exit, and read
the exit status, you need to define the conditions under which you may
open the fd and under which you may read the exit status once you have
the fd. There are probably multiple valid answers, but the question
still needs to be answered. My POLLERR hack, aside from being ugly,
avoids this particular issue because it merely lets you wait for
something you already could have observed using readdir().
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 18:15 UTC (permalink / raw)
To: Daniel Colascione
Cc: Christian Brauner, Andrew Lutomirski, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, David Howells
In-Reply-To: <CAKOZueto1CmUSWFKjOkazcDcZH6CiXXCr4U=ghewLh8GjE2-=A@mail.gmail.com>
On Sun, Nov 18, 2018 at 10:07 AM Daniel Colascione <dancol@google.com> wrote:
> Next, I want to merge my exithand proposal, or something like it. It's
> likewise a simple change that, in a minimal way, addresses a
> longstanding API deficiency. I'm very strongly against the
> POLLERR-on-directory variant of the idea.
Can you explain why you don't like POLLERR-on-a-directory? I'm not
saying that POLLERR-on-a-directory is fantastic, but I'd like to
understand what your objection is.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 18:07 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, David Howells
In-Reply-To: <20181118174148.nvkc4ox2uorfatbm@brauner.io>
On Sun, Nov 18, 2018 at 9:41 AM, Christian Brauner <christian@brauner.io> wrote:
> On Sun, Nov 18, 2018 at 07:38:09AM -0800, Andy Lutomirski wrote:
>> On Sun, Nov 18, 2018 at 5:59 AM Daniel Colascione <dancol@google.com> wrote:
>> >
>> > I had been led to believe that the proposal would be a comprehensive
>> > process API, not an ioctl basically equivalent to my previous patch.
>> > If you had a more comprehensive proposal, please just share it on LKML
>> > instead of limiting the discussion to those able to attend these
>> > various conferences. If there's some determined opposition to a
>> > general new process API, this opposition needs a fair and full airing,
>> > as not everyone can attend these conferences.
>> >
>> > On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
>> > > With this patch an open() call on /proc/<pid> will give userspace a handle
>> > > to struct pid of the process associated with /proc/<pid>. This allows to
>> > > maintain a stable handle on a process.
>> > > I have been discussing various approaches extensively during technical
>> > > conferences this year culminating in a long argument with Eric at Linux
>> > > Plumbers. The general consensus was that having a handle on a process
>> > > will be something that is very simple and easy to maintain
>> >
>> > ioctls are the opposite of "easy to maintain". Their
>> > file-descriptor-specific behavior makes it difficult to use the things
>> > safely. If you want to take this approach, please make a new system
>> > call. An ioctl is just a system call with a very strange spelling and
>> > unfortunate collision semantics.
>> >
>> > > with the
>> > > option of being extensible via a more advanced api if the need arises.
>> >
>> > The need *has* arisen; see my exithand patch.
>> >
>> > > I
>> > > believe that this patch is the most simple, dumb, and therefore
>> > > maintainable solution.
>> > >
>> > > The need for this has arisen in order to reliably kill a process without
>> > > running into issues of the pid being recycled as has been described in the
>> > > rejected patch [1].
>> >
>> > That patch was not "rejected". It was tabled pending the more
>> > comprehensive process API proposal that was supposed to have emerged.
>> > This patch is just another variant of the sort of approach we
>> > discussed on that patch's thread here. As I mentioned on that thread,
>> > the right approach option is a new system call, not an ioctl.
>> >
>> > To fulfill the need described in that patchset a new
>> > > ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
>> > > process via a file descriptor:
>> > >
>> > > int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
>> > > ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
>> > > close(fd);
>> > >
>> > > Note, the stable handle will allow us to carefully extend this feature in
>> > > the future.
>> >
>> > We still need the ability to synchronously wait on a process's death,
>> > as in my patch set. I will be refreshing that patch set.
>>
>> I fully agree that a more comprehensive, less expensive API for
>> managing processes would be nice. But I also think that this patch
>> (using the directory fd and ioctl) is better from a security
>> perspective than using a new file in /proc.
>>
>> I have an old patch to make proc directory fds pollable:
>>
>> https://lore.kernel.org/patchwork/patch/345098/
>>
>> That patch plus the one in this thread might make a nice addition to
>> the kernel even if we expect something much better to come along
>> later.
>
> I agree. Eric's point was to make the first implementation of this as
> simple as possible that's why this patch is intentionally almost
> trivial. And I like it for its simplicity.
>
> I had a more comprehensive API proposal of which open(/proc/<pid>) was a
> part. I didn't send out alongside this patch as Eric clearly prefered to
> only have the /proc/<pid> part. Here is the full proposal as I intended
> to originally send it out:
Thanks.
> The gist is to have file descriptors for processes which is obviously not a new
> idea. This has been done before in other OSes and it has been tried before in
> Linux [2], [3] (Thanks to Kees for pointing out these patches.). So I want to
> make it very clear that I'm not laying claim to this being my or even a novel
> idea in any way. However, I want to diverge from previous approaches with my
> suggestion. (Though I can't be sure that there's not something similar in other
> OSes already.)
Windows works basically as you describe. You can create a process is
suspended state, configure it however you want, then let it run.
CreateProcess (and even moreso, NtCreateProcess) also provide a rich
(and *extensible*) interface for pre-creation process configuration.
>> One of the main motivations for having procfds is to have a race-free way of
> configuring, starting, polling, and killing a process. Basically, a process
> lifecycle api if you want to think about it that way. The api should also be
> easily extendable in the future to avoid running into the limitations we
> currently see with the clone*() syscall(s) again.
>
> One of the crucial points of the api is to *separate the configuration
> of a process through a procfd from actually creating the process*.
> This is a crucial property expressed in the open*() system calls. First, get a
> stable handle on an object then allow for ways to configure it. As such the
> procfd api shares the same insight with Al's and David's new mount api.
> (Fwiw, Andy also pointed out similarities with posix_spawn().)
> What I envisioned was to have the following syscalls (multiple name suggestions):
>
> 1. int process_open / proc_open / procopen
> 2. int process_config / proc_config / procconfig or ioctl()-based
> 3. int process_info / proc_info / procinfo or ioctl()-based
> 4. int process_manage / proc_manage / procmanage or ioctl()-based
The API you've proposed seems fine to me, although I'd either 1)
consolidate operations further into one system call, or 2) separate
the different management operations into more and different system
calls that can be audited independently. The grouping you've proposed
seems to have the worst aspects of API splitting and API multiplexing.
But I have no objection to it in spirit.
That said, while I do want to fix process configuration and startup
generally, I want to fix specific holes in the existing API surface
first. The two patches I've already sent do that, and this work
shouldn't wait on an ideal larger process-API overhaul that may or may
not arrive. Based on previous history, I suspect that an API of the
scope you're proposing would take years to overcome all LKML
objections and land. I don't want to wait that long when we can make
smaller fixes that would not conflict with the general architecture.
The original patch on this thread is half of the right fix. While I
think we should use a system call instead of an ioctl, and while I
have some specific implementation critiques (which I described in a
different message), it's the right general sort of thing. We should
merge it.
Next, I want to merge my exithand proposal, or something like it. It's
likewise a simple change that, in a minimal way, addresses a
longstanding API deficiency. I'm very strongly against the
POLLERR-on-directory variant of the idea.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 17:56 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Andy Lutomirski, Christian Brauner, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <875zwu46z1.fsf@xmission.com>
On Sun, Nov 18, 2018 at 9:43 AM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Daniel Colascione <dancol@google.com> writes:
>
>> On Sun, Nov 18, 2018 at 9:13 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>>>>
>>>> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>>> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>>> >>
>>>> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>>> >> > I fully agree that a more comprehensive, less expensive API for
>>>> >> > managing processes would be nice. But I also think that this patch
>>>> >> > (using the directory fd and ioctl) is better from a security
>>>> >> > perspective than using a new file in /proc.
>>>> >>
>>>> >> That's an assertion, not an argument. And I'm not opposed to an
>>>> >> operation on the directory FD, now that it's clear Linus has banned
>>>> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
>>>> >> with a system call instead of a less-reliable ioctl due to the
>>>> >> inherent namespace collision issues in ioctl command names.
>>>> >
>>>> > Linus banned it because of bugs iike the ones in the patch.
>>>>
>>>> Maybe: he didn't provide a reason. What's your point?
>>>
>>> My point is that an API that involves a file like /proc/PID/kill is
>>> very tricky to get right. Here are some considerations:
>>
>> Moot. write(2) for this interface is off the table anyway. The right
>> approach here is a system call that accepts a /proc/pid directory file
>> descriptor, a signal number, and a signal information field (as in
>> sigqueue(2)).
>
> If we did not have the permission check challenges and could perform
> the permission checks in open, write(2) would be on the table.
> Performing write(2) would only be concrend about data.
>
> Unfortunately we have setresuid and exec which make that infeasible
> for the kill operations.
>
>>> Now if we had an ioctlat() API, maybe it would make sense. But we
>>> don't, and I think it would be a bit crazy to add one.
>>
>> A process is not a driver. Why won't this idea of using an ioctl for
>> the kill-process-by-dfd thing just won't die? An ioctl has *zero*
>> advantage in this context.
>
> An ioctl has an advantage in implementation complexity. An ioctl is
> very much easier to wire up that a system call.
>
> I don't know if that outweighs ioctls disadvantages in long term
> maintainability.
It's not just maintainability. It's safety. We want to expose the new
kill interface to userspace via some kill(1) extension, probably. So
you should be able to write something like `cd /proc/12345 && kill
--by-handle .`. How does kill --by-handle know that it's safe to
perform the kill-by-proc-dfd operation on the file descriptor that it
opens? If the kill operation is an ioctl, you could pass it
/proc/self/fd/whatever of a completely different type; kill would call
ioctl on whatever FD it got, and potentially do a completely random
thing instead of killing a process. In the same situation, a new
system call would fail reliably. Yes, kill could check that the device
numbers of the file it opened matched proc's somehow, but that's
annoying and error-prone and nobody's going to bother in practice. A
new system call, by contrast, fails safe.
I really don't want to give up safety and fail-safe behavior forever
just because it's annoying, today, to wire up a new system call. (The
new table-driven system call stuff, if it ever lands, would make
things easier.)
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 17:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Randy Dunlap, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, Jan Engelhardt
In-Reply-To: <CALCETrUeNZPfrSYa9vH5Ukrk1Y+Kb9GkZOh6LkqG6Z9NpK5P0w@mail.gmail.com>
On Sun, Nov 18, 2018 at 9:42 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 9:24 AM Daniel Colascione <dancol@google.com> wrote:
>> Assuming we don't broaden exit status readability (which would make a
>> lot of things simpler), the exit notification mechanism must work like
>> this: if you can see a process in /proc, you should be able to wait on
>> it. If you learn that process's exit status through some other means
>> --- e.g., you're the process's parent, you can ptrace the process, you
>> have CAP_WHATEVER_IT_IS_ --- then you should be able to learn the fate
>> of the process. Otherwise you just be able to learn that the process
>> exited.
>
> Sounds reasonable to me. Except for the obvious turd that, if you
> open /proc/PID/whatever, and the process calls execve(), then the
> resulting semantics are awkward at best.
A process calling execve does not give up its logical identity. Lots
of programs exec themselves, e.g., to reload configuration.
>> > Windows has an easy time of it because
>>
>> Windows has an easier time of it because it doesn't use an ad-hoc
>> ambient authority permission model. In Windows, if you can open a
>> handle to do something, that handle lets you do the thing. Period.
>> There's none of this "well, I opened this process FD, but since I
>> opened it, the process called setuid, so now I can't get its exit
>> status" nonsense. Privilege elevation is always accomplished via a
>> separate call to CreateProcessWithToken, which creates a *new* process
>> with the elevated privileges. An existing process can't suddenly and
>> magically become this special thing that you can't inspect, but that
>> has the same PID and identity as this other process that you used to
>> be able to inspect. The model is just better, because permission is
>> baked into the HANDLE. Now, that ship has sailed. We're stuck with
>> setreuid and exec. But let's be clear about what's causing the
>> complexity.
>
> I'm not entirely sure that ship has sailed. In the kernel, we already
> have a bit of a distinction between a pid (and tid, etc -- I'm
> referring to struct pid) and a task. If we make a new
> process-management API, we could put a distinction like this into the
> API.
It would be a disaster to have different APIs give callers a different
idea of process identity over its lifetime. The precedent is
well-established that execve and setreuid do not change a process's
identity. Invaliding some identifiers but not others in response to
supposedly-internal things a process might do under rare circumstances
is creating a bug machine..
> setresuid() has no effect
> here -- if you have W access to the process and the process calls
> setresuid(), you still have W access.
Now you've created a situation in which an operation that security
policy previously blocked now becomes possible, invaliding previous
designs based on the old security invariant. That's the definition of
introducing a security hole.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:45 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Daniel Colascione, Andrew Lutomirski, Christian Brauner, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook
In-Reply-To: <875zwu46z1.fsf@xmission.com>
On Sun, Nov 18, 2018 at 9:43 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Daniel Colascione <dancol@google.com> writes:
>
> > On Sun, Nov 18, 2018 at 9:13 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
> >>>
> >>> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >>> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
> >>> >>
> >>> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >>> >> > I fully agree that a more comprehensive, less expensive API for
> >>> >> > managing processes would be nice. But I also think that this patch
> >>> >> > (using the directory fd and ioctl) is better from a security
> >>> >> > perspective than using a new file in /proc.
> >>> >>
> >>> >> That's an assertion, not an argument. And I'm not opposed to an
> >>> >> operation on the directory FD, now that it's clear Linus has banned
> >>> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
> >>> >> with a system call instead of a less-reliable ioctl due to the
> >>> >> inherent namespace collision issues in ioctl command names.
> >>> >
> >>> > Linus banned it because of bugs iike the ones in the patch.
> >>>
> >>> Maybe: he didn't provide a reason. What's your point?
> >>
> >> My point is that an API that involves a file like /proc/PID/kill is
> >> very tricky to get right. Here are some considerations:
> >
> > Moot. write(2) for this interface is off the table anyway. The right
> > approach here is a system call that accepts a /proc/pid directory file
> > descriptor, a signal number, and a signal information field (as in
> > sigqueue(2)).
>
> If we did not have the permission check challenges and could perform
> the permission checks in open, write(2) would be on the table.
> Performing write(2) would only be concrend about data.
>
> Unfortunately we have setresuid and exec which make that infeasible
> for the kill operations.
setresuid() should be irrelevant. If you had permission to kill a
process and the process calls setresuid(), you should still have
permission to kill it.
For execve(), we could make execve() invalidate the fd. (See other email.)
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:44 UTC (permalink / raw)
To: Christian Brauner
Cc: Andrew Lutomirski, Daniel Colascione, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, David Howells
In-Reply-To: <20181118174148.nvkc4ox2uorfatbm@brauner.io>
On Sun, Nov 18, 2018 at 9:42 AM Christian Brauner <christian@brauner.io> wrote:
>
> On Sun, Nov 18, 2018 at 07:38:09AM -0800, Andy Lutomirski wrote:
> > On Sun, Nov 18, 2018 at 5:59 AM Daniel Colascione <dancol@google.com> wrote:
> > >
> > > I had been led to believe that the proposal would be a comprehensive
> > > process API, not an ioctl basically equivalent to my previous patch.
> > > If you had a more comprehensive proposal, please just share it on LKML
> > > instead of limiting the discussion to those able to attend these
> > > various conferences. If there's some determined opposition to a
> > > general new process API, this opposition needs a fair and full airing,
> > > as not everyone can attend these conferences.
> > >
> > > On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> > > > With this patch an open() call on /proc/<pid> will give userspace a handle
> > > > to struct pid of the process associated with /proc/<pid>. This allows to
> > > > maintain a stable handle on a process.
> > > > I have been discussing various approaches extensively during technical
> > > > conferences this year culminating in a long argument with Eric at Linux
> > > > Plumbers. The general consensus was that having a handle on a process
> > > > will be something that is very simple and easy to maintain
> > >
> > > ioctls are the opposite of "easy to maintain". Their
> > > file-descriptor-specific behavior makes it difficult to use the things
> > > safely. If you want to take this approach, please make a new system
> > > call. An ioctl is just a system call with a very strange spelling and
> > > unfortunate collision semantics.
> > >
> > > > with the
> > > > option of being extensible via a more advanced api if the need arises.
> > >
> > > The need *has* arisen; see my exithand patch.
> > >
> > > > I
> > > > believe that this patch is the most simple, dumb, and therefore
> > > > maintainable solution.
> > > >
> > > > The need for this has arisen in order to reliably kill a process without
> > > > running into issues of the pid being recycled as has been described in the
> > > > rejected patch [1].
> > >
> > > That patch was not "rejected". It was tabled pending the more
> > > comprehensive process API proposal that was supposed to have emerged.
> > > This patch is just another variant of the sort of approach we
> > > discussed on that patch's thread here. As I mentioned on that thread,
> > > the right approach option is a new system call, not an ioctl.
> > >
> > > To fulfill the need described in that patchset a new
> > > > ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> > > > process via a file descriptor:
> > > >
> > > > int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> > > > ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> > > > close(fd);
> > > >
> > > > Note, the stable handle will allow us to carefully extend this feature in
> > > > the future.
> > >
> > > We still need the ability to synchronously wait on a process's death,
> > > as in my patch set. I will be refreshing that patch set.
> >
> > I fully agree that a more comprehensive, less expensive API for
> > managing processes would be nice. But I also think that this patch
> > (using the directory fd and ioctl) is better from a security
> > perspective than using a new file in /proc.
> >
> > I have an old patch to make proc directory fds pollable:
> >
> > https://lore.kernel.org/patchwork/patch/345098/
> >
> > That patch plus the one in this thread might make a nice addition to
> > the kernel even if we expect something much better to come along
> > later.
>
> I agree. Eric's point was to make the first implementation of this as
> simple as possible that's why this patch is intentionally almost
> trivial. And I like it for its simplicity.
>
> I had a more comprehensive API proposal of which open(/proc/<pid>) was a
> part. I didn't send out alongside this patch as Eric clearly prefered to
> only have the /proc/<pid> part. Here is the full proposal as I intended
> to originally send it out:
>
> The gist is to have file descriptors for processes which is obviously not a new
> idea. This has been done before in other OSes and it has been tried before in
> Linux [2], [3] (Thanks to Kees for pointing out these patches.). So I want to
> make it very clear that I'm not laying claim to this being my or even a novel
> idea in any way. However, I want to diverge from previous approaches with my
> suggestion. (Though I can't be sure that there's not something similar in other
> OSes already.)
>
> One of the main motivations for having procfds is to have a race-free way of
> configuring, starting, polling, and killing a process. Basically, a process
> lifecycle api if you want to think about it that way. The api should also be
> easily extendable in the future to avoid running into the limitations we
> currently see with the clone*() syscall(s) again.
>
> One of the crucial points of the api is to *separate the configuration
> of a process through a procfd from actually creating the process*.
> This is a crucial property expressed in the open*() system calls. First, get a
> stable handle on an object then allow for ways to configure it. As such the
> procfd api shares the same insight with Al's and David's new mount api.
> (Fwiw, Andy also pointed out similarities with posix_spawn().)
> What I envisioned was to have the following syscalls (multiple name suggestions):
>
> 1. int process_open / proc_open / procopen
> 2. int process_config / proc_config / procconfig or ioctl()-based
> 3. int process_info / proc_info / procinfo or ioctl()-based
> 4. int process_manage / proc_manage / procmanage or ioctl()-based
Emails crossed :(
For process management, I generally like this, although we might do
better if we make execve() effectively invalidate the handle. Then we
avoid a bunch of nasty permission issues. For process *creation*, we
have the problem that libc authors feel that they can't safely use fds
at all. There was a proposal for "high fds" a long time back to solve
that. We might finally need to do something like that.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Eric W. Biederman @ 2018-11-18 17:43 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Christian Brauner, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <CAKOZues3_uh1mnQr=VPCxoWiY0wfiUq0oFXcVuiHF7rPmUXsuw@mail.gmail.com>
Daniel Colascione <dancol@google.com> writes:
> On Sun, Nov 18, 2018 at 9:13 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>>>
>>> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>>> >>
>>> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>>> >> > I fully agree that a more comprehensive, less expensive API for
>>> >> > managing processes would be nice. But I also think that this patch
>>> >> > (using the directory fd and ioctl) is better from a security
>>> >> > perspective than using a new file in /proc.
>>> >>
>>> >> That's an assertion, not an argument. And I'm not opposed to an
>>> >> operation on the directory FD, now that it's clear Linus has banned
>>> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
>>> >> with a system call instead of a less-reliable ioctl due to the
>>> >> inherent namespace collision issues in ioctl command names.
>>> >
>>> > Linus banned it because of bugs iike the ones in the patch.
>>>
>>> Maybe: he didn't provide a reason. What's your point?
>>
>> My point is that an API that involves a file like /proc/PID/kill is
>> very tricky to get right. Here are some considerations:
>
> Moot. write(2) for this interface is off the table anyway. The right
> approach here is a system call that accepts a /proc/pid directory file
> descriptor, a signal number, and a signal information field (as in
> sigqueue(2)).
If we did not have the permission check challenges and could perform
the permission checks in open, write(2) would be on the table.
Performing write(2) would only be concrend about data.
Unfortunately we have setresuid and exec which make that infeasible
for the kill operations.
>> Now if we had an ioctlat() API, maybe it would make sense. But we
>> don't, and I think it would be a bit crazy to add one.
>
> A process is not a driver. Why won't this idea of using an ioctl for
> the kill-process-by-dfd thing just won't die? An ioctl has *zero*
> advantage in this context.
An ioctl has an advantage in implementation complexity. An ioctl is
very much easier to wire up that a system call.
I don't know if that outweighs ioctls disadvantages in long term
maintainability.
Eric
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:42 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Lutomirski, Randy Dunlap, Christian Brauner,
Eric W. Biederman, LKML, Serge E. Hallyn, Jann Horn,
Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, Jan Engelhardt
In-Reply-To: <CAKOZuesCKo4GH9fdum2EUFLrtTWam3aizcDQUn3-vCYg4T1P8w@mail.gmail.com>
On Sun, Nov 18, 2018 at 9:24 AM Daniel Colascione <dancol@google.com> wrote:
> Assuming we don't broaden exit status readability (which would make a
> lot of things simpler), the exit notification mechanism must work like
> this: if you can see a process in /proc, you should be able to wait on
> it. If you learn that process's exit status through some other means
> --- e.g., you're the process's parent, you can ptrace the process, you
> have CAP_WHATEVER_IT_IS_ --- then you should be able to learn the fate
> of the process. Otherwise you just be able to learn that the process
> exited.
Sounds reasonable to me. Except for the obvious turd that, if you
open /proc/PID/whatever, and the process calls execve(), then the
resulting semantics are awkward at best.
>
> > Windows has an easy time of it because
>
> Windows has an easier time of it because it doesn't use an ad-hoc
> ambient authority permission model. In Windows, if you can open a
> handle to do something, that handle lets you do the thing. Period.
> There's none of this "well, I opened this process FD, but since I
> opened it, the process called setuid, so now I can't get its exit
> status" nonsense. Privilege elevation is always accomplished via a
> separate call to CreateProcessWithToken, which creates a *new* process
> with the elevated privileges. An existing process can't suddenly and
> magically become this special thing that you can't inspect, but that
> has the same PID and identity as this other process that you used to
> be able to inspect. The model is just better, because permission is
> baked into the HANDLE. Now, that ship has sailed. We're stuck with
> setreuid and exec. But let's be clear about what's causing the
> complexity.
I'm not entirely sure that ship has sailed. In the kernel, we already
have a bit of a distinction between a pid (and tid, etc -- I'm
referring to struct pid) and a task. If we make a new
process-management API, we could put a distinction like this into the
API. As a straw-man proposal (highly incomplete and probably wrong,
but maybe it gets the idea across):
Have a way to get an fd that refers to a "running program". (I'm
calling it that to distinguish it from "task" and "pid", both of which
already mean something.) You'd be able to open such an fd given a
pid, and your permissions would be checked at that time. R access
means you can read the running program's memory and otherwise
introspect it. W means you can modify it's memory and otherwise mess
with it. X means you can send it signals. We might need more bits to
really do this right.
Now here's the kicker: if the "running program" calls execve(), it
goes away. The fd gets some sort of notification that this happened
and there's an API to get a handle to the new running program *if the
caller has the appropriate permissions*. setresuid() has no effect
here -- if you have W access to the process and the process calls
setresuid(), you still have W access.
To make this fully useful, we'd probably want to elaborate it with a
race-free way to track all descendents and, if needed, kill them all,
subject to permissions.
This API ought to be extensible to replace ptrace() eventually.
Does this seem like a reasonable direction to go in?
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Christian Brauner @ 2018-11-18 17:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook, David Howells
In-Reply-To: <CALCETrUd2Y2MmO8Qx8aUvQDOTNiyj35Y7bctPBZe0p2i1EUiLw@mail.gmail.com>
On Sun, Nov 18, 2018 at 07:38:09AM -0800, Andy Lutomirski wrote:
> On Sun, Nov 18, 2018 at 5:59 AM Daniel Colascione <dancol@google.com> wrote:
> >
> > I had been led to believe that the proposal would be a comprehensive
> > process API, not an ioctl basically equivalent to my previous patch.
> > If you had a more comprehensive proposal, please just share it on LKML
> > instead of limiting the discussion to those able to attend these
> > various conferences. If there's some determined opposition to a
> > general new process API, this opposition needs a fair and full airing,
> > as not everyone can attend these conferences.
> >
> > On Sun, Nov 18, 2018 at 3:17 AM, Christian Brauner <christian@brauner.io> wrote:
> > > With this patch an open() call on /proc/<pid> will give userspace a handle
> > > to struct pid of the process associated with /proc/<pid>. This allows to
> > > maintain a stable handle on a process.
> > > I have been discussing various approaches extensively during technical
> > > conferences this year culminating in a long argument with Eric at Linux
> > > Plumbers. The general consensus was that having a handle on a process
> > > will be something that is very simple and easy to maintain
> >
> > ioctls are the opposite of "easy to maintain". Their
> > file-descriptor-specific behavior makes it difficult to use the things
> > safely. If you want to take this approach, please make a new system
> > call. An ioctl is just a system call with a very strange spelling and
> > unfortunate collision semantics.
> >
> > > with the
> > > option of being extensible via a more advanced api if the need arises.
> >
> > The need *has* arisen; see my exithand patch.
> >
> > > I
> > > believe that this patch is the most simple, dumb, and therefore
> > > maintainable solution.
> > >
> > > The need for this has arisen in order to reliably kill a process without
> > > running into issues of the pid being recycled as has been described in the
> > > rejected patch [1].
> >
> > That patch was not "rejected". It was tabled pending the more
> > comprehensive process API proposal that was supposed to have emerged.
> > This patch is just another variant of the sort of approach we
> > discussed on that patch's thread here. As I mentioned on that thread,
> > the right approach option is a new system call, not an ioctl.
> >
> > To fulfill the need described in that patchset a new
> > > ioctl() PROC_FD_SIGNAL is added. It can be used to send signals to a
> > > process via a file descriptor:
> > >
> > > int fd = open("/proc/1234", O_DIRECTORY | O_CLOEXEC);
> > > ioctl(fd, PROC_FD_SIGNAL, SIGKILL);
> > > close(fd);
> > >
> > > Note, the stable handle will allow us to carefully extend this feature in
> > > the future.
> >
> > We still need the ability to synchronously wait on a process's death,
> > as in my patch set. I will be refreshing that patch set.
>
> I fully agree that a more comprehensive, less expensive API for
> managing processes would be nice. But I also think that this patch
> (using the directory fd and ioctl) is better from a security
> perspective than using a new file in /proc.
>
> I have an old patch to make proc directory fds pollable:
>
> https://lore.kernel.org/patchwork/patch/345098/
>
> That patch plus the one in this thread might make a nice addition to
> the kernel even if we expect something much better to come along
> later.
I agree. Eric's point was to make the first implementation of this as
simple as possible that's why this patch is intentionally almost
trivial. And I like it for its simplicity.
I had a more comprehensive API proposal of which open(/proc/<pid>) was a
part. I didn't send out alongside this patch as Eric clearly prefered to
only have the /proc/<pid> part. Here is the full proposal as I intended
to originally send it out:
The gist is to have file descriptors for processes which is obviously not a new
idea. This has been done before in other OSes and it has been tried before in
Linux [2], [3] (Thanks to Kees for pointing out these patches.). So I want to
make it very clear that I'm not laying claim to this being my or even a novel
idea in any way. However, I want to diverge from previous approaches with my
suggestion. (Though I can't be sure that there's not something similar in other
OSes already.)
One of the main motivations for having procfds is to have a race-free way of
configuring, starting, polling, and killing a process. Basically, a process
lifecycle api if you want to think about it that way. The api should also be
easily extendable in the future to avoid running into the limitations we
currently see with the clone*() syscall(s) again.
One of the crucial points of the api is to *separate the configuration
of a process through a procfd from actually creating the process*.
This is a crucial property expressed in the open*() system calls. First, get a
stable handle on an object then allow for ways to configure it. As such the
procfd api shares the same insight with Al's and David's new mount api.
(Fwiw, Andy also pointed out similarities with posix_spawn().)
What I envisioned was to have the following syscalls (multiple name suggestions):
1. int process_open / proc_open / procopen
2. int process_config / proc_config / procconfig or ioctl()-based
3. int process_info / proc_info / procinfo or ioctl()-based
4. int process_manage / proc_manage / procmanage or ioctl()-based
and the following procfs extension:
int procfd = open("/proc/<pid>", O_DIRECTORY | O_CLOEXEC);
Some of you will notice right away that we could replace 2-4 with ioctl()s.
#### process_open()
will return an fd that creates a process context.
The fd returned by process_open() does neither refer to any existing process
nor has the process actually been started yet. So non-configuration operations
on it or trying to interact with it would fail with e.g. ESRCH/EINVAL.
#### process_config() / ioctl()
takes an fd returned by process_open() and can be used to configure a process
context *before it is alive*.
Some things that I would like to be able to do with this syscall are:
- configure signals
- set clone flags
- write idmappings if the process runs in a new user namespace
- configure what happens when all procfds referring to the process are gone
- ...
Just to get a very rough feel for this without detailing parameters right now:
/* process should have own mountns */
process_config/ioctl(fd, PROC_SET_FLAG, CLONE_NEWNS, <potentially additional arguments>)
/* process should get SIGKILL when all procfds are closed */
process_config/ioctl(fd, PROC_SET_CLOSE, SIGKILL, <potentially additional arguments>)
After the caller is done configuring the process there would be a final step:
process_config/ioctl(fd, PROC_CREATE, 0, <potentially additional arguments>)
which would create the process and (either as return value or through a
parameter) return the pid of the newly created process.
These fds should be pollable (though this is maybe out of scope for a first
implementation). In combination with the split between getting an fd for a
process context and starting the process would this would then allow for nice
things such as adding an fd gotten via process_open() to an epoll() instance
where other processes can poll the fd to e.g. (given appropriate privileges)
get an event when process_config/ioctl()(fd, PROC_CREATE, *, <potentially
additional arguments>) has actually started the process or it exited.
#### int process_info / ioctl()
allows to retrieve information about a process (e.g. signals, namespaces, or
even information available through getrusage()).
This would be a more performant and race-free way then parsing through various
files in /proc. I remember quite some people asking for a variation of this.
#### process_manage / ioctl()
allows to interact/manage a process through a procfd.
Specifically, one would be able to send signals to the process, retrieve the
exit status from it etc. Here is an example to get a feel for it:
/* send SIGTERM to process /
process_manage/ioctl(fd, PROC_SIGNAL, SIGTERM, <potentially additional arguments>)
/* block until the process has exited and retrieve exit information via
* <potentially additional arguments>.
* One could also make it possible to specify a timeout here.
*/
process_manage/ioctl(fd, PROC_WAIT, 0, <potentially additional arguments>)
#### /proc/<pid>
allow to get a procfd for an existing process.
This adds a new file "handle" to /proc that serves as a way to get a procfd for
a process.
I hope that's enough information for now without too much detail.
I think that /proc/<pid> is probably the easiest to target and that I
prototyped.
[1]: https://lkml.org/lkml/2018/10/30/118
[2]: https://lkml.kernel.org/r/cover.1426180120.git.josh@joshtriplett.org
[3]: https://lore.kernel.org/lkml/2279556.Wl6mCVq5Zi@tjmaciei-mobl4
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 17:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Randy Dunlap, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook, Jan Engelhardt
In-Reply-To: <CALCETrVg71XBv-gMOtL-m0Dd0HNz8_oXOUDSWin5LeViAL0UYA@mail.gmail.com>
On Sun, Nov 18, 2018 at 9:09 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 8:49 AM Daniel Colascione <dancol@google.com> wrote:
>>
>> On Sun, Nov 18, 2018 at 8:33 AM, Randy Dunlap <rdunlap@infradead.org> wrote:
>> > On 11/18/18 8:17 AM, Andy Lutomirski wrote:
>> >> On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>> >>>
>> >>> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> >>>> I fully agree that a more comprehensive, less expensive API for
>> >>>> managing processes would be nice. But I also think that this patch
>> >>>> (using the directory fd and ioctl) is better from a security
>> >>>> perspective than using a new file in /proc.
>> >>>
>> >>> That's an assertion, not an argument. And I'm not opposed to an
>> >>> operation on the directory FD, now that it's clear Linus has banned
>> >>> "write(2)-as-a-command" APIs. I just insist that we implement the API
>> >>> with a system call instead of a less-reliable ioctl due to the
>> >>> inherent namespace collision issues in ioctl command names.
>> >>
>> >> Linus banned it because of bugs iike the ones in the patch.
>> >>
>> >>>
>> >>>> I have an old patch to make proc directory fds pollable:
>> >>>>
>> >>>> https://lore.kernel.org/patchwork/patch/345098/
>> >>>>
>> >>>> That patch plus the one in this thread might make a nice addition to
>> >>>> the kernel even if we expect something much better to come along
>> >>>> later.
>> >>>
>> >>> I've always commented on that patch. You never addressed my technical
>> >>> objections. Why are you bringing up this patch again as if that
>> >>> discussion had never happened? To review, that patch has various race
>> >>> conditions
>> >>
>> >> I don't think I ever saw that review.
>> >>
>> >>> and even if it were technically correct, it'd be an abuse
>> >>> of directory objects (in what other circumstance do we poll
>> >>> directories?) and not logically generalizable to a model in which we
>> >>> expose process exit status via the exit-monitoring API.
>> >>
>> >> I agree it's weird. It might be better to have /proc/PID/exit_status
>> >> and make *that* pollable.
>> >>
>> >
>> > If there is a new exit_status file, it could even be more than
>> > 8 bits of exit status:
>> >
>> > See https://lore.kernel.org/lkml/alpine.LSU.2.20.1507091257010.9602@nerf40.vanv.qr/T/#u
>> > and http://austingroupbugs.net/view.php?id=594#c1317
>>
>> First of all, as I discussed in [1], we need to first figure out *who*
>> should have access to the process exit information. FreeBSD appears to
>> make it public without disaster, and if we make exit status public, a
>> lot of problems just disappear.
>
> I kind of want to go in the other direction of making a lot of process
> information (especially cmdline) less publicly accessible.
Okay. That has nothing to do with exit status. Please address the
points related to the API we're discussing and that I raised in the
other thread.
Assuming we don't broaden exit status readability (which would make a
lot of things simpler), the exit notification mechanism must work like
this: if you can see a process in /proc, you should be able to wait on
it. If you learn that process's exit status through some other means
--- e.g., you're the process's parent, you can ptrace the process, you
have CAP_WHATEVER_IT_IS_ --- then you should be able to learn the fate
of the process. Otherwise you just be able to learn that the process
exited.
> Windows has an easy time of it because
Windows has an easier time of it because it doesn't use an ad-hoc
ambient authority permission model. In Windows, if you can open a
handle to do something, that handle lets you do the thing. Period.
There's none of this "well, I opened this process FD, but since I
opened it, the process called setuid, so now I can't get its exit
status" nonsense. Privilege elevation is always accomplished via a
separate call to CreateProcessWithToken, which creates a *new* process
with the elevated privileges. An existing process can't suddenly and
magically become this special thing that you can't inspect, but that
has the same PID and identity as this other process that you used to
be able to inspect. The model is just better, because permission is
baked into the HANDLE. Now, that ship has sailed. We're stuck with
setreuid and exec. But let's be clear about what's causing the
complexity.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Daniel Colascione @ 2018-11-18 17:17 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Christian Brauner, Eric W. Biederman, LKML, Serge E. Hallyn,
Jann Horn, Andrew Morton, Oleg Nesterov, Aleksa Sarai, Al Viro,
Linux FS Devel, Linux API, Tim Murray, Kees Cook
In-Reply-To: <CALCETrXNBNcASRtxTL_aO-KSj4ahPRpXKLUdHaWG3sPjsWx3CQ@mail.gmail.com>
On Sun, Nov 18, 2018 at 9:13 AM, Andy Lutomirski <luto@kernel.org> wrote:
> On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>>
>> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
>> >>
>> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
>> >> > I fully agree that a more comprehensive, less expensive API for
>> >> > managing processes would be nice. But I also think that this patch
>> >> > (using the directory fd and ioctl) is better from a security
>> >> > perspective than using a new file in /proc.
>> >>
>> >> That's an assertion, not an argument. And I'm not opposed to an
>> >> operation on the directory FD, now that it's clear Linus has banned
>> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
>> >> with a system call instead of a less-reliable ioctl due to the
>> >> inherent namespace collision issues in ioctl command names.
>> >
>> > Linus banned it because of bugs iike the ones in the patch.
>>
>> Maybe: he didn't provide a reason. What's your point?
>
> My point is that an API that involves a file like /proc/PID/kill is
> very tricky to get right. Here are some considerations:
Moot. write(2) for this interface is off the table anyway. The right
approach here is a system call that accepts a /proc/pid directory file
descriptor, a signal number, and a signal information field (as in
sigqueue(2)).
> Now if we had an ioctlat() API, maybe it would make sense. But we
> don't, and I think it would be a bit crazy to add one.
A process is not a driver. Why won't this idea of using an ioctl for
the kill-process-by-dfd thing just won't die? An ioctl has *zero*
advantage in this context.
^ permalink raw reply
* Re: [PATCH] proc: allow killing processes via file descriptors
From: Andy Lutomirski @ 2018-11-18 17:13 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andrew Lutomirski, Christian Brauner, Eric W. Biederman, LKML,
Serge E. Hallyn, Jann Horn, Andrew Morton, Oleg Nesterov,
Aleksa Sarai, Al Viro, Linux FS Devel, Linux API, Tim Murray,
Kees Cook
In-Reply-To: <CAKOZuetuuxW+yFnyaqS5kW6gkVMstLa8D-7SgYtrLBUQB+sgDg@mail.gmail.com>
On Sun, Nov 18, 2018 at 8:29 AM Daniel Colascione <dancol@google.com> wrote:
>
> On Sun, Nov 18, 2018 at 8:17 AM, Andy Lutomirski <luto@kernel.org> wrote:
> > On Sun, Nov 18, 2018 at 7:53 AM Daniel Colascione <dancol@google.com> wrote:
> >>
> >> On Sun, Nov 18, 2018 at 7:38 AM, Andy Lutomirski <luto@kernel.org> wrote:
> >> > I fully agree that a more comprehensive, less expensive API for
> >> > managing processes would be nice. But I also think that this patch
> >> > (using the directory fd and ioctl) is better from a security
> >> > perspective than using a new file in /proc.
> >>
> >> That's an assertion, not an argument. And I'm not opposed to an
> >> operation on the directory FD, now that it's clear Linus has banned
> >> "write(2)-as-a-command" APIs. I just insist that we implement the API
> >> with a system call instead of a less-reliable ioctl due to the
> >> inherent namespace collision issues in ioctl command names.
> >
> > Linus banned it because of bugs iike the ones in the patch.
>
> Maybe: he didn't provide a reason. What's your point?
My point is that an API that involves a file like /proc/PID/kill is
very tricky to get right. Here are some considerations:
1. The .write handler for the file must not behave differently
depending on current. So we can't check the caller's creds. (There
are legacy things that are generally buggy that look at current's
creds in write handlers. They're legacy, and they're almost
universally at least a little bit buggy, and many have been
exploitable.)
2. Even if we have ioctl() or a new syscall on /proc/PID/kill, we at
least have to define whether *opening* kill checks any credentials.
It probably shouldn't, since I don't see what the semantics should be.
3. The current Linux kill_pid stuff doesn't take a cred parameter, so
it's not so easy to check f_cred even if we wanted to.
So the simplest implementation using /proc/PID/kill would be for .open
to do essentially nothing and for ioctl or a new syscall to check
credentials as usual. But this seems to have no technical advantage
over just using /proc/PID itself, and it's a good deal slower, as it
requires an open/close cycle.
Now if we had an ioctlat() API, maybe it would make sense. But we
don't, and I think it would be a bit crazy to add one.
--Andy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox