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.

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:

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:

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)

Yes exacly, they do fix these side-effects, as your test correctly gives:

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>

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

ptrace-minimum.dif < 1K Download