* [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
@ 2009-02-25 19:02 Oleg Nesterov
2009-02-25 19:41 ` Roland McGrath
2009-04-06 14:16 ` [PATCH, RESEND] " Oleg Nesterov
0 siblings, 2 replies; 11+ messages in thread
From: Oleg Nesterov @ 2009-02-25 19:02 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: Alan Cox, Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Roland McGrath, Tavis Ormandy,
Vitaly Mayatskikh, stable, linux-kernel
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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 19:02 [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check Oleg Nesterov
@ 2009-02-25 19:41 ` Roland McGrath
2009-02-25 21:53 ` Serge E. Hallyn
2009-04-06 14:16 ` [PATCH, RESEND] " Oleg Nesterov
1 sibling, 1 reply; 11+ messages in thread
From: Roland McGrath @ 2009-02-25 19:41 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Linus Torvalds, Alan Cox, Chris Evans,
David Howells, Don Howard, Eugene Teo, Michael Kerrisk,
Tavis Ormandy, Vitaly Mayatskikh, stable, linux-kernel
> 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 19:41 ` Roland McGrath
@ 2009-02-25 21:53 ` Serge E. Hallyn
2009-02-25 22:03 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Serge E. Hallyn @ 2009-02-25 21:53 UTC (permalink / raw)
To: Roland McGrath
Cc: Oleg Nesterov, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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);
}
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 21:53 ` Serge E. Hallyn
@ 2009-02-25 22:03 ` Oleg Nesterov
2009-02-25 22:14 ` Serge E. Hallyn
0 siblings, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2009-02-25 22:03 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 22:03 ` Oleg Nesterov
@ 2009-02-25 22:14 ` Serge E. Hallyn
2009-02-25 22:32 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Serge E. Hallyn @ 2009-02-25 22:14 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 22:14 ` Serge E. Hallyn
@ 2009-02-25 22:32 ` Oleg Nesterov
2009-02-25 22:47 ` Serge E. Hallyn
0 siblings, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2009-02-25 22:32 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 22:32 ` Oleg Nesterov
@ 2009-02-25 22:47 ` Serge E. Hallyn
2009-02-25 23:16 ` Oleg Nesterov
0 siblings, 1 reply; 11+ messages in thread
From: Serge E. Hallyn @ 2009-02-25 22:47 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 22:47 ` Serge E. Hallyn
@ 2009-02-25 23:16 ` Oleg Nesterov
2009-02-25 23:54 ` Serge E. Hallyn
0 siblings, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2009-02-25 23:16 UTC (permalink / raw)
To: Serge E. Hallyn
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 23:16 ` Oleg Nesterov
@ 2009-02-25 23:54 ` Serge E. Hallyn
0 siblings, 0 replies; 11+ messages in thread
From: Serge E. Hallyn @ 2009-02-25 23:54 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Roland McGrath, Andrew Morton, Linus Torvalds, Alan Cox,
Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Tavis Ormandy, Vitaly Mayatskikh, stable,
linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH, RESEND] exit_notify: kill the wrong capable(CAP_KILL) check
2009-02-25 19:02 [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check Oleg Nesterov
2009-02-25 19:41 ` Roland McGrath
@ 2009-04-06 14:16 ` Oleg Nesterov
2009-04-06 19:36 ` Roland McGrath
1 sibling, 1 reply; 11+ messages in thread
From: Oleg Nesterov @ 2009-04-06 14:16 UTC (permalink / raw)
To: Andrew Morton, Linus Torvalds
Cc: Alan Cox, Chris Evans, David Howells, Don Howard, Eugene Teo,
Michael Kerrisk, Roland McGrath, Tavis Ormandy,
Vitaly Mayatskikh, linux-kernel, Serge Hallyn
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);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH, RESEND] exit_notify: kill the wrong capable(CAP_KILL) check
2009-04-06 14:16 ` [PATCH, RESEND] " Oleg Nesterov
@ 2009-04-06 19:36 ` Roland McGrath
0 siblings, 0 replies; 11+ messages in thread
From: Roland McGrath @ 2009-04-06 19:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Andrew Morton, Linus Torvalds, Alan Cox, Chris Evans,
David Howells, Don Howard, Eugene Teo, Michael Kerrisk,
Tavis Ormandy, Vitaly Mayatskikh, linux-kernel, Serge Hallyn
Acked-by: Roland McGrath <roland@redhat.com>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2009-04-06 19:38 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-25 19:02 [PATCH 2/2] exit_notify: kill the wrong capable(CAP_KILL) check Oleg Nesterov
2009-02-25 19:41 ` Roland McGrath
2009-02-25 21:53 ` Serge E. Hallyn
2009-02-25 22:03 ` Oleg Nesterov
2009-02-25 22:14 ` Serge E. Hallyn
2009-02-25 22:32 ` Oleg Nesterov
2009-02-25 22:47 ` Serge E. Hallyn
2009-02-25 23:16 ` Oleg Nesterov
2009-02-25 23:54 ` Serge E. Hallyn
2009-04-06 14:16 ` [PATCH, RESEND] " Oleg Nesterov
2009-04-06 19:36 ` Roland McGrath
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).