Re: [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix

[Nuno Silva]

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