[ping] nptl: Fix abort in case of set*id failure

  If a call to the set*id functions fails in a multi-threaded program,
the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
was triggered.

We address by checking that all calls to set*id on all threads give
the same result, and only abort if we see success followed by failure
(or vice versa).

2014-07-02  Florian Weimer  <fweimer@redhat.com>

	* nptl/pthreadP.h (__nptl_setxid_error): Declare function.
	* nptl/allocatestack.c (__nptl_setxid_error): New function.
	(__nptl_setxid): Initialize error member.  Call
	__nptl_setxid_error.
	* nptl/nptl-init.c (sighandler_setxid): Call __nptl_setxid_error.
	* nptl/descr.h (struct xid_command): Add error member.
	* nptl/tst-setuid3.c: New file.
	* nptl/Makefile (tests): Add it.
  

On Wed, Jul 02, 2014 at 02:18:06PM +0200, Florian Weimer wrote:
> If a call to the set*id functions fails in a multi-threaded program,
> the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
> was triggered.

There should be a bug report for this I think.  This is suitable for
2.20 since it is a regression from a patch that went in earlier to
abort on set*id failure.

> We address by checking that all calls to set*id on all threads give
> the same result, and only abort if we see success followed by failure
> (or vice versa).

The fix looks good to me barring a couple of minor issues that I have
mentioned below.

The original bug report mentions a kernel failure case being the only
case causing the setuid failure, but I think there may be another case
worth mentioning. It goes like this:

1. A calls setuid(x) and starts signalling other threads

2. B is signalled with some other signal and its handler has a call to
   setuid(y) - this is allowed since setuid is considered
   async-signal-safe.

3. B signals A due to which A sets its uid to y

4. A tries calling setuid(x) on itself and fails.

3. and 4. may happen on any other thread too when they're setuid'd to
a uid that can't setuid to the other uid.  This can result in
partially failed setuid calls for A as well as B.

> 
> 2014-07-02  Florian Weimer  <fweimer@redhat.com>
> 

BZ # would go here once you file the bug.

> 	* nptl/pthreadP.h (__nptl_setxid_error): Declare function.
> 	* nptl/allocatestack.c (__nptl_setxid_error): New function.
> 	(__nptl_setxid): Initialize error member.  Call
> 	__nptl_setxid_error.
> 	* nptl/nptl-init.c (sighandler_setxid): Call __nptl_setxid_error.
> 	* nptl/descr.h (struct xid_command): Add error member.
> 	* nptl/tst-setuid3.c: New file.
> 	* nptl/Makefile (tests): Add it.
> 
> -- 
> Florian Weimer / Red Hat Product Security

> diff --git a/nptl/Makefile b/nptl/Makefile
> index cd3be12..34d14c7 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -269,6 +269,7 @@ tests = tst-typesizes \
>  	tst-abstime \
>  	tst-vfork1 tst-vfork2 tst-vfork1x tst-vfork2x \
>  	tst-getpid1 tst-getpid2 tst-getpid3 \
> +	tst-setuid3 \
>  	tst-initializers1 $(patsubst %,tst-initializers1-%,c89 gnu89 c99 gnu99)
>  xtests = tst-setuid1 tst-setuid1-static tst-setuid2 \
>  	tst-mutexpp1 tst-mutexpp6 tst-mutexpp10
> diff --git a/nptl/allocatestack.c b/nptl/allocatestack.c
> index 9095ef4..72a898a 100644
> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c
> @@ -1059,6 +1059,25 @@ setxid_signal_thread (struct xid_command *cmdp, struct pthread *t)
>      return 0;
>  }
>  
> +/* Check for consistency across set*id system call results.  The abort
> +   should not happen as long as all privileges changes happen through
> +   the glibc wrappers.  ERROR must be 0 (no error) or an errno
> +   code.  */
> +void
> +attribute_hidden
> +__nptl_setxid_error (struct xid_command *cmdp, int error)
> +{
> +  do
> +    {
> +      int olderror = cmdp->error;
> +      if (olderror == error)
> +	break;
> +      if (olderror != -1)
> +	/* Mismatch between current and previous results.  */
> +	abort ();
> +    }
> +  while (atomic_compare_and_exchange_bool_acq (&cmdp->error, error, -1));
> +}
>  
>  int
>  attribute_hidden
> @@ -1070,6 +1089,7 @@ __nptl_setxid (struct xid_command *cmdp)
>  
>    __xidcmd = cmdp;
>    cmdp->cntr = 0;
> +  cmdp->error = -1;
>  
>    struct pthread *self = THREAD_SELF;
>  
> @@ -1155,9 +1175,13 @@ __nptl_setxid (struct xid_command *cmdp)
>  				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>    if (INTERNAL_SYSCALL_ERROR_P (result, err))
>      {
> -      __set_errno (INTERNAL_SYSCALL_ERRNO (result, err));
> +      int error = INTERNAL_SYSCALL_ERRNO (result, err);
> +      __nptl_setxid_error (cmdp, error);
> +      __set_errno (error);
>        result = -1;
>      }
> +  else
> +    __nptl_setxid_error (cmdp, 0);

