[PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Oleg Nesterov]

I can't understand why exit_notify() checks capable(CAP_KILL), but this
looks just wrong.

Whatever logic we have to reset ->exit_signal, the bad user can bypass
it if it execs the setuid application before exiting, kill the CAP_KILL
check.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/kernel/exit.c~2_EXIT_NOTIFY	2009-02-13 07:04:12.000000000 +0100
+++ 6.29-rc3/kernel/exit.c	2009-02-25 19:41:57.000000000 +0100
@@ -874,8 +874,7 @@ static void exit_notify(struct task_stru
 	 */
 	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
 	    (tsk->parent_exec_id != tsk->real_parent->self_exec_id ||
-	     tsk->self_exec_id != tsk->parent_exec_id) &&
-	    !capable(CAP_KILL))
+	     tsk->self_exec_id != tsk->parent_exec_id))
 		tsk->exit_signal = SIGCHLD;
 
 	signal = tracehook_notify_death(tsk, &cookie, group_dead);

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Roland McGrath]

> I can't understand why exit_notify() checks capable(CAP_KILL), but this
> looks just wrong.

I don't know either why it's there.  My guess is that it was not actually
thought out specifically, just a "unless capable" exception added when the
security-motivated exclusions (exec_id stuff) were added.

I can't think of any reason not to drop this check.


Thanks,
Roland

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Serge E. Hallyn]

Quoting Roland McGrath (roland@redhat.com):
> > I can't understand why exit_notify() checks capable(CAP_KILL), but this
> > looks just wrong.
> 
> I don't know either why it's there.  My guess is that it was not actually
> thought out specifically, just a "unless capable" exception added when the
> security-motivated exclusions (exec_id stuff) were added.
> 
> I can't think of any reason not to drop this check.

Because of the following test?

#include <stdio.h>
#include <sched.h>
#include <signal.h>
#include <stdlib.h>

int childfn(void *data)
{
	printf("hi there, i'm the child\n");
	sleep(10);
	exit(0);
}

int main()
{
	int stacksize = 4*getpagesize();
	void *stack, *stacktop;

	stack = malloc(stacksize);
	stacktop = stack + stacksize;

	int p = clone(childfn, stacktop, CLONE_PARENT|SIGSTOP, NULL);
	exit(0);
}

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Oleg Nesterov]

On 02/25, Serge E. Hallyn wrote:
>
> Quoting Roland McGrath (roland@redhat.com):
> > > I can't understand why exit_notify() checks capable(CAP_KILL), but this
> > > looks just wrong.
> >
> > I don't know either why it's there.  My guess is that it was not actually
> > thought out specifically, just a "unless capable" exception added when the
> > security-motivated exclusions (exec_id stuff) were added.
> >
> > I can't think of any reason not to drop this check.
>
> Because of the following test?
>
> #include <stdio.h>
> #include <sched.h>
> #include <signal.h>
> #include <stdlib.h>
>
> int childfn(void *data)
> {
> 	printf("hi there, i'm the child\n");
> 	sleep(10);
> 	exit(0);
> }
>
> int main()
> {
> 	int stacksize = 4*getpagesize();
> 	void *stack, *stacktop;
>
> 	stack = malloc(stacksize);
> 	stacktop = stack + stacksize;
>
> 	int p = clone(childfn, stacktop, CLONE_PARENT|SIGSTOP, NULL);
> 	exit(0);
> }

Can't understand... Why do you think CAP_KILL makes things better?

Actually, how can it make any difference in this case?

