2.4+ptrace exploit fix breaks root's ability to strace

2.4+ptrace exploit fix breaks root's ability to strace

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Alan Cox]

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.

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]


> --- 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Arjan van de Ven]

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. 

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Alan Cox]

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

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Lists (lst)]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1066 bytes --]

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: mlafon@arkoon.net
To: linux-kernel@vger.kernel.org
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

[-- Attachment #2: Type: TEXT/PLAIN, Size: 410 bytes --]

diff -u -r1.3.24.1 ptrace.c
--- linux-2.4/kernel/ptrace.c	2003/03/19 10:50:57	1.3.24.1
+++ linux-2.4/kernel/ptrace.c	2003/03/19 10:54:45
@@ -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)) && (current->uid != 0))
 		mm = NULL;
 	if (mm)
 		atomic_inc(&mm->mm_users);

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Russell King]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Martin Loschwitz]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

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 (rmk@arm.linux.org.uk)                The developer of ARM Linux
>              http://www.arm.linux.org.uk/personal/aboutme.html
> 

-- 
  .''`.   Martin Loschwitz           Debian GNU/Linux developer
 : :'  :  madkiss@madkiss.org        madkiss@debian.org
 `. `'`   http://www.madkiss.org/    people.debian.org/~madkiss/
   `-     Use Debian GNU/Linux 3.0!  See http://www.debian.org/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Arjan van de Ven]

[-- Attachment #1: Type: text/plain, Size: 514 bytes --]

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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Yusuf Wilajati Purna]

Hi,

On 2003-03-22 17:28:54, Arjan van de Ven wrote:
>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.

So, do you mean that the following is enough:

int ptrace_check_attach(struct task_struct *child, int kill)
{
      ...
+       if (!child->task_dumpable)
+               return -EPERM;
}

Regards,

Purna
         		

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Bernhard Kaindl]

On Thu, 17 Apr 2003, Yusuf Wilajati Purna wrote:
> On 2003-03-22 17:28:54, Arjan van de Ven wrote:
> >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.
>
> So, do you mean that the following is enough:
>
> int ptrace_check_attach(struct task_struct *child, int kill)
> {
>       ...
> +       if (!child->task_dumpable)
> +               return -EPERM;
> }

It's enough to still be safe against the ptrace/kmod exploits.

I could not find a security problem in it yet because
compute_cred() ignores the suid bit on exec when the
process is being traced, so the strace does not get
access to privileges from somebody else and ptrace_attach
uses is_dumpable() which also checks task->mm->dumpable
so a tracer can't attach to a suid program.

It will also help the case Russell King describes above
where root failed to trace a daemon which changed uids
or a suid program, AFAICS.

It is not the complete fix for it because the ptrace functions
also use access_process_vm() where the patch had this hunk:

@@ -123,6 +127,8 @@ int access_process_vm(struct task_struct
        /* Worry about races with exit() */
        task_lock(tsk);
        mm = tsk->mm;
+       if (!is_dumpable(tsk) || (&init_mm == mm))
+               mm = NULL;
        if (mm)
                atomic_inc(&mm->mm_users);
        task_unlock(tsk);

You need to backout the tsk->mm->dumpable check done within is_dumpable
here by just checking task_dumpable and then ptracing from root works
prperly again.

As the kmod ptrace fix relies on task_dumpable for it's protection against
kernel thread trace, and you just remove the tsk->mm->dumpable check by
replacing !is_dumpable(tsk) with !tsk->task_dumpable here also, you don't
affect the kmod ptrace exploit protection in any way while fixing the
ability of root to trace any task.

This also fixes the problem /proc/<pid>/cmdline being empty (also for root)
if <pid> is not dumpable, which is the other bug introduced by this hunk
and broke process managment tools as it was also read on l-k.

Of course now people may say that this opens a security hole because a
normal user ist now able to read /proc/*/cmdline again (also for not
dumpable processes) but I'm answering to this that it has been well
known that it is insecure to put secrets onto the command line space
and e.g. if programs don't clear it properly, they are buggy and should
be audited to fix the bugs and not add a workaound to the kernel for
such programs.

Bernd

Re: 2.4+ptrace exploit fix breaks root's ability to strace

[Yusuf Wilajati Purna]

Hi,

Thanks for the clarification. :-)

Bernhard Kaindl wrote:

>On Thu, 17 Apr 2003, Yusuf Wilajati Purna wrote:
>
>>On 2003-03-22 17:28:54, Arjan van de Ven wrote:
>>
>>>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.
>>>
>>So, do you mean that the following is enough:
>>
>>int ptrace_check_attach(struct task_struct *child, int kill)
>>{
>>      ...
>>+       if (!child->task_dumpable)
>>+               return -EPERM;
>>}
>>
>
>It's enough to still be safe against the ptrace/kmod exploits.
>
>I could not find a security problem in it yet because
>compute_cred() ignores the suid bit on exec when the
>process is being traced, so the strace does not get
>access to privileges from somebody else and ptrace_attach
>uses is_dumpable() which also checks task->mm->dumpable
>so a tracer can't attach to a suid program.
>
>It will also help the case Russell King describes above
>where root failed to trace a daemon which changed uids
>or a suid program, AFAICS.
>
>It is not the complete fix for it because the ptrace functions
>also use access_process_vm() where the patch had this hunk:
>
>@@ -123,6 +127,8 @@ int access_process_vm(struct task_struct
>        /* Worry about races with exit() */
>        task_lock(tsk);
>        mm = tsk->mm;
>+       if (!is_dumpable(tsk) || (&init_mm == mm))
>+               mm = NULL;
>        if (mm)
>                atomic_inc(&mm->mm_users);
>        task_unlock(tsk);
>
>You need to backout the tsk->mm->dumpable check done within is_dumpable
>here by just checking task_dumpable and then ptracing from root works
>prperly again.
>
>As the kmod ptrace fix relies on task_dumpable for it's protection against
>kernel thread trace, and you just remove the tsk->mm->dumpable check by
>replacing !is_dumpable(tsk) with !tsk->task_dumpable here also, you don't
>affect the kmod ptrace exploit protection in any way while fixing the
>ability of root to trace any task.
>
>This also fixes the problem /proc/<pid>/cmdline being empty (also for root)
>if <pid> is not dumpable, which is the other bug introduced by this hunk
>and broke process managment tools as it was also read on l-k.
>
Just to recapitulate,
The following changes to the original patch (Alan's patch):

   int ptrace_check_attach(struct task_struct *child, int kill)
   {
           ...
   +      if (!child->task_dumpable)
   +      return -EPERM;
   }

   int access_process_vm(struct task_struct *tsk, unsigned long addr, 
void *buf, int len, int write)
   {
           ...
           /* Worry about races with exit() */
           task_lock(tsk);
           mm = tsk->mm;
   +      if (!tsk->task_dumpable || (&init_mm == mm))
   +                mm = NULL;
           ...
   }

can solve the following side-effects introduced by the original patch:

- /proc/PID/cmdline and /proc/PID/environ are empty for non-dumpable 
process es
  even for root. (ps displays those processes in [] brackets.)
  http://marc.theaimsgroup.com/?l=linux-kernel&m=104807368719299&w=2

- strace started by root cannot ptrace user threads or such non-dumpable 
processes.
  http://marc.theaimsgroup.com/?l=linux-kernel&m=104835339619706&w=2

At least, I have confirmed this on an i386/IA-32 platform. And I have 
checked also
that ptrace/kmod exploits such as isec-ptrace-kmod-exploit.c, ptrace.c, 
km3.c cannot
get root privilege with the changes.

Any comments?

Thanks,
Purna