Get ERROR out and initialize it to 0 so that you can call
__nptl_setxid_error just once outside the if/else.  Also, you could
mark the error path as unlikely.  That is:

    int error = 0;
    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
      {
        error = INTERNAL_SYSCALL_ERRNO (result, err);
	__set_errno (error);
	result = -1;
      }
    __nptl_setxid_error (cmdp, error);

>  
>    lll_unlock (stack_cache_lock, LLL_PRIVATE);
>    return result;
> diff --git a/nptl/descr.h b/nptl/descr.h
> index 61d57d5..6738591 100644
> --- a/nptl/descr.h
> +++ b/nptl/descr.h
> @@ -100,6 +100,7 @@ struct xid_command
>    int syscall_no;
>    long int id[3];
>    volatile int cntr;
> +  volatile int error; /* -1: no call yet, 0: success seen, >0: error seen.  */
>  };
>  
>  
> diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
> index 2796dc5..86d9d77 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -249,9 +249,12 @@ sighandler_setxid (int sig, siginfo_t *si, void *ctx)
>    result = INTERNAL_SYSCALL_NCS (__xidcmd->syscall_no, err, 3, __xidcmd->id[0],
>  				 __xidcmd->id[1], __xidcmd->id[2]);
>    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
> -    /* Safety check.  This should never happen if the setxid system
> -       calls are only ever called through their glibc wrappers.  */
> -    abort ();
> +    {
> +      int error = INTERNAL_SYSCALL_ERRNO (result, err);
> +      __nptl_setxid_error (__xidcmd, error);
> +    }
> +  else
> +    __nptl_setxid_error (__xidcmd, 0);

Likewise.

