prctl: add PR_[GS]ET_PDEATHSIG_PROC

PR_SET_PDEATHSIG_PROC sets a process-based death signal.

Unlike
PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

This can be used for sandboxing when combined with a seccomp filter.

There have been previous attempts to support this by changing the behavior of PR_SET_PDEATHSIG. However, that would break existing applications. See https://marc.info/?l=linux-kernel&m=117621804801689
and https://bugzilla.kernel.org/show_bug.cgi?id=43300

Signed-off-by Jürg Billeter <[email protected]>

---
 fs/exec.c                    |  1 +
 include/linux/sched/signal.h |  3 +++
 include/uapi/linux/prctl.h   |  4 ++++
 kernel/cred.c                |  1 +
 kernel/exit.c                |  4 ++++
 kernel/fork.c                |  2 ++
 kernel/sys.c                 | 11 +++++++++++
 security/apparmor/lsm.c      |  1 +
 security/selinux/hooks.c     |  1 +
 9 files changed, 28 insertions(+)

fs/exec.c +1 -0

index 01a9fb9d8ac3..bb389c3c596d 100644
@@ -1353,6 +1353,7 @@ void setup_new_exec(struct linux_binprm * bprm)
 	if (bprm->secureexec) {
 		/* Make sure parent cannot signal privileged process. */
 		current->pdeath_signal = 0;
+		current->signal->pdeath_signal_proc = 0;
 
 		/*
 		 * For secureexec, reset the stack limit to sane default to

include/linux/sched/signal.h +3 -0

index 2a0dd40b15db..c5c137e5ef39 100644
@@ -103,6 +103,9 @@ struct signal_struct {
 	int			group_stop_count;
 	unsigned int		flags; /* see SIGNAL_* flags below */
 
+	/* The signal sent when the parent dies: */
+	int			pdeath_signal_proc;
+
 	/*
 	 * PR_SET_CHILD_SUBREAPER marks a process, like a service
 	 * manager, to re-parent orphan (double-forking) child processes

include/uapi/linux/prctl.h +4 -0

index a8d0759a9e40..04508e81d4f2 100644
@@ -197,4 +197,8 @@ struct prctl_mm_map {
 # define PR_CAP_AMBIENT_LOWER		3
 # define PR_CAP_AMBIENT_CLEAR_ALL	4
 
+/* Process-based variant of PDEATHSIG */
+#define PR_SET_PDEATHSIG_PROC		48
+#define PR_GET_PDEATHSIG_PROC		49
+
 #endif /* _LINUX_PRCTL_H */

kernel/cred.c +1 -0

index ecf03657e71c..0192a94670e1 100644
@@ -448,6 +448,7 @@ int commit_creds(struct cred *new)
 		if (task->mm)
 			set_dumpable(task->mm, suid_dumpable);
 		task->pdeath_signal = 0;
+		task->signal->pdeath_signal_proc = 0;
 		smp_wmb();
 	}
 

kernel/exit.c +4 -0

index a35d8a17e01f..1be0616239e0 100644
@@ -635,6 +635,10 @@ static void reparent_leader(struct task_struct *father, struct task_struct *p,
 	if (unlikely(p->exit_state == EXIT_DEAD))
 		return;
 
+	if (p->signal->pdeath_signal_proc)
+		group_send_sig_info(p->signal->pdeath_signal_proc,
+				    SEND_SIG_NOINFO, p);
+
 	/* We don't want people slaying init. */
 	p->exit_signal = SIGCHLD;
 

kernel/fork.c +2 -0

index 24a4c0be80d5..f6482392ece9 100644
@@ -1412,6 +1412,8 @@ static int copy_signal(unsigned long clone_flags, struct task_struct *tsk)
 
 	mutex_init(&sig->cred_guard_mutex);
 
+	sig->pdeath_signal_proc = current->signal->pdeath_signal_proc;
+
 	return 0;
 }
 

kernel/sys.c +11 -0

index 2855ee73acd0..c47e92fa5370 100644
@@ -2210,6 +2210,17 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 	case PR_GET_PDEATHSIG:
 		error = put_user(me->pdeath_signal, (int __user *)arg2);
 		break;
