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

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

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"

Attachment: pgpJqpRyjdW4w.pgp
Description: PGP signature