>  
>    /* Reset the SETXID flag.  */
>    struct pthread *self = THREAD_SELF;
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 197401a..94e7890 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -578,6 +578,8 @@ extern void _pthread_cleanup_pop_restore (struct _pthread_cleanup_buffer *buffer
>  
>  extern void __nptl_deallocate_tsd (void) attribute_hidden;
>  
> +extern void __nptl_setxid_error (struct xid_command *cmdp, int error)
> +  attribute_hidden;
>  extern int __nptl_setxid (struct xid_command *cmdp) attribute_hidden;
>  #ifndef SHARED
>  extern void __nptl_set_robust (struct pthread *self);
> diff --git a/nptl/tst-setuid3.c b/nptl/tst-setuid3.c
> new file mode 100644
> index 0000000..f78f485
> --- /dev/null
> +++ b/nptl/tst-setuid3.c
> @@ -0,0 +1,104 @@
> +/* Copyright (C) 2014 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <err.h>
> +#include <errno.h>
> +#include <pthread.h>
> +#include <stdbool.h>
> +#include <unistd.h>
> +
> +/* The test must run under a non-privileged user ID.  */
> +static const uid_t test_uid = 1;
> +
> +static pthread_barrier_t barrier1;
> +static pthread_barrier_t barrier2;
> +
> +static void *
> +thread_func (void *ctx __attribute__ ((unused)))
> +{
> +  int ret = pthread_barrier_wait (&barrier1);
> +  if (ret != PTHREAD_BARRIER_SERIAL_THREAD && ret != 0)
> +    errx (1, "pthread_barrier_wait (barrier1) (on thread): %d", ret);
> +  ret = pthread_barrier_wait (&barrier2);
> +  if (ret != PTHREAD_BARRIER_SERIAL_THREAD && ret != 0)
> +    errx (1, "pthread_barrier_wait (barrier2) (on thread): %d", ret);
> +  return NULL;
> +}
> +
> +static void
> +setuid_failure (int phase)
> +{
> +  int ret = setuid (0);
> +  switch (ret)
> +    {
> +    case 0:
> +      errx (1, "setuid succeeded unexpectedly in phase %d", phase);
> +    case -1:
> +      if (errno != EPERM)
> +	err (1, "setuid phase %d", phase);
> +      break;
> +    default:
> +      errx (1, "invalid setuid return value in phase %d: %d", phase, ret);
> +    }
> +}
> +
> +static int
> +do_test (void)
> +{
> +  if (getuid () == 0)
> +    if (setuid (test_uid) != 0)
> +      err (1, "setuid (%u)", (unsigned) test_uid);
> +  if (setuid (getuid ()))
> +    err (1, "setuid (getuid ())");
> +  setuid_failure (1);
> +
> +  int ret = pthread_barrier_init (&barrier1, NULL, 2);
> +  if (ret != 0)
> +    errx (1, "pthread_barrier_init (barrier1): %d", ret);
> +  ret = pthread_barrier_init (&barrier2, NULL, 2);
> +  if (ret != 0)
> +    errx (1, "pthread_barrier_init (barrier2): %d", ret);
> +
> +  pthread_t thread;
> +  ret = pthread_create (&thread, NULL, thread_func, NULL);
> +  if (ret != 0)
> +    errx (1, "pthread_create: %d", ret);
> +
> +  /* Ensure that the thread is running properly.  */
> +  ret = pthread_barrier_wait (&barrier1);
> +  if (ret != 0)
> +    errx (1, "pthread_barrier_wait (barrier1): %d", ret);
> +
> +  setuid_failure (2);
> +
> +  /* Check success case. */
> +  if (setuid (getuid ()) != 0)
> +    err (1, "setuid (getuid ())");
> +
> +  /* Shutdown.  */
> +  ret = pthread_barrier_wait (&barrier2);
> +  if (ret != PTHREAD_BARRIER_SERIAL_THREAD && ret != 0)
> +    errx (1, "pthread_barrier_wait (barrier2): %d", ret);
> +
> +  if (ret != PTHREAD_BARRIER_SERIAL_THREAD && ret != 0)
> +    errx (1, "pthread_join: %d", ret);
> +
> +  return 0;
> +}
> +
> +#define TEST_FUNCTION do_test ()
> +#include "../test-skeleton.c"
  

On 07/09/2014 08:09 AM, Siddhesh Poyarekar wrote:
> On Wed, Jul 02, 2014 at 02:18:06PM +0200, Florian Weimer wrote:
>> If a call to the set*id functions fails in a multi-threaded program,
>> the abort introduced in commit 13f7fe35ae2b0ea55dc4b9628763aafdc8bdc30c
>> was triggered.
>
> There should be a bug report for this I think.  This is suitable for
> 2.20 since it is a regression from a patch that went in earlier to
> abort on set*id failure.

I filed bug 17135 for this.

>> We address by checking that all calls to set*id on all threads give
>> the same result, and only abort if we see success followed by failure
>> (or vice versa).
>
> The fix looks good to me barring a couple of minor issues that I have
> mentioned below.

Thanks for your review.

> The original bug report mentions a kernel failure case being the only
> case causing the setuid failure, but I think there may be another case
> worth mentioning. It goes like this:
>
> 1. A calls setuid(x) and starts signalling other threads
>
> 2. B is signalled with some other signal and its handler has a call to
>     setuid(y) - this is allowed since setuid is considered
>     async-signal-safe.
>
> 3. B signals A due to which A sets its uid to y
>
> 4. A tries calling setuid(x) on itself and fails.
>
> 3. and 4. may happen on any other thread too when they're setuid'd to
> a uid that can't setuid to the other uid.  This can result in
> partially failed setuid calls for A as well as B.

Doesn't the stack_cache_lock in __nptl_setxid prevent this?  I don't 
know what it does to async-signal-safety, and there is likely some issue 
here, but I think your A/B case is properly locked out.

>> @@ -1155,9 +1175,13 @@ __nptl_setxid (struct xid_command *cmdp)
>>   				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
>>     if (INTERNAL_SYSCALL_ERROR_P (result, err))
>>       {
>> -      __set_errno (INTERNAL_SYSCALL_ERRNO (result, err));
>> +      int error = INTERNAL_SYSCALL_ERRNO (result, err);
>> +      __nptl_setxid_error (cmdp, error);
>> +      __set_errno (error);
>>         result = -1;
>>       }
>> +  else
>> +    __nptl_setxid_error (cmdp, 0);
>
> Get ERROR out and initialize it to 0 so that you can call
> __nptl_setxid_error just once outside the if/else.  Also, you could
> mark the error path as unlikely.  That is:
>
>      int error = 0;
>      if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
>        {
>          error = INTERNAL_SYSCALL_ERRNO (result, err);
> 	__set_errno (error);
> 	result = -1;
>        }
>      __nptl_setxid_error (cmdp, error);

Thanks for this suggestion, applied (twice).
  

On Wed, Jul 09, 2014 at 12:42:56PM +0200, Florian Weimer wrote:
> Doesn't the stack_cache_lock in __nptl_setxid prevent this?  I don't know
> what it does to async-signal-safety, and there is likely some issue here,
> but I think your A/B case is properly locked out.

Of course, I overlooked the lock.  The lock could then deadlock if
thread A is holding the lock and it is interrupted by another signal
and the handler also calls setxid.

Siddhesh
  

On 07/09/2014 12:59 PM, Siddhesh Poyarekar wrote:
> On Wed, Jul 09, 2014 at 12:42:56PM +0200, Florian Weimer wrote:
>> Doesn't the stack_cache_lock in __nptl_setxid prevent this?  I don't know
>> what it does to async-signal-safety, and there is likely some issue here,
>> but I think your A/B case is properly locked out.
>
> Of course, I overlooked the lock.  The lock could then deadlock if
> thread A is holding the lock and it is interrupted by another signal
> and the handler also calls setxid.

Is there anything we can do about this?

(I don't plan to fix it with the current change, I'm just wondering if 
we can improve things further.  The same issue also appears in other 
contexts, and I wonder if there's some idiom which can be used to 
resolve it.)
  

On Wed, Jul 09, 2014 at 01:01:36PM +0200, Florian Weimer wrote:
> On 07/09/2014 12:59 PM, Siddhesh Poyarekar wrote:
> >On Wed, Jul 09, 2014 at 12:42:56PM +0200, Florian Weimer wrote:
> >>Doesn't the stack_cache_lock in __nptl_setxid prevent this?  I don't know
> >>what it does to async-signal-safety, and there is likely some issue here,
> >>but I think your A/B case is properly locked out.
> >
> >Of course, I overlooked the lock.  The lock could then deadlock if
> >thread A is holding the lock and it is interrupted by another signal
> >and the handler also calls setxid.
> 
> Is there anything we can do about this?
> 
> (I don't plan to fix it with the current change, I'm just wondering if we
> can improve things further.  The same issue also appears in other contexts,
> and I wonder if there's some idiom which can be used to resolve it.)

Nothing in the short term I guess, short of masking all signals when
you're in setxid.  In the long term the real fix ought to be to have a
setxid_group set of syscalls that operate on all threads in a process.

Siddhesh