Oleg.

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 02/25, Serge E. Hallyn wrote:
> >
> > Quoting Roland McGrath (roland@redhat.com):
> > > > I can't understand why exit_notify() checks capable(CAP_KILL), but this
> > > > looks just wrong.
> > >
> > > I don't know either why it's there.  My guess is that it was not actually
> > > thought out specifically, just a "unless capable" exception added when the
> > > security-motivated exclusions (exec_id stuff) were added.
> > >
> > > I can't think of any reason not to drop this check.
> >
> > Because of the following test?
> >
> > #include <stdio.h>
> > #include <sched.h>
> > #include <signal.h>
> > #include <stdlib.h>
> >
> > int childfn(void *data)
> > {
> > 	printf("hi there, i'm the child\n");
> > 	sleep(10);
> > 	exit(0);
> > }
> >
> > int main()
> > {
> > 	int stacksize = 4*getpagesize();
> > 	void *stack, *stacktop;
> >
> > 	stack = malloc(stacksize);
> > 	stacktop = stack + stacksize;
> >
> > 	int p = clone(childfn, stacktop, CLONE_PARENT|SIGSTOP, NULL);
> > 	exit(0);
> > }
> 
> Can't understand... Why do you think CAP_KILL makes things better?
> 
> Actually, how can it make any difference in this case?

Well the check by itself isn't quite right - it seems to me it
should also check whether tsk->euid == parent->uid.  But letting
an unprivileged task send SIGSTOP to a privileged one bc of
some fluke in the task hierarchy doesn't seem right.

-serge

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Oleg Nesterov]

On 02/25, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 02/25, Serge E. Hallyn wrote:
> > >
> > > Quoting Roland McGrath (roland@redhat.com):
> > > > > I can't understand why exit_notify() checks capable(CAP_KILL), but this
> > > > > looks just wrong.
> > > >
> > > > I don't know either why it's there.  My guess is that it was not actually
> > > > thought out specifically, just a "unless capable" exception added when the
> > > > security-motivated exclusions (exec_id stuff) were added.
> > > >
> > > > I can't think of any reason not to drop this check.
> > >
> > > Because of the following test?
> > >
> > > #include <stdio.h>
> > > #include <sched.h>
> > > #include <signal.h>
> > > #include <stdlib.h>
> > >
> > > int childfn(void *data)
> > > {
> > > 	printf("hi there, i'm the child\n");
> > > 	sleep(10);
> > > 	exit(0);
> > > }
> > >
> > > int main()
> > > {
> > > 	int stacksize = 4*getpagesize();
> > > 	void *stack, *stacktop;
> > >
> > > 	stack = malloc(stacksize);
> > > 	stacktop = stack + stacksize;
> > >
> > > 	int p = clone(childfn, stacktop, CLONE_PARENT|SIGSTOP, NULL);
> > > 	exit(0);
> > > }
> >
> > Can't understand... Why do you think CAP_KILL makes things better?
> >
> > Actually, how can it make any difference in this case?
>
> Well the check by itself isn't quite right - it seems to me it
> should also check whether tsk->euid == parent->uid.  But letting
> an unprivileged task send SIGSTOP to a privileged one bc of
> some fluke in the task hierarchy doesn't seem right.

I think you misread this CAP_KILL check.

It does not restrict the unprivileged task to send the signal. Instead,
if the exiting task has CAP_KILL, we bypass other security checks.

Oleg.

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 02/25, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 02/25, Serge E. Hallyn wrote:
> > > Can't understand... Why do you think CAP_KILL makes things better?
> > >
> > > Actually, how can it make any difference in this case?
> >
> > Well the check by itself isn't quite right - it seems to me it
> > should also check whether tsk->euid == parent->uid.  But letting
> > an unprivileged task send SIGSTOP to a privileged one bc of
> > some fluke in the task hierarchy doesn't seem right.
> 
> I think you misread this CAP_KILL check.
> 
> It does not restrict the unprivileged task to send the signal. Instead,
> if the exiting task has CAP_KILL, we bypass other security checks.

?  If the exiting task does not have CAP_KILL, we set the signal to
SIGCHILD (which is deemed safe).

Or, I'm completely misreading...

-serge

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Oleg Nesterov]

