lkml.org 
[lkml]   [2003]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH][2.4+ptrace] fix side effects of the kmod/ptrace secfix
    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);

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-03-22 13:34    [W:0.046 / U:150.248 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site