+	case PR_SET_PDEATHSIG_PROC:
+		if (!valid_signal(arg2)) {
+			error = -EINVAL;
+			break;
+		}
+		me->signal->pdeath_signal_proc = arg2;
+		break;
+	case PR_GET_PDEATHSIG_PROC:
+		error = put_user(me->signal->pdeath_signal_proc,
+				 (int __user *)arg2);
+		break;
 	case PR_GET_DUMPABLE:
 		error = get_dumpable(me->mm);
 		break;

security/apparmor/lsm.c +1 -0

index 7a82c0f61452..c8bd6b1331c1 100644
@@ -628,6 +628,7 @@ static void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
 	aa_inherit_files(bprm->cred, current->files);
 
 	current->pdeath_signal = 0;
+	current->signal->pdeath_signal_proc = 0;
 
 	/* reset soft limits and set hard limits for the new label */
 	__aa_transition_rlimits(label, new_ctx->label);

security/selinux/hooks.c +1 -0

index ad3b0f53ede0..574d6238f8de 100644
@@ -2527,6 +2527,7 @@ static void selinux_bprm_committing_creds(struct linux_binprm *bprm)
 
 	/* Always clear parent death signal on SID transitions. */
 	current->pdeath_signal = 0;
+	current->signal->pdeath_signal_proc = 0;
 
 	/* Check whether the new SID can inherit resource limits from the old
 	 * SID.  If not, reset all soft limits to the lower of the current

Signature · 1 line

2.14.1

Quoted 3 lines · Jürg Billeter

Unlike
PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

but I am still not sure this is right... at least I can't understand the "without race conditions" above.

IOW, the child can do prctl(PR_SET_PDEATHSIG_PROC, SIGKILL) right after fork(), why this is not enough to kill a whole subtree without race conditions?

OTOH. If you want to kill a whole sub-tree then perhaps the exiting process should simply send the ->pdeath_signal_proc to the whole sub-tree? Not that I really think this makes more sense, but if we add the new API we should discuss everything we can.

Say, CLONE_PARENT. Should it succeed if ->pdeath_signal_proc != 0 ?

Quoted 5 lines · Oleg Nesterov

Quoted 3 lines · Jürg Billeter

Unlike
PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

Quoted 5 lines

but I am still not sure this is right... at least I can't understand the "without race conditions" above.

IOW, the child can do prctl(PR_SET_PDEATHSIG_PROC, SIGKILL) right after fork(), why this is not enough to kill a whole subtree without race conditions?

What if the parent dies between fork() and prctl()?

it also makes it relatively easy to enforce
PDEATHSIG_PROC for all descendants of a process.

Quoted 1 line

Say, CLONE_PARENT. Should it succeed if ->pdeath_signal_proc != 0 ?

Yes, I don't see an issue with that. The new process will be a sibling and inheriting pdeath_signal_proc seems sensible to me for this.

Quoted 10 lines · Jürg Billeter

Quoted 8 lines · Oleg Nesterov

Quoted 3 lines · Jürg Billeter

Unlike
PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

but I am still not sure this is right... at least I can't understand the "without race conditions" above.

Quoted 4 lines

Quoted 2 lines

IOW, the child can do prctl(PR_SET_PDEATHSIG_PROC, SIGKILL) right after fork(), why this is not enough to kill a whole subtree without race conditions?

What if the parent dies between fork() and prctl()?

The child will be killed? Sorry, can't understand...

Quoted 4 lines

Quoted 1 line

Say, CLONE_PARENT. Should it succeed if ->pdeath_signal_proc != 0 ?

Yes, I don't see an issue with that. The new process will be a sibling and inheriting pdeath_signal_proc seems sensible to me for this.

I meant, the process created by clone(CLONE_PARENT) won't be killed by pdeath_signal if the creator process exits, exactly because it won't be its child. Not that I think this is wrong.

Quoted 14 lines · Oleg Nesterov

Quoted 13 lines · Jürg Billeter

Quoted 10 lines · Oleg Nesterov

Quoted 3 lines · Jürg Billeter

Unlike
PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

but I am still not sure this is right... at least I can't understand the "without race conditions" above.

IOW, the child can do prctl(PR_SET_PDEATHSIG_PROC, SIGKILL) right after fork(), why this is not enough to kill a whole subtree without race conditions?

Quoted 3 lines

Quoted 1 line

What if the parent dies between fork() and prctl()?

The child will be killed? Sorry, can't understand...

If PR_SET_PDEATHSIG_PROC was not inherited across fork and the parent died between fork() and prctl(PR_SET_PDEATHSIG_PROC, SIGKILL) in the child, the child would not be killed.

PR_SET_PDEATHSIG sets a parent death signal that the calling process will get when its parent thread dies, even when the result of getppid() doesn't change because the calling process is reparented to a different thread in the same parent process. When managing multiple processes, a process-based parent death signal is much more useful. E.g., to avoid stray child processes.

PR_SET_PDEATHSIG_PROC sets a process-based death signal. Unlike PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

This can be used for sandboxing when combined with a seccomp filter.

There have been previous attempts to support this by changing the behavior of PR_SET_PDEATHSIG. However, that would break existing applications. See https://marc.info/?l=linux-kernel&m=117621804801689
and https://bugzilla.kernel.org/show_bug.cgi?id=43300

Quoted 13 lines · On Fri, 29 Sep 2017 14:30:58 +0200 Jürg Billeter <[email protected]> wrote:

PR_SET_PDEATHSIG sets a parent death signal that the calling process will get when its parent thread dies, even when the result of getppid() doesn't change because the calling process is reparented to a different thread in the same parent process. When managing multiple processes, a process-based parent death signal is much more useful. E.g., to avoid stray child processes.

PR_SET_PDEATHSIG_PROC sets a process-based death signal. Unlike PR_SET_PDEATHSIG, this is inherited across fork to allow killing a whole subtree without race conditions.

This can be used for sandboxing when combined with a seccomp filter.

Quoted 4 lines

There have been previous attempts to support this by changing the behavior of PR_SET_PDEATHSIG. However, that would break existing applications. See https://marc.info/?l=linux-kernel&m=117621804801689
and https://bugzilla.kernel.org/show_bug.cgi?id=43300

Are Eric and Oleg OK with this?

A prctl manpage update will be needed, please (cc linux-api).

The code where it calls group_send_sig_info is buggy for pdeath_signal. And it no less buggy for this new case. There is no point to check permissions when sending a signal to yourself. Especially this signal gets cleared during exec with a change of permissions.

I would recommend using:
do_send_sig_info(p->signal->pdeath_signal_proc, SEND_SIG_NOINFO, p, true);

Perhaps with a comment saying that no permission check is needed when sending a signal to yourself.

I don't know what I think about inherit over fork, and the whole tree killing thing. Except when the signal is SIGKILL I don't know if that code does what is intended. So I am a little leary of it.

Quoted 9 lines · Eric W. Biederman

The code where it calls group_send_sig_info is buggy for pdeath_signal. And it no less buggy for this new case. There is no point to check permissions when sending a signal to yourself. Especially this signal gets cleared during exec with a change of permissions.

I would recommend using:
do_send_sig_info(p->signal->pdeath_signal_proc, SEND_SIG_NOINFO, p, true);

Quoted 2 lines

Perhaps with a comment saying that no permission check is needed when sending a signal to yourself.

Depending on how you look at it, one could also argue that the dying parent sends the signal. However, I'm fine with dropping the permission check in v2. I'll also send a patch to change this for the existing pdeath_signal.

Quoted 3 lines

I don't know what I think about inherit over fork, and the whole tree killing thing. Except when the signal is SIGKILL I don't know if that code does what is intended. So I am a little leary of it.

I agree that inheritance across fork is mainly useful for SIGKILL. While non-SIGKILL users could clear the setting after fork(), another option would be to allow the caller to specify whether the setting should be inherited using prctl arg3.

This would allow both, the exact process-based equivalent to pdeath_signal (no inheritance) as well as the interesting SIGKILL case for killing a process tree. Does this sound sensible? I'd be happy to add this to v2.

The process that requests the signal be sent is the process that is receiving the signal. I can see a theoretical need for a permission check in there somewhere (especially as this persists over fork).

Quoted 4 lines · Eric W. Biederman

The process that requests the signal be sent is the process that is receiving the signal. I can see a theoretical need for a permission check in there somewhere (especially as this persists over fork).

Note that it also persists over not just fork, but execve() too.

Yes, the signal is cleared if the e[ug]id/fs[ug]id is changed by exec, but not (for example) if just uid is changed.

Does that matter? Probably not. But signal handling does actually check uid, so it does actually affect signal permission checks across execve.

*Scratches head*
pdeath_signal is cleared during exec if bprm->cap_elevated.

is_setid is set if the uid != eid or gid != egid.

Quoted 3 lines · Eric W. Biederman

*Scratches head*
pdeath_signal is cleared during exec if bprm->cap_elevated.

It's not cleared if we are *releasing* capabilities, which is exactly what might cause a "we can no longer send a signal"

Quoted 1 line

is_setid is set if the uid != eid or gid != egid.

Again, that may be exactly what changes - the original process may have uid != euid, and now we're going from an "we still had a root uid/suid" to "dropping everything to euid".

IOW, we're _dropping_ capabilities, not adding them. Maybe we don't want to allow signaling the original parent any more.

That said, as mentioned, I actually don't think it's a real problem. The real problem is entirely conceptual: yet more complexity in an area that we've already had problems in before.

From the conversation so far it does appear we can do better.

We never signal the orignal parent. We signal the child that requested the pdeath_signal when the original parent dies.

Quoted 3 lines · Eric W. Biederman

We never signal the orignal parent. We signal the child that requested the pdeath_signal when the original parent dies.

Yeah, I keep making that mistake, because I always confuse this with the exit_signal handling.

Just mentally kick me next time I do that: "Christ, Linus, not *again*! Take your damn meds"

Anyway, it's more the "another confusing and fragile special case that will probably not be used very widely and cause confusion because it lacks any test coverage" thing I worry about most.

There is a general need to find out about the death of other processes,
if you are not the parent of the process. I would be inclined to call
it waitfd. Something that you give a pid. It performs a permission check and the pid becomes readable when the process dies. With poll working on the fd, and the fd returning wstatus of the dead child.

Support SIGIO on the fd and you have a signal delivery mechanism, if you want it.

For the kill all children when the parent dies the mechanism you are proposing is escapable. We already have an inescapable version of it with init in a pid namespace. We already have an escapable version of it with orphaned process groups and SIGHUP.

So I would really appreciate a very clear use case for what we are building here. As it appears the killing of children can already be done another way, and that the waiting for the parent can be done better another way.

Quoted 8 lines · Eric W. Biederman

There is a general need to find out about the death of other processes,
if you are not the parent of the process. I would be inclined to call
it waitfd. Something that you give a pid. It performs a permission check and the pid becomes readable when the process dies. With poll working on the fd, and the fd returning wstatus of the dead child.

Support SIGIO on the fd and you have a signal delivery mechanism, if you want it.

File descriptors for processes (waitfd/clonefd) are definitely interesting. Especially if reaping the process (and reparenting its children) is delayed until the last process file descriptor is closed. However, this would be a much larger addition and also less intuitive to use if all you want is killing the process tree.

Quoted 9 lines

For the kill all children when the parent dies the mechanism you are proposing is escapable. We already have an inescapable version of it with init in a pid namespace. We already have an escapable version of it with orphaned process groups and SIGHUP.

So I would really appreciate a very clear use case for what we are building here. As it appears the killing of children can already be done another way, and that the waiting for the parent can be done better another way.

My use case is to provide a way for a process to spawn a child and ensure that no descendants survive when that child dies. Avoiding runaway processes is desirable in many situations. My motivation is very lightweight (nested) sandboxing (every process is potentially sandboxed).

I.e., pid namespaces would be a pretty good fit (assuming they are sufficiently lightweight) but CLONE_NEWPID requires CAP_SYS_ADMIN. User namespaces can help here, but creating tons of user namespaces just for this doesn't sound sensible. MAX_PID_NS_LEVEL could be an issue as well at some point but 32 levels are likely fine in practice.

For my particular scenario I may actually be able to create a single user namespace, run all processes with (namespaced) CAP_SYS_ADMIN and use CLONE_NEWPID for every process. However, I would prefer not requiring CAP_SYS_ADMIN and a regular application that wants to avoid runaway processes for a spawned helper process cannot rely on CAP_SYS_ADMIN.

My plan was to use PR_SET_PDEATHSIG_PROC with PR_NO_NEW_PRIVS and a suitable seccomp filter to prevent changes to pdeath_signal_proc. For my SIGKILL use case it would be even better to simply require PR_NO_NEW_PRIVS and make pdeath_signal_proc sticky, avoiding the need for seccomp. I wanted to keep the differences to the existing PR_SET_PDEATHSIG minimal but if we argue that the non-SIGKILL use case is better solved with waitfd (or maybe the process events connector), we could tailor the prctl for the SIGKILL use case (or support both via prctl arg3).

I have another small patch locally that adds a prctl that restricts kill(2) to direct children of the current thread group for lightweight sandboxing. That would also be redundant if it was possible to use CLONE_NEWPID for every process.

What's actually the reason that CLONE_NEWPID requires CAP_SYS_ADMIN? Does CLONE_NEWPID pose any risks that don't exist for CLONE_NEWUSER|CLONE_NEWPID? Assuming we can't simply drop the CAP_SYS_ADMIN requirement, do you see a better solution for this use case?

Quoted 5 lines · Jürg Billeter

What's actually the reason that CLONE_NEWPID requires CAP_SYS_ADMIN? Does CLONE_NEWPID pose any risks that don't exist for CLONE_NEWUSER|CLONE_NEWPID? Assuming we can't simply drop the CAP_SYS_ADMIN requirement, do you see a better solution for this use case?

CLONE_NEWPID without a permission check would allow runing a setuid root application in a pid namespace. Off the top of my head I can't think of a really good exploit. But when you mess up pid files, and hide information from a privileged application I can completely imagine forcing that application to misbehave in ways the attacker can control. Leading to bad things.

Quoted 7 lines · Eric W. Biederman

Quoted 5 lines · Jürg Billeter

What's actually the reason that CLONE_NEWPID requires CAP_SYS_ADMIN? Does CLONE_NEWPID pose any risks that don't exist for CLONE_NEWUSER|CLONE_NEWPID? Assuming we can't simply drop the CAP_SYS_ADMIN requirement, do you see a better solution for this use case?

Quoted 6 lines

CLONE_NEWPID without a permission check would allow runing a setuid root application in a pid namespace. Off the top of my head I can't think of a really good exploit. But when you mess up pid files, and hide information from a privileged application I can completely imagine forcing that application to misbehave in ways the attacker can control. Leading to bad things.

Could we allow unprivileged CLONE_NEWPID if the no_new_privs bit is set?

My use case is to provide a way for a process to spawn a child and ensure that no descendants survive when that child dies. Avoiding runaway processes is desirable in many situations. My motivation is very lightweight (nested) sandboxing (every process is potentially sandboxed).

I.e., pid namespaces would be a pretty good fit (assuming they are sufficiently lightweight) but CLONE_NEWPID

Quoted 9 lines · Jürg Billeter

My use case is to provide a way for a process to spawn a child and ensure that no descendants survive when that child dies. Avoiding runaway processes is desirable in many situations. My motivation is very lightweight (nested) sandboxing (every process is potentially sandboxed).

I.e., pid namespaces would be a pretty good fit (assuming they are sufficiently lightweight) but CLONE_NEWPID

sorry if this was already discussed, I didn't read this thread yet...

if CLONE_NEWPID is not suitable for any reason. We already have PR_SET_CHILD_SUBREAPER. Perhaps we can simply add another PR_SET_KILL_ALL_DESCEDANTS_ON_EXIT? we can use walk_process_tree() to send SIGKILL.

Quoted 11 lines · Oleg Nesterov

Quoted 9 lines · Jürg Billeter

My use case is to provide a way for a process to spawn a child and ensure that no descendants survive when that child dies. Avoiding runaway processes is desirable in many situations. My motivation is very lightweight (nested) sandboxing (every process is potentially sandboxed).

I.e., pid namespaces would be a pretty good fit (assuming they are sufficiently lightweight) but CLONE_NEWPID

Quoted 6 lines

sorry if this was already discussed, I didn't read this thread yet...

if CLONE_NEWPID is not suitable for any reason. We already have PR_SET_CHILD_SUBREAPER. Perhaps we can simply add another PR_SET_KILL_ALL_DESCEDANTS_ON_EXIT? we can use walk_process_tree() to send SIGKILL.

Yes, this is an option. However, after the discussion in this thread I believe it would be better to drop the CAP_SYS_ADMIN requirement for CLONE_NEWPID (when no_new_privs is set) as this would avoid adding another API and code path for a similar effect. I'm interested in possible security concerns about such a change. Adding Andy Lutomirski to cc.