On 02/25, Serge E. Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 02/25, Serge E. Hallyn wrote:
> > >
> > > Quoting Oleg Nesterov (oleg@redhat.com):
> > > > On 02/25, Serge E. Hallyn wrote:
> > > > Can't understand... Why do you think CAP_KILL makes things better?
> > > >
> > > > Actually, how can it make any difference in this case?
> > >
> > > Well the check by itself isn't quite right - it seems to me it
> > > should also check whether tsk->euid == parent->uid.  But letting
> > > an unprivileged task send SIGSTOP to a privileged one bc of
> > > some fluke in the task hierarchy doesn't seem right.
> >
> > I think you misread this CAP_KILL check.
> >
> > It does not restrict the unprivileged task to send the signal. Instead,
> > if the exiting task has CAP_KILL, we bypass other security checks.
>
> ?  If the exiting task does not have CAP_KILL,

_and_ (not "or") the execution domains for parent/chils are different,

> we set the signal to
> SIGCHILD (which is deemed safe).

Yes. So why we should not set the signal to SIGCHLD if the task has
CAP_KILL ?

And again, the malicious application can exec the setuid binary before
exit, in this case we never reset ->exit_signal (of course, unless
that binary drops CAP_KILL).

Oleg.

Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> On 02/25, Serge E. Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > On 02/25, Serge E. Hallyn wrote:
> > > >
> > > > Quoting Oleg Nesterov (oleg@redhat.com):
> > > > > On 02/25, Serge E. Hallyn wrote:
> > > > > Can't understand... Why do you think CAP_KILL makes things better?
> > > > >
> > > > > Actually, how can it make any difference in this case?
> > > >
> > > > Well the check by itself isn't quite right - it seems to me it
> > > > should also check whether tsk->euid == parent->uid.  But letting
> > > > an unprivileged task send SIGSTOP to a privileged one bc of
> > > > some fluke in the task hierarchy doesn't seem right.
> > >
> > > I think you misread this CAP_KILL check.
> > >
> > > It does not restrict the unprivileged task to send the signal. Instead,
> > > if the exiting task has CAP_KILL, we bypass other security checks.
> >
> > ?  If the exiting task does not have CAP_KILL,
> 
> _and_ (not "or") the execution domains for parent/chils are different,
> 
> > we set the signal to
> > SIGCHILD (which is deemed safe).
> 
> Yes. So why we should not set the signal to SIGCHLD if the task has
> CAP_KILL ?

Yeah, you're right,  I wasn't thinking right.

> And again, the malicious application can exec the setuid binary before
> exit, in this case we never reset ->exit_signal (of course, unless
> that binary drops CAP_KILL).

Heh, thanks for taking the time to set me straight.

Acked-by: Serge Hallyn <serue@us.ibm.com>

Thanks.

-serge

[PATCH, RESEND] exit_notify: kill the wrong capable(CAP_KILL) check

[Oleg Nesterov]

The CAP_KILL check in exit_notify() looks just wrong, kill it.

Whatever logic we have to reset ->exit_signal, the malicious user
can bypass it if it execs the setuid application before exiting.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>

--- 6.30/kernel/exit.c~EXIT_CAP_KILL	2009-04-06 00:03:42.000000000 +0200
+++ 6.30/kernel/exit.c	2009-04-06 15:30:32.000000000 +0200
@@ -837,8 +837,7 @@ static void exit_notify(struct task_stru
 	 */
 	if (tsk->exit_signal != SIGCHLD && !task_detached(tsk) &&
 	    (tsk->parent_exec_id != tsk->real_parent->self_exec_id ||
-	     tsk->self_exec_id != tsk->parent_exec_id) &&
-	    !capable(CAP_KILL))
+	     tsk->self_exec_id != tsk->parent_exec_id))
 		tsk->exit_signal = SIGCHLD;
 
 	signal = tracehook_notify_death(tsk, &cookie, group_dead);

Re: [PATCH, RESEND] exit_notify: kill the wrong capable(CAP_KILL) check

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>