* Re: 2.4+ptrace exploit fix breaks root's ability to strace
@ 2003-04-17 5:46 Yusuf Wilajati Purna
2003-04-19 5:57 ` Bernhard Kaindl
0 siblings, 1 reply; 19+ messages in thread
From: Yusuf Wilajati Purna @ 2003-04-17 5:46 UTC (permalink / raw)
To: linux-kernel, rmk, arjanv, alan; +Cc: 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-04-17 5:46 2.4+ptrace exploit fix breaks root's ability to strace Yusuf Wilajati Purna @ 2003-04-19 5:57 ` Bernhard Kaindl 2003-04-22 5:03 ` Yusuf Wilajati Purna 0 siblings, 1 reply; 19+ messages in thread From: Bernhard Kaindl @ 2003-04-19 5:57 UTC (permalink / raw) To: rmk, Yusuf Wilajati Purna; +Cc: linux-kernel, Bernhard Kaindl, arjanv 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-04-19 5:57 ` Bernhard Kaindl @ 2003-04-22 5:03 ` Yusuf Wilajati Purna 2003-04-22 22:30 ` [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix Bernhard Kaindl 0 siblings, 1 reply; 19+ messages in thread From: Yusuf Wilajati Purna @ 2003-04-22 5:03 UTC (permalink / raw) To: Bernhard Kaindl, rmk, linux-kernel, Bernhard Kaindl, arjanv; +Cc: 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix 2003-04-22 5:03 ` Yusuf Wilajati Purna @ 2003-04-22 22:30 ` Bernhard Kaindl 2003-04-24 5:40 ` Nuno Silva 0 siblings, 1 reply; 19+ messages in thread From: Bernhard Kaindl @ 2003-04-22 22:30 UTC (permalink / raw) To: Yusuf Wilajati Purna, Marcelo Tosatti Cc: rmk, linux-kernel, arjanv, Bernhard Kaindl [-- Attachment #1: Type: TEXT/PLAIN, Size: 6604 bytes --] Hello Marcelo, Hello Yusuf! I've attached a patch which fixes the remaining side effects which the ptrace fix posted by Alan introduced(which affect production systems) and I'm sending this because I think 2.4.20-rc1 should not be released as 2.4.21 without these problems fixed. On Tue, 22 Apr 2003, Yusuf Wilajati Purna wrote: > > Thanks for the clarification. :-) Sorry if my descriptions in my previos mail did not have any word too much(really short) but I tried to make the point straight for people which know the code. I'm adding a little bit more verbosity now :-) The check added by Alan's patch to ptrace_check_attach was: + if (!is_dumpable(child)) + return -EPERM; New, replacement check in ptrace_check_attach: + if (!child->task_dumpable) + return -EPERM; I want to explain now, why the above use of is_dumpable() broke ptrace of setuid programs by root: is_dumpable() checks if both, task_dumpable and mm->dumpable are set, and evaluates to false, if one of them is false. The new kernel_thread() function added by Alan's patch sets task_dumpable (which is 1 by default) for the new kernel thread to 0, and this is the only place where his new variable is set to 0, so "non_kernel_thread" would accurately describe what it is saying. By adding is_dumpable(child) to ptrace_check_attach(), the patch posted by Alan, not only a check if the task is a kernel thread, but also a check if the task changed it's uid's was added(what mm->dumpable says) so even root was blocked out by this check. So, removing the wrong check to child->mm->dumpable and only checking child->task_dumpable (wnich really means "non_kernel_thread") is the first part of the fix. The other place which needed to be touched to fix Alan's patch was access_process_vm(), where Alan's patch did this change: > >@@ -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); access_process_vm() is in the same code patch as ptrace_check_attach. If you read the sys_ptrace implementation in arch/i386/kernel/ptrace.c, you'll find a call to ptrace_check_attach() and then, shortly afterwards, depending on what ptrace action was requested, a call to access_process_vm() So the !is_dumpable(tsk) check above it just a repetition if the previous check which you can also replace with !tsk->task_dumpable which you correctly understood and you show below in your change: > 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; Note, in addtion to breaking root's ability to trace setuid programs, having the tsk->mm->dumpable checked by !is_dumpable(tsk) at this place also broke /proc/PID/cmdline and /proc/PID/environ because access_process_vm() is also used by these proc functions. If somebody says this opens a securtiy leak, I'd have to say: If a suid task leaks such information thru it's cmdline buffer, it's the problem of the suid process not acting secure and should be reviewed. You would need to restrict cmdline access to all root processes(not only suid) and maybe even to all processes with different capabilites and uid/gid to work around problems in such processes. But you would break even more system monitoring stuff this way(I've even heard shutdown is affected) > ... > } > > 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 Yes exacly, they do fix these side-effects, as your test correctly gives: > 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. Exactly, the change (effectve removal of the task->mm->dumpable flag check, which is not part of the kernel_thread/ptrace issue, fix the the two side effects you describe above while maintaining same security against the kmod/ptrace exploits because it only removes code that has nothing to do with the kernel_thread/ptrace issue, which was added by Alan's patch and introduced these side effects. It's really that easy, you just have to look and the code and see it ;-) Ok, you need to understand how the ptrace code works and how Alan's patch effectively blocks all possible trace attempts and backdoors, but once you understood how it works, it's easy to identify the parts of Alan's patch which cause these side effects. It's just cleanup, nothing more, nothing very creative, but a nice opportunity to learn a little bit about the kernel, no deep knowledge about VM or something really complex is needed. <tiny font> If you look sharper, you can even start cleaning up more of code added by the patch Alan sent (the above checks are completely unneccesary ;-) but you need the big picture for this and I have to give you this big picture in a separate mail to make this point. </tiny font> > Any comments? I'm sorry that I did not send a patch the first time to make the change 100% clear to anybody, but I'm doing this now. Incremental patch which applies on top of the patch posted by Alan and also on top of 2.4.21-rc1 is attached now. With only this patch applied I'd think 2.4.21 could be released, but not without this minimum fix. 2.4.21-rc1, if it would be released as is, has the potential of breaking lots of systems which rely on not seeing the side effects Yusuf Wilajati Purna describes above and are fixed by this incremental fix. Best Regards, Bernhard Kaindl SuSE Linux [-- Attachment #2: Type: TEXT/PLAIN, Size: 546 bytes --] --- kernel/ptrace.c 2003/04/22 21:14:20 1.1 +++ kernel/ptrace.c 2003/04/22 21:15:40 @@ -22,7 +22,7 @@ int ptrace_check_attach(struct task_struct *child, int kill) { mb(); - if (!is_dumpable(child)) + if (!child->task_dumpable) return -EPERM; if (!(child->ptrace & PT_PTRACED)) @@ -127,7 +127,7 @@ /* Worry about races with exit() */ task_lock(tsk); mm = tsk->mm; - if (!is_dumpable(tsk) || (&init_mm == mm)) + if (!tsk->task_dumpable || (&init_mm == mm)) mm = NULL; if (mm) atomic_inc(&mm->mm_users); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix 2003-04-22 22:30 ` [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix Bernhard Kaindl @ 2003-04-24 5:40 ` Nuno Silva 2003-04-24 9:00 ` Arjan van de Ven 0 siblings, 1 reply; 19+ messages in thread From: Nuno Silva @ 2003-04-24 5:40 UTC (permalink / raw) To: Bernhard Kaindl Cc: Yusuf Wilajati Purna, Marcelo Tosatti, rmk, linux-kernel, arjanv, Bernhard Kaindl Good morning! :) I'd like to ear an "official" word on this subject, please. :) Is this patch still secure? I don't like having a known root hole in my servers but i'd like to have the same functionallity as before. Right now it seams that I can't have both :( Alan? Marcelo? Linus? HELPPPPPPPPP! :) Regards, Nuno Silva Bernhard Kaindl wrote: > Hello Marcelo, Hello Yusuf! > > I've attached a patch which fixes the remaining side effects > which the ptrace fix posted by Alan introduced(which affect production > systems) and I'm sending this because I think 2.4.20-rc1 should > not be released as 2.4.21 without these problems fixed. > > On Tue, 22 Apr 2003, Yusuf Wilajati Purna wrote: > >>Thanks for the clarification. :-) > > > Sorry if my descriptions in my previos mail did not have any word too > much(really short) but I tried to make the point straight for people > which know the code. I'm adding a little bit more verbosity now :-) > > The check added by Alan's patch to ptrace_check_attach was: > > + if (!is_dumpable(child)) > + return -EPERM; > > New, replacement check in ptrace_check_attach: > > + if (!child->task_dumpable) > + return -EPERM; > > I want to explain now, why the above use of is_dumpable() broke ptrace > of setuid programs by root: > > is_dumpable() checks if both, task_dumpable and mm->dumpable are set, and > evaluates to false, if one of them is false. > > The new kernel_thread() function added by Alan's patch sets task_dumpable > (which is 1 by default) for the new kernel thread to 0, and this is the > only place where his new variable is set to 0, so "non_kernel_thread" > would accurately describe what it is saying. > > By adding is_dumpable(child) to ptrace_check_attach(), the patch posted > by Alan, not only a check if the task is a kernel thread, but also a > check if the task changed it's uid's was added(what mm->dumpable says) > so even root was blocked out by this check. > > So, removing the wrong check to child->mm->dumpable and only checking > child->task_dumpable (wnich really means "non_kernel_thread") is the > first part of the fix. > > The other place which needed to be touched to fix Alan's patch was > access_process_vm(), where Alan's patch did this change: > > >>>@@ -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); > > > access_process_vm() is in the same code patch as ptrace_check_attach. > > If you read the sys_ptrace implementation in arch/i386/kernel/ptrace.c, > you'll find a call to ptrace_check_attach() and then, shortly afterwards, > depending on what ptrace action was requested, a call to access_process_vm() > > So the !is_dumpable(tsk) check above it just a repetition if the previous > check which you can also replace with !tsk->task_dumpable which you correctly > understood and you show below in your change: > > >>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; > > > Note, in addtion to breaking root's ability to trace setuid programs, > having the tsk->mm->dumpable checked by !is_dumpable(tsk) at this place > also broke /proc/PID/cmdline and /proc/PID/environ because access_process_vm() > is also used by these proc functions. > > If somebody says this opens a securtiy leak, I'd have to say: > > If a suid task leaks such information thru it's cmdline buffer, it's > the problem of the suid process not acting secure and should be reviewed. > > You would need to restrict cmdline access to all root processes(not only > suid) and maybe even to all processes with different capabilites and uid/gid > to work around problems in such processes. But you would break even more > system monitoring stuff this way(I've even heard shutdown is affected) > > >> ... >> } >> >>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 > > > Yes exacly, they do fix these side-effects, as your test correctly gives: > > >>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. > > > Exactly, the change (effectve removal of the task->mm->dumpable flag check, > which is not part of the kernel_thread/ptrace issue, fix the the two side > effects you describe above while maintaining same security against the > kmod/ptrace exploits because it only removes code that has nothing to > do with the kernel_thread/ptrace issue, which was added by Alan's patch > and introduced these side effects. > > It's really that easy, you just have to look and the code and see it ;-) > > Ok, you need to understand how the ptrace code works and how Alan's > patch effectively blocks all possible trace attempts and backdoors, > but once you understood how it works, it's easy to identify the parts > of Alan's patch which cause these side effects. > > It's just cleanup, nothing more, nothing very creative, but a nice > opportunity to learn a little bit about the kernel, no deep knowledge > about VM or something really complex is needed. > > <tiny font> > If you look sharper, you can even start cleaning up more of code added > by the patch Alan sent (the above checks are completely unneccesary ;-) > but you need the big picture for this and I have to give you this big > picture in a separate mail to make this point. > </tiny font> > >>Any comments? > > > I'm sorry that I did not send a patch the first time to make the > change 100% clear to anybody, but I'm doing this now. > > Incremental patch which applies on top of the patch posted by > Alan and also on top of 2.4.21-rc1 is attached now. > > With only this patch applied I'd think 2.4.21 could be released, > but not without this minimum fix. > > 2.4.21-rc1, if it would be released as is, has the potential of > breaking lots of systems which rely on not seeing the side effects > Yusuf Wilajati Purna describes above and are fixed by this > incremental fix. > > Best Regards, > Bernhard Kaindl > SuSE Linux > > > ------------------------------------------------------------------------ > > --- kernel/ptrace.c 2003/04/22 21:14:20 1.1 > +++ kernel/ptrace.c 2003/04/22 21:15:40 > @@ -22,7 +22,7 @@ > int ptrace_check_attach(struct task_struct *child, int kill) > { > mb(); > - if (!is_dumpable(child)) > + if (!child->task_dumpable) > return -EPERM; > > if (!(child->ptrace & PT_PTRACED)) > @@ -127,7 +127,7 @@ > /* Worry about races with exit() */ > task_lock(tsk); > mm = tsk->mm; > - if (!is_dumpable(tsk) || (&init_mm == mm)) > + if (!tsk->task_dumpable || (&init_mm == mm)) > mm = NULL; > if (mm) > atomic_inc(&mm->mm_users); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix 2003-04-24 5:40 ` Nuno Silva @ 2003-04-24 9:00 ` Arjan van de Ven 2003-04-24 11:26 ` Bernhard Kaindl 0 siblings, 1 reply; 19+ messages in thread From: Arjan van de Ven @ 2003-04-24 9:00 UTC (permalink / raw) To: Nuno Silva Cc: Bernhard Kaindl, Yusuf Wilajati Purna, Marcelo Tosatti, rmk, linux-kernel, arjanv, Bernhard Kaindl On Thu, Apr 24, 2003 at 06:40:55AM +0100, Nuno Silva wrote: > Good morning! :) > > I'd like to ear an "official" word on this subject, please. :) > Is this patch still secure? The check is loosend too much. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix 2003-04-24 9:00 ` Arjan van de Ven @ 2003-04-24 11:26 ` Bernhard Kaindl 0 siblings, 0 replies; 19+ messages in thread From: Bernhard Kaindl @ 2003-04-24 11:26 UTC (permalink / raw) To: Arjan van de Ven Cc: Nuno Silva, Yusuf Wilajati Purna, Marcelo Tosatti, rmk, linux-kernel Hi Arjan! On Thu, 24 Apr 2003, Arjan van de Ven wrote: > On Thu, Apr 24, 2003 at 06:40:55AM +0100, Nuno Silva wrote: > > Good morning! :) > > > > I'd like to ear an "official" word on this subject, please. :) > > Is this patch still secure? > > The check is loosend too much. Last month, you sounded different: 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. Can you give me an explanation that changed with regard to the kmod/ptrace fix? Im private discussuion this possible scenario has been brought up: > execed setuid > opens RAW_SOCKET > setuid back > > ptrace_attach() > ... AFAICS, ptrace_attach() will abort, because the "setuid back" does not set mm->dumpable back to 1, it is left at 0 and ptrace_attach() aborts then. Of course you could do an exec() to get a new mm and then, you may get an mm->dumpable is set != 0. But especially in the case of an exec() I think the setuid program should not go back to the original uid because the user may send any signal it wants to the suid program then, read it's environment and do all other sorts of bad stuff e.g. if the program is stupid enough to not use a safe directory for temp files and such. If we really need to protect such (IMHO insecure) progam to be somewhat more secure, we could inherit task_dumpable over exec's, but I'm not sure what kind of side effects whis would have when using programs like su and login... I think by adding such workarounds which are unrelated to kmod, we would introduce more unwanted side effects insted of fixing what we alread have because of too strong checks. Best Regards, Bernhard Kaindl ^ permalink raw reply [flat|nested] 19+ messages in thread
* 2.4+ptrace exploit fix breaks root's ability to strace @ 2003-03-22 10:31 Russell King 2003-03-22 14:58 ` Alan Cox 0 siblings, 1 reply; 19+ messages in thread From: Russell King @ 2003-03-22 10:31 UTC (permalink / raw) To: Linux Kernel List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 10:31 2.4+ptrace exploit fix breaks root's ability to strace Russell King @ 2003-03-22 14:58 ` Alan Cox 2003-03-22 14:10 ` Russell King 2003-03-23 10:31 ` Lists (lst) 0 siblings, 2 replies; 19+ messages in thread From: Alan Cox @ 2003-03-22 14:58 UTC (permalink / raw) To: Russell King; +Cc: Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 14:58 ` Alan Cox @ 2003-03-22 14:10 ` Russell King 2003-03-22 15:28 ` Arjan van de Ven 2003-03-23 10:31 ` Lists (lst) 1 sibling, 1 reply; 19+ messages in thread From: Russell King @ 2003-03-22 14:10 UTC (permalink / raw) To: Alan Cox; +Cc: Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 14:10 ` Russell King @ 2003-03-22 15:28 ` Arjan van de Ven 2003-03-22 17:13 ` Russell King 0 siblings, 1 reply; 19+ messages in thread From: Arjan van de Ven @ 2003-03-22 15:28 UTC (permalink / raw) To: Russell King; +Cc: Alan Cox, Linux Kernel Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 15:28 ` Arjan van de Ven @ 2003-03-22 17:13 ` Russell King 2003-03-22 17:28 ` Arjan van de Ven 2003-03-22 19:09 ` Alan Cox 0 siblings, 2 replies; 19+ messages in thread From: Russell King @ 2003-03-22 17:13 UTC (permalink / raw) To: Arjan van de Ven; +Cc: Alan Cox, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 17:13 ` Russell King @ 2003-03-22 17:28 ` Arjan van de Ven 2003-03-22 19:09 ` Alan Cox 1 sibling, 0 replies; 19+ messages in thread From: Arjan van de Ven @ 2003-03-22 17:28 UTC (permalink / raw) To: Arjan van de Ven, Alan Cox, Linux Kernel Mailing List 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 17:13 ` Russell King 2003-03-22 17:28 ` Arjan van de Ven @ 2003-03-22 19:09 ` Alan Cox 2003-03-22 18:01 ` Russell King 1 sibling, 1 reply; 19+ messages in thread From: Alan Cox @ 2003-03-22 19:09 UTC (permalink / raw) To: Russell King; +Cc: Arjan van de Ven, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 19:09 ` Alan Cox @ 2003-03-22 18:01 ` Russell King 0 siblings, 0 replies; 19+ messages in thread From: Russell King @ 2003-03-22 18:01 UTC (permalink / raw) To: Alan Cox; +Cc: Arjan van de Ven, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-22 14:58 ` Alan Cox 2003-03-22 14:10 ` Russell King @ 2003-03-23 10:31 ` Lists (lst) 2003-03-23 10:38 ` Russell King 2003-03-23 10:43 ` Arjan van de Ven 1 sibling, 2 replies; 19+ messages in thread From: Lists (lst) @ 2003-03-23 10:31 UTC (permalink / raw) To: Alan Cox; +Cc: Russell King, Linux Kernel Mailing List [-- 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); ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-23 10:31 ` Lists (lst) @ 2003-03-23 10:38 ` Russell King 2003-03-23 11:11 ` Martin Loschwitz 2003-03-23 10:43 ` Arjan van de Ven 1 sibling, 1 reply; 19+ messages in thread From: Russell King @ 2003-03-23 10:38 UTC (permalink / raw) To: Lists (lst); +Cc: Alan Cox, Linux Kernel Mailing List 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-23 10:38 ` Russell King @ 2003-03-23 11:11 ` Martin Loschwitz 0 siblings, 0 replies; 19+ messages in thread From: Martin Loschwitz @ 2003-03-23 11:11 UTC (permalink / raw) To: Russell King; +Cc: Linux Kernel Mailing List, Alan Cox [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: 2.4+ptrace exploit fix breaks root's ability to strace 2003-03-23 10:31 ` Lists (lst) 2003-03-23 10:38 ` Russell King @ 2003-03-23 10:43 ` Arjan van de Ven 1 sibling, 0 replies; 19+ messages in thread From: Arjan van de Ven @ 2003-03-23 10:43 UTC (permalink / raw) To: Lists (lst); +Cc: Alan Cox, Russell King, Linux Kernel Mailing List [-- 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 --] ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2003-04-24 12:11 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2003-04-17 5:46 2.4+ptrace exploit fix breaks root's ability to strace Yusuf Wilajati Purna 2003-04-19 5:57 ` Bernhard Kaindl 2003-04-22 5:03 ` Yusuf Wilajati Purna 2003-04-22 22:30 ` [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix Bernhard Kaindl 2003-04-24 5:40 ` Nuno Silva 2003-04-24 9:00 ` Arjan van de Ven 2003-04-24 11:26 ` Bernhard Kaindl -- strict thread matches above, loose matches on Subject: below -- 2003-03-22 10:31 2.4+ptrace exploit fix breaks root's ability to strace Russell King 2003-03-22 14:58 ` Alan Cox 2003-03-22 14:10 ` Russell King 2003-03-22 15:28 ` Arjan van de Ven 2003-03-22 17:13 ` Russell King 2003-03-22 17:28 ` Arjan van de Ven 2003-03-22 19:09 ` Alan Cox 2003-03-22 18:01 ` Russell King 2003-03-23 10:31 ` Lists (lst) 2003-03-23 10:38 ` Russell King 2003-03-23 11:11 ` Martin Loschwitz 2003-03-23 10:43 ` Arjan van de Ven
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).