2.4+ptrace exploit fix breaks root's ability to strace
Yusuf Wilajati Purna
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Bernhard Kaindl
> 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
Yusuf Wilajati Purna
Thanks for the clarification. :-)
Bernhard Kaindl wrote:
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
Bernhard Kaindl
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
Nuno Silva
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
> ------------------------------------------------------------------------
>
> --- 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);
-
Arjan van de Ven
> 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.
Bernhard Kaindl
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
Andreas Gietl
> 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)
i can confirm that shutdown (halt|reboot) does not work on my 2.4.21-rc1-ac1
boxes. (gentoo + redhat).
But your patch does not seem to fix it.
--
e-admin internet gmbh
Andreas Gietl tel +49 941 3810884
Ludwig-Thoma-Strasse 35 fax +49 89 244329104
93051 Regensburg mobil +49 171 6070008
PGP/GPG-Key unter http://www.e-admin.de/gpg.html
Bernhard Kaindl
> > system monitoring stuff this way(I've even heard shutdown is affected)
>
> i can confirm that shutdown (halt|reboot) does not work on my 2.4.21-rc1-ac1
> boxes. (gentoo + redhat).
Thanks for the info!
> But your patch does not seem to fix it.
Very interesting also, the two liner adressed only the well-known problems.
To fix the other not so well-known side effects, a real cleanup is the way
to go.
Can you try the attached cleanup patch instead of the two-liner?
It's an inital cleanup and should fix the other side effects which
I described in my mails.
Bernhard Kaindl
PS: If either patch is applied correctly, this
su guest -c 'ps $PPID;wc -m </proc/$PPID/cmdline'
should give:
PID TTY STAT TIME COMMAND
2452 pts/2 S 0:00 su bin -c ps $PPID;wc -m </proc/$PPID/cmdline
46
If it does not, access_process_vm is not fixed properly.
Second, calling this as root:
strace -fewrite su -c /bin/echo 2>&1 | grep pid
should give:
[pid 2599] --- SIGSTOP (Stopped (signal)) @ 0 (0) ---
[pid 2599] write(1, "\n", 1
If it does not, ptrace_check_attach is not fixed properly.
(These are only the checks for the well known side
effects which should be fixed even with the short
is_dumpable() -> task_dumpable patch.)
Andreas Gietl
> On Fri, 25 Apr 2003, Andreas Gietl wrote:
> > > system monitoring stuff this way(I've even heard shutdown is affected)
> >
> > i can confirm that shutdown (halt|reboot) does not work on my
> > 2.4.21-rc1-ac1 boxes. (gentoo + redhat).
>
> Thanks for the info!
>
> > But your patch does not seem to fix it.
>
> Very interesting also, the two liner adressed only the well-known problems.
> To fix the other not so well-known side effects, a real cleanup is the way
> to go.
>
> Can you try the attached cleanup patch instead of the two-liner?
>
> It's an inital cleanup and should fix the other side effects which
> I described in my mails.
>
> Bernhard Kaindl
>
> PS: If either patch is applied correctly, this
>
> su guest -c 'ps $PPID;wc -m </proc/$PPID/cmdline'
>
> should give:
>
> PID TTY STAT TIME COMMAND
> 2452 pts/2 S 0:00 su bin -c ps $PPID;wc -m </proc/$PPID/cmdline
> 46
it shows:
PID TTY STAT TIME COMMAND
2092 ttyp0 S 0:00 su guest -c ps $PPID;wc -m </proc/$PPID/cmdline
0
but cat shows:
su guest -c 'ps $PPID; cat /proc/$PPID/cmdline'
PID TTY STAT TIME COMMAND
2144 ttyp0 S 0:00 su guest -c ps $PPID; cat /proc/$PPID/cmdline
suguest-cps $PPID; cat /proc/$PPID/cmdline
what happened?
>
> If it does not, access_process_vm is not fixed properly.
>
> Second, calling this as root:
>
> strace -fewrite su -c /bin/echo 2>&1 | grep pid
>
> should give:
>
> [pid 2599] --- SIGSTOP (Stopped (signal)) @ 0 (0) ---
> [pid 2599] write(1, "\n", 1
it shows:
localhost root # strace -fewrite su -c /bin/echo 2>&1 | grep pid
[pid 2159] write(1, "\n", 1
>
> If it does not, ptrace_check_attach is not fixed properly.
>
> (These are only the checks for the well known side
> effects which should be fixed even with the short
> is_dumpable() -> task_dumpable patch.)
Looks like my results are slightly diffent, Does this mean i did not apply the
patch correctly? I applied it twice manually, because patch did not succeed
and compiled the kernel 3 times...
Bernhard Kaindl
>
> it shows:
> PID TTY STAT TIME COMMAND
> 2092 ttyp0 S 0:00 su guest -c ps $PPID;wc -m </proc/$PPID/cmdline
> 0
Ah ok, I tested this with LC_CTYPE=de_DE.UTF-8, in this case wc has
to read the contents of the file. When a single-byte locale is used,
wc just does fstat64() to get the file size(which returns 0 for the file).
> but cat shows:
> su guest -c 'ps $PPID; cat /proc/$PPID/cmdline'
> PID TTY STAT TIME COMMAND
> 2144 ttyp0 S 0:00 su guest -c ps $PPID; cat /proc/$PPID/cmdline
> suguest-cps $PPID; cat /proc/$PPID/cmdline
>
> what happened?
This is ok, access_proces_vm() is working.
> > [pid 2599] --- SIGSTOP (Stopped (signal)) @ 0 (0) ---
> > [pid 2599] write(1, "\n", 1
>
> it shows:
> localhost root # strace -fewrite su -c /bin/echo 2>&1 | grep pid
> [pid 2159] write(1, "\n", 1
Looks ok also, the task->mm->dumpable check is removed from prace_check_attach
> Looks like my results are slightly diffent, Does this mean i did not apply the
> patch correctly? I applied it twice manually, because patch did not succeed
The above shows that at least the two-liner task_dumpable diff is applied.
> and compiled the kernel 3 times...
Sorry for the different output, I should have made my tests safer against
system variants. It should have applied clean, maybe some white space issue
or so, I'll check it. shutdown didn't change? What does it on your system?
Bernd
Andreas Gietl
looks like the shutdown problem was not due to a ptrace-problem, but to a
IDE-Bug in the ac1-patch:
Hi!
Here is a first test report for 2.4.21-rc1-ac2:
*) Shutdown/Reboot problems in 2.4.21-rc1-ac1 due to
deadlock in ide_unregister_subdriver() seem to be
fixed!
rc1-ac2 fixed my shutdown problem.
thanx for the support
--
e-admin internet gmbh
Andreas Gietl tel +49 941 3810884
Ludwig-Thoma-Strasse 35 fax +49 89 244329104
93051 Regensburg mobil +49 171 6070008
PGP/GPG-Key unter http://www.e-admin.de/gpg.html