Hello,
The attached patch cleans up the too restrictive checks which were
included in the original ptrace/kmod secfix posted by Alan Cox
and applies on top of a clean 2.4.20-rc1 source tree.
It is the result of my continued work on fixing the side effects
introduced by the too restrictive checks which were included and
and I'm sending it here for review.
One side effect introduced by the ptrace/kmod secfix remains unfixed
for now: It results in modules not being loaded if the tasks which
would normally cause them being loaded are traced.
No error code is returned to the traced process in this case and
an printk like this is triggered by the kernel:
request_module[net-pf-15]: waitpid(4363,...) failed, errno 512
You could trigger this if you debug e.g. FreeS/WAN commands.
Fixing this is not as trivial as fixing other side effects, but
should be possible also.
I'm posting this cleanup because it fixes most side effects and
I don't know how long it will take to fix this remaining issue,
but I've a plan in preparation which should finally fix this last
issue.
For understanding the rest of this mail, the reader should have knowledge
about how the ptrace implementation in the linux kernel works, I don't have
a complete documentation at hand, please just send it if you know one.
For now, the best thing is to read all code paths which start at sys_ptrace()
in arch/i386/kernel/process.c, with background info from ptrace(2).
List of issues addressed by this cleanup:
Issue a:
Description: root is prevented from tracing not dumpable processes
Symptom: root can't debug setuid applications
Problem: The checks added to ptrace_check_attach() and access_process_vm()
were too strict and at the wrong place.
Solution: Revert the checks which were added to ptrace_check_attach() and
access_process_vm().
Details: ptrace_check_attach() is called by sys_ptrace() to verify if
current has properly attached to child, no checks should be
added there.
The revert in to access_process_vm() is also needed. The dumpable
check were not neccesary because ptrace_check_attach has to be
always consulted before.
The init_mm check there was also not neccesary as ptrace_attach
already does the right check with task_dumpable and task->mm.
Issue b:
Description: /proc/<pid>/cmdline of not dumpable processes is empty
Symptom: Broken process monitoring,
e.g. running processes may be reported dead.
Problem: The change in access_process_vm() affected not only ptrace.
access_process_vm() is also used by procfs which is changes
the userland interface for process monitoring tools
Solution: Revert the check added to access_process_vm(), see description
of issue a) for details about access_process_vm()
Issue c:
Description: priviliged tracers can cause processes hanging in stopped state
Symptom: processes traced by root which call exec() for a setuid program
or which call setuid finctions to change uids are left in stopped
state, even ptrace detach is blocked.
Problem: The checks added to ptrace_check_attach() and access_process_vm()
were too strict and at the wrong place.
The check in ptrace_attach() together with the other code is
perfect.
Solution: same as a)
Issue d:
Description: tracing processes which call prctl(PR_SET_DUMPABLE, 0)
causes traced processes hanging in stopped state.
Symptom: traced processes hang in stopped state after they called
prctl(PR_SET_DUMPABLE, 0)
Problem: The checks added to ptrace_check_attach() and access_process_vm()
were too strict and at the wrong place.
Solution: same as a)
Issue e:
Description: processes which want to change their dumpable status have
prctl(PR_SET_DUMPABLE, 1) ignored.
Symptom: Impossible to create a core dump of processes which changed uids,
even if the process requests it by calling prctl(PR_SET_DUMPABLE,1)
Problem: The change to the PR_SET_DUMPABLE in sys_prctl was too strict.
Solution: Revert the change to the sys_prctl(PR_SET_DUMPABLE) case:
No change was neccesary to prevent a thread with same mm to change
task->mm->dumpable, because a kernel_thread() and ptrace_attach()
use task->task_dumpable to forbid ptrace for a kernel thread.
Patch with comments added:
(no comments in the attached version)
--- kernel/ptrace.c
+++ kernel/ptrace.c
@@ -21,9 +21,6 @@
*/
int ptrace_check_attach(struct task_struct *child, int kill)
{
- mb();
- if (!is_dumpable(child))
- return -EPERM;
if (!(child->ptrace & PT_PTRACED))
return -ESRCH;
Using is_dumpable() here is not neccesary because the checks done here are:
> if (!(child->ptrace & PT_PTRACED))
> return -ESRCH;
>
> if (child->p_pptr != current)
> return -ESRCH;
This means ptrace_check_attach() returns -ESRCH if "current" is not properly
attached as tracer to "child", which is the only check it is called for.
The only place where child->ptrace and child->p_pptr are set to the values
which pass this test, is ptrace_attach() which works as explained in the
ptrace fix description further down this mail.
In turn, ptrace_attach() does the is_dumpable check *before* setting
these fields.
And as kernel_thread() aborts if child->ptrace is set, a kernel
thread with ptrace flag set cannot be created and as this flag
is checked here, adding any other checks here hurts the implementation.
@@ -127,8 +124,6 @@ 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);
Similar case here for the is_dumpable(tsk) check. access_process_vm()
is only called if ptrace_check_attach() passed it's tests.
See text above for more detail.
The check if &init_mm == mm is also not neccesary because kernel_thread()
sets task_dumpable to 0 which indicates a kernel thread and causes
ptrace_attach() to abort and because ptrace_check_attach() must pass
it's attach checks, access_process_vm() cannot be reached then.
So was with ptrace_check_attach, adding any check here is superflous
and can be harmful, which the is_dumpable check here already proofed
to be.
--- kernel/sys.c 2003/04/25 06:23:15 1.1
+++ kernel/sys.c 2003/04/25 06:23:51
@@ -1252,8 +1252,7 @@ asmlinkage long sys_prctl(int option, un
error = -EINVAL;
break;
}
- if (is_dumpable(current))
- current->mm->dumpable = arg2;
+ current->mm->dumpable = arg2;
break;
case PR_SET_UNALIGN:
Adding is_dumpable(current) as guard against setting
current->mm->dumpable is not neccesary because mm->dumpable
is not checked in kmod creation, task_dumpable is used.
See the ptrace fix description for a detailed explanation further down
this mail.
Very Best Begards,
Bernhard Kaindl, SuSE Linux AG
PS:
Description of the last remaining side effect(already mentioned
the the beginning of the mail):
The current 2.4.20-rc1 code aborts the creation of the kernel thread
if it finds the task traced. This is a side effect of the patch which
may possibly be preventable by creating the new thread detached from
ptrace.
This could be done by disabling the ptrace status of the calling
task before calling arch_kernel_thread() and restoring the ptrace
status in the parent afterwards.
Other ways are also possible, but would either require assembler
changes to all architectures or adding a call to simial code to
a function which should be called from all kernel threads.
I think this can be also done in a later kernel release, because
it should not have the severity the other side effects have.
PPS:
Detailed description of the core of ptrace/kmod fix:
====================================================
You have to think SMP here, two processes can execute kernel code
at the same time so proper SMP-safe locking has to be ensured.
SMP is the reason why the task->task_dumpable flag is needed, which plays
a major role in the fix. It is 0 for all kernel threads created by the
new kernel_thread function, otherwise it's 1.
task->ptrace is the other important flag. It is used to indicate wether
task is being traced. It gets a bit set when a tracer attaches to the
task and if task->ptrace is 0, a tracer has to attach to the process
again to be able to trace.
The third important part is the task_lock, a spinlock within the
task_struct which holds the two important flags ptrace and task_dumpable.
It serves a tool to serialize and group the accesses to these flags
as if there would be only one CPU executing the code parts which are
surrounded by take and release this lock.
With this this power at hand, the kernel serializes the important code
section in ptrace_attach() with the corresponding code in kernel_thread():
Let me show you the relevant code of kernel_thread():
/* lock out any potential ptracer */
spin> task_lock(task);
check> if (task->ptrace) {
task_unlock(task);
exit> return -EPERM;
}
old_task_dumpable = task->task_dumpable;
flag> task->task_dumpable = 0;
unlock> task_unlock(task);
I've added tags here so you see:
- the take of the task lock: task_lock(task);
- the check of the task->ptrace flag: if (task->ptrace) {
if ptraced:
- the exit if task is being traced: return -EPERM;
if not:
- the set of the task_dumpable flag to 0: task->task_dumpable = 0;
- the release of the task lock: task_unlock(task);
And finally you see the unlock of the task's spinlock which blocked
other code, possibly running on antoher CPU, waiting in busy loop
until this task_unlock is executed if it was trying to accesss the
same task.
This is the corresponding code in ptrace_attach():
spin> task_lock(task);
if (task->pid <= 1)
goto bad;
if (task == current)
goto bad;
if (!task->mm)
goto bad;
if(((current->uid != task->euid) ||
(current->uid != task->suid) ||
(current->uid != task->uid) ||
(current->gid != task->egid) ||
(current->gid != task->sgid) ||
(!cap_issubset(task->cap_permitted, current->cap_permitted)) ||
(current->gid != task->gid)) && !capable(CAP_SYS_PTRACE))
goto bad;
rmb();
check> if (!is_dumpable(task) && !capable(CAP_SYS_PTRACE))
exit> goto bad;
/* the same process cannot be attached many times */
if (task->ptrace & PT_PTRACED)
goto bad;
/* Go */
flag> task->ptrace |= PT_PTRACED;
if (capable(CAP_SYS_PTRACE))
task->ptrace |= PT_PTRACE_CAP;
unlock> task_unlock(task);
I've added tags here so you see:
- the take of the task lock: task_lock(task);
- the check of task_dumpable: if (!is_dumpable(task)
- the abort on a kernel thread: goto bad;
- the set of the ptrace flag: task->ptrace |= PT_PTRACED;
- the release of the task lock: task_unlock(task);
As there is no code which manipulates task->task_dumpable or
task->ptrace in a way which would allow to fool these checks
the code aboves prevents tracing kernel threads.
If you know a way to fool these checks, please tell me and Marcelo.
EOF
On Iau, 2003-05-08 at 23:05, Bernhard Kaindl wrote:
> - mb();
> - if (!is_dumpable(child))
> - return -EPERM;
>
> if (!(child->ptrace & PT_PTRACED))
> return -ESRCH;
>
> Using is_dumpable() here is not neccesary because the checks done here are:
>
> > if (!(child->ptrace & PT_PTRACED))
> > return -ESRCH;
Except that current->mm->dumpable is not per task but per mm so you
might ptrace one thread and have another go setuid.
On Iau, 2003-05-08 at 22:31, Alan Cox wrote:
> > Using is_dumpable() here is not neccesary because the checks done here are:
> >
> > > if (!(child->ptrace & PT_PTRACED))
> > > return -ESRCH;
>
> Except that current->mm->dumpable is not per task but per mm so you
> might ptrace one thread and have another go setuid.
Thinking about this harder
A ptraced thread cannot go setuid since we don't permit the exec to do
it
A setuid thread marks the mm dumpable so no thread can be ptraced (since
all threads inheriting the mm inherit it from the exec)
So ignore my earlier message
On 8 May 2003, Alan Cox wrote:
> On Iau, 2003-05-08 at 23:05, Bernhard Kaindl wrote:
> > - mb();
> > - if (!is_dumpable(child))
> > - return -EPERM;
> >
> > if (!(child->ptrace & PT_PTRACED))
> > return -ESRCH;
> >
> > Using is_dumpable() here is not neccesary because the checks done here are:
> >
> > > if (!(child->ptrace & PT_PTRACED))
> > > return -ESRCH;
>
> Except that current->mm->dumpable is not per task but per mm so you
> might ptrace one thread and have another go setuid.
You might try, but as far as I know:
a) setuid requires execve() which decouples from the other thread
and also gives the new thread a newly allocated task->mm.
b) If the thread which calls execve() is being traced, execve ignores
setuid.
c) If the thread which calls execve() is being not traced, a tracer has
to attach first, otherwise
> > > if (!(child->ptrace & PT_PTRACED))
> > > return -ESRCH;
catches the agaist-the-API ptrace call and the only way to set
child->ptrace & PT_PTRACED
is to call ptrace_attach() which checks for matching uids/gids
beween tracer and the setuid task and requiring that the tracer
may not miss a capability which the setuid task has granted and
effectively denies ptrace access otherwise:
if(((current->uid != task->euid) ||
(current->uid != task->suid) ||
(current->uid != task->uid) ||
(current->gid != task->egid) ||
(current->gid != task->sgid) ||
(!cap_issubset(task->cap_permitted, current->cap_permitted)) ||
(current->gid != task->gid)) && !capable(CAP_SYS_PTRACE))
goto bad;
d) Even if the setuid program changes uids and capabilies back so that
you would pass the check above, you will fail in the next line here:
if (!is_dumpable(task) && !capable(CAP_SYS_PTRACE))
goto bad;
And you can't change task->mm->dumpable back to 1 because the new
task has got a new mm allocated to which you have no access.
So, unless you have CAP_SYS_PTRACE, you will fail to trace the setuid
program(CAP_SYS_PTRACE is documented to allow to trace setuid) unless
it does prctl(PR_SET_DUMPABLE, 1) and after giving all capabilies gained
away and setting uid and gids back. At this point you could attach to
it again, but by calling prctl(PR_SET_DUMPABLE, 1), the setudi program
decares that it does not have any sensitive data anymore because you
could also send him a signal to cause him core dumping and read the
core with emacs then.
Do you see any chance to gain anything this way or
do you mean a scenario which I did not describe here?
Thanks,
Bernd
PS:
Just in case if people are interested where execve() gets a new mm
for the new program:
execve() calls do_execve()
which calls the binfmt's file loader(e.g. load_elf_binary)
which calls flush_old_exec()
which calls exec_mmap()
which releases the old mm and allocates a
new task->mm for the new process:
fs/exec.c, exec_mmap():
old_mm = current->mm;
if (old_mm) {
rlimit_rss = old_mm->rlimit_rss;
if (atomic_read(&old_mm->mm_users) == 1) {
mm_release();
exit_aio(old_mm);
exit_mmap(old_mm);
return 0;
}
}
mm = mm_alloc();
[...]
current->mm = mm;
EOF
Alan wrote:
> Thinking about this harder
>
> A ptraced thread cannot go setuid since we don't permit the exec to do
> it
>
> A setuid thread marks the mm dumpable so no thread can be ptraced (since
> all threads inheriting the mm inherit it from the exec)
Agreed, this is the short form of what I tried to say.
> So ignore my earlier message
Thanks,
Bernd
On Fri, 9 May 2003, Bernhard Kaindl wrote:
>
> a) setuid requires execve() which decouples from the other thread
> and also gives the new thread a newly allocated task->mm.
>
> b) If the thread which calls execve() is being traced, execve ignores
> setuid.
>
> c) If the thread which calls execve() is being not traced, a tracer has
> to attach first, otherwise
...
> d)
...
On Fri, May 09, 2003 at 12:05:52AM +0200, Bernhard Kaindl wrote:
> Hello,
>
> The attached patch cleans up the too restrictive checks which were
> included in the original ptrace/kmod secfix posted by Alan Cox
> and applies on top of a clean 2.4.20-rc1 source tree.
But the ptrace hole is _NOT_ fixed... :(
adamm@polaris:~/test$ uname -r
2.4.21-rc2
\u@\h:\w\$ ls -ltr hehe
-rw------- 1 root root 17 May 10 15:44 hehe
\u@\h:\w\$ whoami
root
\u@\h:\w\$ cat hehe
I can see you!!
\u@\h:\w\$ rm hehh
\u@\h:\w\$ ls -ltr hehe
ls: hehe: No such file or directory
I'm attaching the exploit so someone can fix the bug properly.
I could get root even with the patched 2.4.20 so I don't think
that this is the fault of the your patch.
- Adam
On Sat, May 10, 2003 at 03:52:49PM -0500, Adam Majer wrote:
> On Fri, May 09, 2003 at 12:05:52AM +0200, Bernhard Kaindl wrote:
> > Hello,
> >
> > The attached patch cleans up the too restrictive checks which were
> > included in the original ptrace/kmod secfix posted by Alan Cox
> > and applies on top of a clean 2.4.20-rc1 source tree.
>
> But the ptrace hole is _NOT_ fixed... :(
This is the exploit which makes itself suid. Did you leave it suid
before retesting it?
> adamm@polaris:~/test$ uname -r
> 2.4.21-rc2
> \u@\h:\w\$ ls -ltr hehe
> -rw------- 1 root root 17 May 10 15:44 hehe
> \u@\h:\w\$ whoami
> root
> \u@\h:\w\$ cat hehe
> I can see you!!
>
> \u@\h:\w\$ rm hehh
> \u@\h:\w\$ ls -ltr hehe
> ls: hehe: No such file or directory
Huh?
--
Daniel Jacobowitz
MontaVista Software Debian GNU/Linux Developer
On Sat, May 10, 2003 at 05:11:54PM -0400, Daniel Jacobowitz wrote:
> On Sat, May 10, 2003 at 03:52:49PM -0500, Adam Majer wrote:
> > On Fri, May 09, 2003 at 12:05:52AM +0200, Bernhard Kaindl wrote:
> > > Hello,
> > >
> > > The attached patch cleans up the too restrictive checks which were
> > > included in the original ptrace/kmod secfix posted by Alan Cox
> > > and applies on top of a clean 2.4.20-rc1 source tree.
> >
> > But the ptrace hole is _NOT_ fixed... :(
>
> This is the exploit which makes itself suid. Did you leave it suid
> before retesting it?
RIGHT..!!! :) Opps. That's why it "worked"... Never mind. 2.4.20-rc2 is
fixed.
- Adam