[patch] Fix -EPERM returned by kernel_thead() if traced...

[patch] Fix -EPERM returned by kernel_thead() if traced...

[Bernhard Kaindl]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5463 bytes --]

Hi,
   as I wrote some days ago, there is another side effect
caused by the initial 2.4.21-pre ptrace/kmod secfix which
I believe is fixable:

> 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
> and a printk() like this is triggered by the kernel:
>
> kernel: request_module[nfsd]: fork failed, errno 1

I've produced this by attempting to run the knfsd start script with
strace(compiled knfsd as module which was not yet loaded)

Starting FreeS/WAN with strace is another example where you could
hit this if this is the first start of FreeS/WAN.

I guess my patch could also help other ptrace users like User Mode Linux
and I've heard Wine in case they need to trigger a module load while
the task doing the syscall which triggers the load is being traced.

Given the previous descriptions I gave about the ptrace/kmod fix,
changing this place (the creation of a new kernel thread) seems
to be quite risky, but if you really think hard, you can see the
point I'm following.

The current kernel_thread() in 2.4.21-rc2 kernel/fork.c looks like
this (any locking removed to make it as simple as possible):

        if (task->ptrace)
                return -EPERM;

        task->task_dumpable = 0;

        ret = arch_kernel_thread(fn, arg, flags);

Ff the task is traced -EPERM is returned, otherwise task_dumpable = 0
is set, to forbid ptrace_attach() to attach to the thread.

Two scenarios:

a) task traced here -> -EPERM
b) task not traced here, but ptrace_attach() to new task:
   -> -EPERM because ptrace_attach() checks task_dumpable which is 0.

so it's safe, and with poper SMP locking (omitted above, but in the real
code) it's also SMP safe.

I've done very hard thinking on how it's possible to fix this and it's
quite suprising how easy it was:

It's again just removal of code! :-) (Yes, funny)

If you read all the surrounding code, you see that kernel_thread() is
being called from various other places to create a new thread which
may become privileged. Now it's neccesary to prevent that somebody
can later run ptrace_attach() against the new pid and attach to it
before it changes the uids and makes itself privileged, which is
the point where, if the task executes user code again, it could
be used for an exploit.

But this is prevented as long as the new thread always has
task->task_dumpable set to 0, at which ptrace_attach aborts.

so case b) is safe as long as task_dumpable is always 0 in the
new thread to prevent the attach before it changes gives itself
privileges.

Now, interestingy, you can just remove the

        if (task->ptrace)
                return -EPERM;

from kernel_thread() and let the traced task do the creation
of the new thread. You are protected against b) because of the
task_dumpable flag and I think you are safe against a) - task
being traced already because:

  arch_kernel_thread() calls clone() which calls do_fork() which
  creates a new task_struct by:

 		p = alloc_task_struct();
		if (!p)
			goto fork_out;

	        *p = *current;

  So at first, it copies the complete task_struct, including
  task_dumpable *and* ptrace.

  But later on, it calls copy_flags() which does this:

        if (!(clone_flags & CLONE_PTRACE))
                p->ptrace = 0;

  So unless someone sets CLONE_PTRACE in the clone_flags passed
  to kernel_thread(), which it passed on to clone() and do_fork()
  ptrace is not copied to the new task.

  kernel_thread() is not a system call(would be bad if), it's
  a kernel function to create kernel threads and to have CLONE_PTRACE
  passed to it, the kernel would have to accept clone_flags from
  the user for creating a kernel thread. So far I have not found
  such code, grep did not find anything and I think it would be
  really weird.

  So I really have to assume that CLONE_PTRACE is never passed
  to create a kernel thread. If you are paranoid, you could just
  unmask it in kernel_thread() if you want.

But finally, because of fork()/clone() disables ptrace by
default for the new task, the new task is effectively detached
from the tracer (can't do a ptrace call on it) and nobody
can attach to it(because task_dumpable is 0).

The minimum diff is:

--- kernel/fork.c
+++ kernel/fork.c
@@ -644,16 +644,9 @@
 	unsigned old_task_dumpable;
 	long ret;

-	/* lock out any potential ptracer */
-	task_lock(task);
-	if (task->ptrace) {
-		task_unlock(task);
-		return -EPERM;
-	}
-
 	old_task_dumpable = task->task_dumpable;
+	/* prevent ptrace_attach() on the new task: */
 	task->task_dumpable = 0;
-	task_unlock(task);

 	ret = arch_kernel_thread(fn, arg, flags);


Yes, it looks Evil ;-) But AFAICS it's ok and I've tested it on
my laptop... Note this code (the whole function) was added by
the original code, so it's quite new code which is changed, not
old code.

I'd have to do some tests on bigger SMP machines and some people
trying their exploits on their machine to feel really convinced
that it is correct.

Ok, start flooding me with mail about this terrible looking patch,
but I also hope some people will like to try it and run their clever
exploits against it and tell what they get... I'm interested.

