Hi,
Are the authors of the ptrace patch aware that, in addition to closing the
hole, the "fix" also prevents a ptrace-capable task (eg, strace started by
root) from ptracing user threads?
For example, you can't strace vsftpd processes started from xinetd.
Is this intended behaviour?
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sat, 2003-03-22 at 10:31, Russell King wrote:
> Hi,
>
> Are the authors of the ptrace patch aware that, in addition to closing the
> hole, the "fix" also prevents a ptrace-capable task (eg, strace started by
> root) from ptracing user threads?
>
> For example, you can't strace vsftpd processes started from xinetd.
>
> Is this intended behaviour?
Its an unintended side effect, nobody has sent a patch to fix it yet.
On Sat, Mar 22, 2003 at 02:58:51PM +0000, Alan Cox wrote:
> On Sat, 2003-03-22 at 10:31, Russell King wrote:
> > Are the authors of the ptrace patch aware that, in addition to closing the
> > hole, the "fix" also prevents a ptrace-capable task (eg, strace started by
> > root) from ptracing user threads?
> >
> > For example, you can't strace vsftpd processes started from xinetd.
> >
> > Is this intended behaviour?
>
> Its an unintended side effect, nobody has sent a patch to fix it yet.
How about this fix? PT_PTRACE_CAP is set when we attach to a process
and the process doing the attaching has the ptrace capability.
Note that this patch is against the 2.4.19 version of ptrace.c with the
2.4.20 ptrace patch applied.
--- orig/kernel/ptrace.c Wed Mar 19 15:54:45 2003
+++ linux/kernel/ptrace.c Sat Mar 22 10:14:01 2003
@@ -22,7 +22,7 @@
int ptrace_check_attach(struct task_struct *child, int kill)
{
mb();
- if (!is_dumpable(child))
+ if (!is_dumpable(child) && !(child->ptrace & PT_PTRACE_CAP))
return -EPERM;
if (!(child->ptrace & PT_PTRACED))
@@ -140,7 +140,7 @@
/* Worry about races with exit() */
task_lock(tsk);
mm = tsk->mm;
- if (!is_dumpable(tsk) || (&init_mm == mm))
+ if ((!is_dumpable(tsk) || (&init_mm == mm)) && !(tsk->ptrace & PT_PTRACE_CAP))
mm = NULL;
if (mm)
atomic_inc(&mm->mm_users);
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
> --- orig/kernel/ptrace.c Wed Mar 19 15:54:45 2003
> +++ linux/kernel/ptrace.c Sat Mar 22 10:14:01 2003
> @@ -22,7 +22,7 @@
> int ptrace_check_attach(struct task_struct *child, int kill)
> {
> mb();
> - if (!is_dumpable(child))
> + if (!is_dumpable(child) && !(child->ptrace & PT_PTRACE_CAP))
> return -EPERM;
>
> if (!(child->ptrace & PT_PTRACED))
this sounds really wrong; the child says it doesn't want to be ptraced
and now you allow it anyway. I think the problem is more that the child
isn't dumpable.... checking why
On Sat, Mar 22, 2003 at 04:28:05PM +0100, Arjan van de Ven wrote:
>
> > --- orig/kernel/ptrace.c Wed Mar 19 15:54:45 2003
> > +++ linux/kernel/ptrace.c Sat Mar 22 10:14:01 2003
> > @@ -22,7 +22,7 @@
> > int ptrace_check_attach(struct task_struct *child, int kill)
> > {
> > mb();
> > - if (!is_dumpable(child))
> > + if (!is_dumpable(child) && !(child->ptrace & PT_PTRACE_CAP))
> > return -EPERM;
> >
> > if (!(child->ptrace & PT_PTRACED))
>
> this sounds really wrong; the child says it doesn't want to be ptraced
> and now you allow it anyway. I think the problem is more that the child
> isn't dumpable.... checking why
ptrace has always explicitly allowed a process with the CAP_SYS_PTRACE
capability to ptrace a task which isn't dumpable. With the ptrace "fix"
in place, you can attach to a non-dumpable thread:
int ptrace_attach(struct task_struct *task)
{
...
- if (!task->mm->dumpable && !capable(CAP_SYS_PTRACE))
+ if (!is_dumpable(task) && !capable(CAP_SYS_PTRACE))
goto bad;
}
but you can't do anything with it (not even detach from it):
int ptrace_check_attach(struct task_struct *child, int kill)
{
...
+ if (!is_dumpable(child))
+ return -EPERM;
}
So, we went from being able to ptrace daemons as root, to being able to
attach daemons and then being unable to do anything with them, even if
you're root (or have the CAP_SYS_PTRACE capability). I think this
behaviour is getting on for being described as "insane" 8) and is
clearly wrong.
Note that processes become "undumpable" as soon as they starts playing
with its [GU]IDs (via setre[gu]id, set[gu]id, set_res[gu]id, setfs[ug]id.)
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sat, Mar 22, 2003 at 05:13:12PM +0000, Russell King wrote:
>
> int ptrace_check_attach(struct task_struct *child, int kill)
> {
> ...
> + if (!is_dumpable(child))
> + return -EPERM;
> }
>
> So, we went from being able to ptrace daemons as root, to being able to
> attach daemons and then being unable to do anything with them, even if
> you're root (or have the CAP_SYS_PTRACE capability). I think this
> behaviour is getting on for being described as "insane" 8) and is
> clearly wrong.
ok it seems this check is too strong. It *has* to check
child->task_dumpable and return -EPERM, but child->mm->dumpable is not
needed.
On Sat, 2003-03-22 at 17:13, Russell King wrote:
> ptrace has always explicitly allowed a process with the CAP_SYS_PTRACE
> capability to ptrace a task which isn't dumpable. With the ptrace "fix"
> in place, you can attach to a non-dumpable thread:
Note that this is a bug, and is now a fixed bug. The looser check you
can do requires you check
my_capabilities >= his capbilities
Otherwise you have priviledge escalation for CAP_SYS_PTRACE to
CAP_SYS_RAWIO trivially
On Sat, Mar 22, 2003 at 07:09:08PM +0000, Alan Cox wrote:
> On Sat, 2003-03-22 at 17:13, Russell King wrote:
> > ptrace has always explicitly allowed a process with the CAP_SYS_PTRACE
> > capability to ptrace a task which isn't dumpable. With the ptrace "fix"
> > in place, you can attach to a non-dumpable thread:
>
> Note that this is a bug, and is now a fixed bug. The looser check you
> can do requires you check
>
> my_capabilities >= his capbilities
>
> Otherwise you have priviledge escalation for CAP_SYS_PTRACE to
> CAP_SYS_RAWIO trivially
In which case we should not allow ptrace to even attach to the process.
Currently you can attach to such a process and stop it running, even if
you have lesser priviledges than the child.
Therefore, I wouldn't call this "fixed" in the existing ptrace patch.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sat, 22 Mar 2003, Alan Cox wrote:
> On Sat, 2003-03-22 at 10:31, Russell King wrote:
> > Are the authors of the ptrace patch aware that, in addition to closing the
> > hole, the "fix" also prevents a ptrace-capable task (eg, strace started by
> > root) from ptracing user threads?
>
> Its an unintended side effect, nobody has sent a patch to fix it yet.
Hi,
mlafon send a patch to the list:
--------------------------------------------------------------------
Date: Wed, 19 Mar 2003 12:28:02 +0100
From: [email protected]
To: [email protected]
Subject: Re: Ptrace hole / Linux 2.2.25
The patch breaks /proc/<pid>/cmdline and /proc/<pid>/environ for 'non
dumpable'
processes, even for root.
We need to access theses proc files for processes monitoring.
Included is a patch to restore this functionnality for root.
Any comments ?
(See attached file: cmdline_environ_fix.diff)
--------------------------------------------------------------------
Nobody responded to his e-mail. I attach the patch again. I will test
the patch tomorow.
Cosmin
On Sun, Mar 23, 2003 at 12:31:39PM +0200, Lists (lst) wrote:
> The patch breaks /proc/<pid>/cmdline and /proc/<pid>/environ for 'non
> dumpable' processes, even for root.
This fix is definitely wrong.
> - if (!is_dumpable(tsk) || (&init_mm == mm))
> + if ((!is_dumpable(tsk) || (&init_mm == mm)) && (current->uid != 0))
> mm = NULL;
Today, security is based around the capability system, not UID numbers.
--
Russell King ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
On Sun, 2003-03-23 at 11:31, Lists (lst) wrote:
> On Sat, 22 Mar 2003, Alan Cox wrote:
>
> > On Sat, 2003-03-22 at 10:31, Russell King wrote:
> > > Are the authors of the ptrace patch aware that, in addition to closing the
> > > hole, the "fix" also prevents a ptrace-capable task (eg, strace started by
> > > root) from ptracing user threads?
> >
> > Its an unintended side effect, nobody has sent a patch to fix it yet.
>
> Hi,
>
> mlafon send a patch to the list:
uid == 0 is the wrong test
On Sun, Mar 23, 2003 at 10:38:54AM +0000, Russell King wrote:
>
> Today, security is based around the capability system, not UID numbers.
>
So can you send a patch to make this work the right[tm] way? I need this
capability as well...
> --
> Russell King ([email protected]) The developer of ARM Linux
> http://www.arm.linux.org.uk/personal/aboutme.html
>
--
.''`. Martin Loschwitz Debian GNU/Linux developer
: :' : [email protected] [email protected]
`. `'` http://www.madkiss.org/ people.debian.org/~madkiss/
`- Use Debian GNU/Linux 3.0! See http://www.debian.org/