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

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

On Thu, Jul 10, 2014 at 07:44:21PM +0200, Florian Weimer wrote:
> On 07/09/2014 12:42 PM, Florian Weimer wrote:
> 
> >Thanks for this suggestion, applied (twice).
> 
> Here's the updated patch, tested on x86_64-redhat-linux-gnu.  Okay to
> commit?

Looks good to me.

Siddhesh

> 
> -- 
> Florian Weimer / Red Hat Product Security

> 2014-07-10  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #17135]
> 	* 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.
> 
> diff --git a/NEWS b/NEWS
> index a6617a1..697b730 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -22,7 +22,7 @@ Version 2.20
>    16927, 16928, 16932, 16943, 16958, 16965, 16966, 16967, 16977, 16978,
>    16984, 16990, 16996, 17009, 17022, 17031, 17042, 17048, 17050, 17058,
>    17061, 17062, 17069, 17075, 17079, 17084, 17086, 17092, 17097, 17125,
> -  17137.
> +  17135, 17137.
>  
>  * Optimized strchr implementation for AArch64.  Contributed by ARM Ltd.
>  
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 67f7d5b..ab3080e 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -271,6 +271,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..d95ffe9 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;
>  
> @@ -1153,11 +1173,14 @@ __nptl_setxid (struct xid_command *cmdp)
>    INTERNAL_SYSCALL_DECL (err);
>    result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, err, 3,
>  				 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> -  if (INTERNAL_SYSCALL_ERROR_P (result, err))
> +  int error = 0;
> +  if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result, err)))
>      {
> -      __set_errno (INTERNAL_SYSCALL_ERRNO (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..9f7b20a 100644
> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -248,10 +248,10 @@ sighandler_setxid (int sig, siginfo_t *si, void *ctx)
>    INTERNAL_SYSCALL_DECL (err);
>    result = INTERNAL_SYSCALL_NCS (__xidcmd->syscall_no, err, 3, __xidcmd->id[0],
>  				 __xidcmd->id[1], __xidcmd->id[2]);
> +  int error = 0;
>    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 ();
> +    error = INTERNAL_SYSCALL_ERRNO (result, err);
> +  __nptl_setxid_error (__xidcmd, error);
>  
>    /* 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"
> -- 
> 1.9.3
>

Attachment: pgpU0uRTHhYKf.pgp
Description: PGP signature