Maybe I've overlooked something, but so far, I could not find it,
so any specific hint is appreciated...

Bernhard

PS:
I've tried some other attempts, but this is the
cleanest way I've found (and also the simplest)

[-- Attachment #2: Type: TEXT/PLAIN, Size: 662 bytes --]

--- linux-2.4.21-rc2/kernel/fork.c
+++ linux-2.4.21-rc2-bk1/kernel/fork.c
@@ -572,21 +572,13 @@
 	unsigned old_task_dumpable;
 	long ret;
 
-	/* lock out any potential ptracer */
-	task_lock(task);
-	if (task->ptrace) {
-		task_unlock(task);
-		return -EPERM;
-	}
-
-	old_task_dumpable = task->task_dumpable;
+	/* lock out any potential ptracer for the new task_struct copy */
 	task->task_dumpable = 0;
-	task_unlock(task);
 
 	ret = arch_kernel_thread(fn, arg, flags);
 
 	/* never reached in child process, only in parent */
-	current->task_dumpable = old_task_dumpable;
+	task->task_dumpable = old_task_dumpable;
 
 	return ret;
 }

Re: [patch] Fix -EPERM returned by kernel_thead() if traced...

[Bernhard Kaindl]

On Mon, 12 May 2003, Chuck Ebbert wrote:
> Bernhard Kaindl wrote:
>
> > -     task_lock(task);
> > -     if (task->ptrace) {
> > -             task_unlock(task);
> > -             return -EPERM;
> > -     }
> > -
> >       old_task_dumpable = task->task_dumpable;
> > +     /* prevent ptrace_attach() on the new task: */
> >       task->task_dumpable = 0;
> > -     task_unlock(task);
>
>   Is it safe to remove the task_lock()?

Yes. After checking, I came to this conclusion:

- the only place were we need to have the task_dumpable flag being 0
  is the *child* task which is created by do_fork() as the result
  of calling arch_kernel_thread().

- the parent task, which only requests the creation of a kernel thread
  but itself does not change itself into one with this call does not even
  need to set task_dumpable to 0 for itself, it's just the easyest way
  to set the task_dumpable = 0 copied to the new task:

  Just to show it, in do_fork(), there is this code at the very beginning:

        retval = -ENOMEM;
        p = alloc_task_struct();
        if (!p)
                goto fork_out;

======> *p = *current;

   This allocates a new task_struct for the skeleton of the new task
   and copies the whole task_struct from the parent (*current) to the
   new task_struct (*p), which also copies current->task_dumpable to
   p->task_dumpable.

   So the new task_struct p has task_dumpable set to the same value
   as current had at this time. And as task_dumpable is never changed
   for with another base than current, task_dumpable cannot change
   behind do_fork() is running. So it's 0 here and 0 for p->task_dumpable.

As said, doing task_dumpable = 0 in the parent before the clone() call
in arch_kernel_thread is only the easyest way to set it.

It's not the best way, because in theory, a tracer could try to attach
to the partent while he is executing arch_kernel_thread and before he
has restored task_dumpable to his old value.

During this time window, such tracer would be wrongly rejected from the
call to ptrace_attach() because withing this window, also the parent has
task_dumpable set to 0, not only the child.

The only clean way to fix this remaining side effect is to create a new
clone_flag (say CLONE_KERNEL) which is used by kernel_thread() to tell
do_fork() (or better copy_flags() within do_fork) to set p->task_dumpable
to 0 (only for the child)

I have not done this yet, but to have all cases covered,
it's the most sane way to do it.

> >  So I really have to assume that CLONE_PTRACE is never passed
> >  to create a kernel thread. If you are paranoid, you could just
> >  unmask it in kernel_thread() if you want.
>
>   I would mask it if it's a possible problem.  Someone might
> try to pass that option in future code...

Sure, no problem, it's easy and protects against such possible future
brain-dead uses of kernel_thread()... But OTOH, the only place where
this would be sufficent to create an exploit would be kmod, and I hope
nobody who does not know what he is doing will mess with it...

Ok, let's see. If nobody protests against adding CLONE_KERNEL support
to copy_flags() to set p->dumpable to 0, I would prepare a patch with
it.

Bernd

[patch] Fix -EPERM returned by kernel_thead() if traced...

[Chuck Ebbert]

Bernhard Kaindl wrote:

> -     task_lock(task);
> -     if (task->ptrace) {
> -             task_unlock(task);
> -             return -EPERM;
> -     }
> -
>       old_task_dumpable = task->task_dumpable;
> +     /* prevent ptrace_attach() on the new task: */
>       task->task_dumpable = 0;
> -     task_unlock(task);

  Is it safe to remove the task_lock()?


>  So I really have to assume that CLONE_PTRACE is never passed
>  to create a kernel thread. If you are paranoid, you could just
>  unmask it in kernel_thread() if you want.

  I would mask it if it's a possible problem.  Someone might
try to pass that option in future code...