Bug 459901

It is not about glibc trying to be smart, it is simply that POSIX requires the call to affect the whole process, and kernel folks refusing to implement that behavior in the kernel.

*** Bug 555858 has been marked as a duplicate of this bug. ***

Andreas, Jakub,

Do you know if there have been any kernel changes proposed upstream that would allow this to be fixed?

We are wondering what can be done to move this issue to resolution.

thanks

(In reply to comment #1)
>
> It is not about glibc trying to be smart, it is simply that POSIX requires the
> call to affect the whole process, and kernel folks refusing to implement that
> behavior in the kernel.

I don't think we can change sys_setresuid() and break the current long-standing
behaviour. Besides, it is not possible to affect the whole process atomically
(from the check_kill_permission() pov) even in kernel. Of course, it _is_
possible with the additional locking, but imho this is not acceptable.

Afaics, it is easy (but not nice) to change libc to fix this particular problem,
__aio_sigqueue() could take lll_lock(stack_cache_lock) to prevent the race with
__nptl_setxid() which takes this lock too.

Or, we can add something like "seqcount_t xid_seqcount". __nptl_setxid() does
write_seqcount_begin/end and __aio_sigqueue() does

        do {
                seq = read_seqcount_begin(&xid_seqcount);
                ret = INLINE_SYSCALL (rt_sigqueueinfo);
                if (!ret || errno != EPERM)
                        break;
        } while (read_seqcount_retry(&xid_seqcount, seq);

Or something else, we should ask libc developers.

----------------------------------------------------------------------------
On the other hand.

Roland, David. Is there any security reason why tkill should fail when we send
a signal to sub-thread? I don't see any reason. The thread can always send
SIGKILL to all threads. It can block SIGXXX and do kill(getpid(), SIGXXX),
the signal will be delivered to some another sub-thread which may have changed
its credentials. It can even modify .text, etc.

IOW. Can't we simply change check_kill_permission()

        --- x/kernel/signal.c
        +++ b/kernel/signal.c
        @@ -656,6 +656,9 @@ static int check_kill_permission(int sig
                if (error)
                        return error;
        
        +       if (same_thread_group(currant, t))
        +               return 0;
        +
                tcred = __task_cred(t);
                if ((cred->euid ^ tcred->suid) &&
                    (cred->euid ^ tcred->uid) &&

and simplify the life of libc/userspace developers?

(In reply to comment #4)
>
> Roland, David. Is there any security reason why tkill should fail when we send
> a signal to sub-thread? I don't see any reason. The thread can always send
> SIGKILL to all threads. It can block SIGXXX and do kill(getpid(), SIGXXX),
> the signal will be delivered to some another sub-thread which may have changed
> its credentials. It can even modify .text, etc.
> 
> IOW. Can't we simply change check_kill_permission()
> 
>         --- x/kernel/signal.c
>         +++ b/kernel/signal.c
>         @@ -656,6 +656,9 @@ static int check_kill_permission(int sig
>                 if (error)
>                         return error;
> 
>         +       if (same_thread_group(currant, t))
>         +               return 0;
>         +
>                 tcred = __task_cred(t);
>                 if ((cred->euid ^ tcred->suid) &&
>                     (cred->euid ^ tcred->uid) &&
> 
> and simplify the life of libc/userspace developers?    

It certainly would seem reasonable: as you point out, threads can usually modify one another anyway (but that's not an absolute requirement on Linux).

What is notable is that if the thread was cloned with CLONE_THREAD, so that same_thread_group() returns true, then it must also have been cloned with CLONE_SIGHAND, so that the threads share signal handling...

So you might want to rephrase your check to a more inclusive if two threads share the same signal handling, then they can splat each other with signals irrespective of security requirements.

(In reply to comment #5)
>
> > IOW. Can't we simply change check_kill_permission()
> > 
> >         --- x/kernel/signal.c
> >         +++ b/kernel/signal.c
> >         @@ -656,6 +656,9 @@ static int check_kill_permission(int sig
> >                 if (error)
> >                         return error;
> > 
> >         +       if (same_thread_group(currant, t))
> >         +               return 0;
> >         +
> >                 tcred = __task_cred(t);
> >                 if ((cred->euid ^ tcred->suid) &&
> >                     (cred->euid ^ tcred->uid) &&
> > 
> > and simplify the life of libc/userspace developers?    
> 
> It certainly would seem reasonable: as you point out, threads can usually
> modify one another anyway (but that's not an absolute requirement on Linux).

OK, great. Lets move this discussion to lkml, I'll send the updated
patch (we shouldn't miss security_task_kill).

> What is notable is that if the thread was cloned with CLONE_THREAD, so that
> same_thread_group() returns true, then it must also have been cloned with
> CLONE_SIGHAND, so that the threads share signal handling...
> 
> So you might want to rephrase your check to a more inclusive if two threads
> share the same signal handling, then they can splat each other with signals
> irrespective of security requirements.    

Hmm, I didn't think about this... Agreed, probably it makes more sense
to check current->sighand == t->sighand to detect the CLONE_SIGHAND case.

But for now I'm inclined to send the patch which checks same_thread_group(),
because it is easier to explain this change. And, CLONE_SIGHAND without
CLONE_THREAD is exotic, I'm afraid the subtleness of CLONE_SIGHAND change
won't be tested at all, and then later we may have the "breaks my testcase"
reports.

IOW, same_thread_group() looks a bit safer and more straightforward to me,
at least for now.

OK?

The patch is already in -mm tree
signals-check_kill_permission-dont-check-creds-if-same_thread_group.patch
and it has acks.

Hopefully it will be merged in this window, after that I'll forward
it to rhkl.

[RHEL5 PATCH] bz#459901: signals: check_kill_permission(): don't check creds if same_thread_group()
http://post-office.corp.redhat.com/archives/rhkernel-list/2010-May/msg03168.html

Switching component to kernel, assignee to Oleg.

This request was evaluated by Red Hat Product Management for inclusion in a Red
Hat Enterprise Linux maintenance release.  Product Management has requested
further review of this request by Red Hat Engineering, for potential
inclusion in a Red Hat Enterprise Linux Update release for currently deployed
products.  This request is not yet committed for inclusion in an Update
release.

in kernel-2.6.18-207.el5
You can download this test kernel from http://people.redhat.com/jwilson/el5

Detailed testing feedback is always welcomed.

@Reporter, @Jeremy, did the reporter test the kernel in comment #20 yet? If so, please let us know the results here. Thanks!

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2011-0017.html