Re: [PATCH] Fix SXID_ERASE behavior in setuid programs (BZ #27471)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 01/03/2021 11:17, Siddhesh Poyarekar via Libc-alpha wrote:
> When parse_tunables tries to erase a tunable marked as SXID_ERASE for
> setuid programs, it ends up setting the envvar string iterator
> incorrectly, because of which it may parse the next tunable
> incorrectly.  Given that currently the implementation allows malformed
> and unrecognized tunables pass through, it may even allow SXID_ERASE
> tunables to go through.
> 
> This change revamps the SXID_ERASE implementation so that:
> 
> - Only valid tunables are written back to the tunestr string, because
>   of which children of SXID programs will only inherit a clean list of
>   identified tunables that are not SXID_ERASE.
> 
> - Unrecognized tunables get scrubbed off from the environment and
>   subsequently from the child environment.
> 
> - This has the side-effect that a tunable that is not identified by
>   the setxid binary, will not be passed on to a non-setxid child even
>   if the child could have identified that tunable.  This may break
>   applications that expect this behaviour but expecting such tunables
>   to cross the SXID boundary is wrong.
> 
> The setuid test for tunables has been bolstered to test different
> combinations of tunable values to ensure that the behaviour is now
> consistent.
> ---
>  elf/Makefile                  |   2 -
>  elf/dl-tunables.c             |  56 +++++-----
>  elf/tst-env-setuid-common.c   | 195 ++++++++++++++++++++++++++++++++++
>  elf/tst-env-setuid-tunables.c |  96 +++++++++++++----
>  elf/tst-env-setuid.c          | 182 +------------------------------
>  5 files changed, 298 insertions(+), 233 deletions(-)
>  create mode 100644 elf/tst-env-setuid-common.c
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 16c89b6d07..21eed715ee 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1652,8 +1652,6 @@ $(objpfx)tst-nodelete-dlclose.out: $(objpfx)tst-nodelete-dlclose-dso.so \
>  
>  tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 \
>  		     LD_HWCAP_MASK=0x1
> -tst-env-setuid-tunables-ENV = \
> -	GLIBC_TUNABLES=glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096
>  
>  $(objpfx)tst-debug1: $(libdl)
>  $(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so

Ok.

> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..f67a09605e 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring)
>      return;
>  
>    char *p = tunestr;
> +  size_t off = 0;
>  
>    while (true)
>      {
> @@ -184,7 +185,11 @@ parse_tunables (char *tunestr, char *valstring)
>        /* If we reach the end of the string before getting a valid name-value
>  	 pair, bail out.  */
>        if (p[len] == '\0')
> -	return;
> +	{
> +	  if (__libc_enable_secure)
> +	    tunestr[off] = '\0';
> +	  return;
> +	}
>  
>        /* We did not find a valid name-value pair before encountering the
>  	 colon.  */
> @@ -210,35 +215,28 @@ parse_tunables (char *tunestr, char *valstring)
>  
>  	  if (tunable_is_name (cur->name, name))
>  	    {
> -	      /* If we are in a secure context (AT_SECURE) then ignore the tunable
> -		 unless it is explicitly marked as secure.  Tunable values take
> -		 precedence over their envvar aliases.  */
> +	      /* If we are in a secure context (AT_SECURE) then ignore the
> +		 tunable unless it is explicitly marked as secure.  Tunable
> +		 values take precedence over their envvar aliases.  We write
> +		 the tunables that are not SXID_ERASE, back to TUNESTR, thus

I think this first comma is not required here.

> +		 dropping all SXID_ERASE tunables and any invalid or
> +		 unrecognized tunables.  */
>  	      if (__libc_enable_secure)
>  		{
> -		  if (cur->security_level == TUNABLE_SECLEVEL_SXID_ERASE)
> +		  if (cur->security_level != TUNABLE_SECLEVEL_SXID_ERASE)
>  		    {
> -		      if (p[len] == '\0')
> -			{
> -			  /* Last tunable in the valstring.  Null-terminate and
> -			     return.  */
> -			  *name = '\0';
> -			  return;
> -			}
> -		      else
> -			{
> -			  /* Remove the current tunable from the string.  We do
> -			     this by overwriting the string starting from NAME
> -			     (which is where the current tunable begins) with
> -			     the remainder of the string.  We then have P point
> -			     to NAME so that we continue in the correct
> -			     position in the valstring.  */
> -			  char *q = &p[len + 1];
> -			  p = name;
> -			  while (*q != '\0')
> -			    *name++ = *q++;
> -			  name[0] = '\0';
> -			  len = 0;
> -			}
> +		      if (off > 0)
> +			tunestr[off++] = ':';
> +
> +		      const char *n = cur->name;
> +
> +		      while (*n != '\0')
> +			tunestr[off++] = *n++;
> +
> +		      tunestr[off++] = '=';
> +
> +		      for (size_t j = 0; j < len; j++)
> +			tunestr[off++] = value[j];
>  		    }
>  
>  		  if (cur->security_level != TUNABLE_SECLEVEL_NONE)
> @@ -251,9 +249,7 @@ parse_tunables (char *tunestr, char *valstring)
>  	    }
>  	}
>  
> -      if (p[len] == '\0')
> -	return;
> -      else
> +      if (p[len] != '\0')
>  	p += len + 1;
>      }
>  }
> diff --git a/elf/tst-env-setuid-common.c b/elf/tst-env-setuid-common.c
> new file mode 100644
> index 0000000000..0145599e30
> --- /dev/null
> +++ b/elf/tst-env-setuid-common.c
> @@ -0,0 +1,195 @@
> +/* Common routines for the tst-env-setuid tests.
> +
> +   Copyright (C) 2021 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
> +   <https://www.gnu.org/licenses/>.  */
> +
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
> +
> +#define CHILD_STATUS 42
> +
> +/* Return a GID which is not our current GID, but is present in the
> +   supplementary group list.  */
> +static gid_t
> +choose_gid (void)
> +{
> +  const int count = 64;
> +  gid_t groups[count];
> +  int ret = getgroups (count, groups);
> +  if (ret < 0)
> +    {
> +      printf ("getgroups: %m\n");
> +      exit (1);
> +    }
> +  gid_t current = getgid ();
> +  for (int i = 0; i < ret; ++i)
> +    {
> +      if (groups[i] != current)
> +	return groups[i];
> +    }
> +  return 0;
> +}
> +
> +/* Spawn and execute a program and verify that it returns the CHILD_STATUS.  */
> +static pid_t
> +do_execve (char **args)
> +{
> +  pid_t kid = vfork ();
> +
> +  if (kid < 0)
> +    {
> +      printf ("vfork: %m\n");
> +      return -1;
> +    }
> +
> +  if (kid == 0)
> +    {
> +      /* Child process.  */
> +      execve (args[0], args, environ);
> +      _exit (-errno);
> +    }
> +
> +  if (kid < 0)
> +    return 1;
> +
> +  int status;
> +
> +  if (waitpid (kid, &status, 0) < 0)
> +    {
> +      printf ("waitpid: %m\n");
> +      return 1;
> +    }
> +
> +  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> +    return EXIT_UNSUPPORTED;
> +
> +  if (!WIFEXITED (status) || WEXITSTATUS (status) != CHILD_STATUS)
> +    {
> +      printf ("Unexpected exit status %d from child process\n",
> +	      WEXITSTATUS (status));
> +      return 1;
> +    }
> +  return 0;
> +}

We have support_capture_subprogram that already handles process
spawning, output redirection, and pid handling.

> +
> +/* Copies the executable into a restricted directory, so that we can
> +   safely make it SGID with the TARGET group ID.  Then runs the
> +   executable.  */
> +static int
> +run_executable_sgid (gid_t target, char *argv1)
> +{
> +  char *dirname = xasprintf ("%s/tst-tunables-setuid.%jd",
> +			     test_dir, (intmax_t) getpid ());
> +  char *execname = xasprintf ("%s/bin", dirname);
> +  int infd = -1;
> +  int outfd = -1;
> +  int ret = 0;
> +  if (mkdir (dirname, 0700) < 0)
> +    {
> +      printf ("mkdir: %m\n");
> +      goto err;
> +    }
> +  infd = open ("/proc/self/exe", O_RDONLY);
> +  if (infd < 0)
> +    {
> +      printf ("open (/proc/self/exe): %m\n");
> +      goto err;
> +    }

Does it work on Hurd? If not I think it should make the tests
unsuppported.

> +  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
> +  if (outfd < 0)
> +    {
> +      printf ("open (%s): %m\n", execname);
> +      goto err;
> +    }
> +  char buf[4096];
> +  for (;;)
> +    {
> +      ssize_t rdcount = read (infd, buf, sizeof (buf));
> +      if (rdcount < 0)
> +	{
> +	  printf ("read: %m\n");
> +	  goto err;
> +	}
> +      if (rdcount == 0)
> +	break;
> +      char *p = buf;
> +      char *end = buf + rdcount;
> +      while (p != end)
> +	{
> +	  ssize_t wrcount = write (outfd, buf, end - p);
> +	  if (wrcount == 0)
> +	    errno = ENOSPC;
> +	  if (wrcount <= 0)
> +	    {
> +	      printf ("write: %m\n");
> +	      goto err;
> +	    }
> +	  p += wrcount;
> +	}
> +    }
> +  if (fchown (outfd, getuid (), target) < 0)
> +    {
> +      printf ("fchown (%s): %m\n", execname);
> +      goto err;
> +    }
> +  if (fchmod (outfd, 02750) < 0)
> +    {
> +      printf ("fchmod (%s): %m\n", execname);
> +      goto err;
> +    }
> +  if (close (outfd) < 0)
> +    {
> +      printf ("close (outfd): %m\n");
> +      goto err;
> +    }
> +  if (close (infd) < 0)
> +    {
> +      printf ("close (infd): %m\n");
> +      goto err;
> +    }
> +
> +  char *args[] = {execname, argv1, NULL};
> +
> +  ret = do_execve (args);
> +
> +err:
> +  if (outfd >= 0)
> +    close (outfd);
> +  if (infd >= 0)
> +    close (infd);
> +  if (execname)
> +    {
> +      unlink (execname);
> +      free (execname);
> +    }
> +  if (dirname)
> +    {
> +      rmdir (dirname);
> +      free (dirname);
> +    }

No implicit checks.

It is basically the run_executable_sgid from stdlib/tst-secure-getenv.c
with an extra argument, so maybe we could reuse this code (through
libsupport).

> +  return ret;
> +}
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 50bef8683d..0363622026 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -25,35 +25,47 @@
>  #include "config.h"
>  #undef _LIBC
>  
> -#define test_parent test_parent_tunables
> -#define test_child test_child_tunables
> +#include "tst-env-setuid-common.c"
>  
> -static int test_child_tunables (void);
> -static int test_parent_tunables (void);
> -
> -#include "tst-env-setuid.c"
> -
> -#define CHILD_VALSTRING_VALUE "glibc.malloc.mmap_threshold=4096"
> -#define PARENT_VALSTRING_VALUE \
> -  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096"
> +const char *teststrings[] =
> +{
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096:glibc.malloc.check=2",
> +  "glibc.malloc.check=4:glibc.malloc.garbage=2:glibc.maoc.mmap_threshold=4096",
> +  ":glibc.malloc.garbage=2:glibc.malloc.check=1",
> +  "glibc.malloc.check=1:glibc.malloc.check=2",

Maybe also add an invalid namespace and invalid values?

The above tests

  1. glibc.malloc.perturb=0x800
  2. glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096
  3. glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096
  4. not_valid.malloc.check=2:glibc_malloc.mmap_threshold=4096
  5. not_valid.malloc.check=2
  6. glibc.not_valid.check=2:glibc_malloc.mmap_threshold=4096
  7. glibc.not_valid.check=2

Return:

  1. glibc.malloc.perturb=0x800
  2. glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096
  3. glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096
  4. 
  5. 
  6. 
  7.

Which I think would require more fixes since I see at least two issues:

  1. Invalid values for valid tunables are not being striped off as shown
     by 1., 2., and 3. Although this will invalided by the code that
     actually consumes it, I think we should be really careful and strip
     off such cases for secured binaries.

  2. Invalid namespaces stripping are inconsistent: if there on second
     and forth they are corrected and further tunables are copied (3). 
     However if they are the first one, further valid tunables are
     ignored (6).

> +};
> +
> +const char *resultstrings[] =
> +{
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "",
> +  "",
> +  "",
> +  "",
> +};
>  
>  static int
> -test_child_tunables (void)
> +test_child (int off)
>  {
>    const char *val = getenv ("GLIBC_TUNABLES");
>  
>  #if HAVE_TUNABLES
> -  if (val != NULL && strcmp (val, CHILD_VALSTRING_VALUE) == 0)
> +  if (val != NULL && strcmp (val, resultstrings[off]) == 0)
>      return 0;
>  
>    if (val != NULL)
> -    printf ("Unexpected GLIBC_TUNABLES VALUE %s\n", val);
> +    printf ("[%d] Unexpected GLIBC_TUNABLES VALUE %s\n", off, val);
>  
>    return 1;
>  #else
>    if (val != NULL)
>      {
> -      printf ("GLIBC_TUNABLES not cleared\n");
> +      printf ("[%d] GLIBC_TUNABLES not cleared\n", off);
>        return 1;
>      }
>    return 0;

Ok.

> @@ -61,15 +73,57 @@ test_child_tunables (void)
>  }
>  
>  static int
> -test_parent_tunables (void)
> +do_test (int argc, char **argv)
>  {
> -  const char *val = getenv ("GLIBC_TUNABLES");
> +  /* Setgid child process.  */
> +  if (argc == 2)
> +    {
> +      if (getgid () == getegid ())
> +	{
> +	  /* This can happen if the file system is mounted nosuid.  */
> +	  fprintf (stderr, "SGID failed: GID and EGID match (%jd)\n",
> +		   (intmax_t) getgid ());
> +	  exit (EXIT_UNSUPPORTED);
> +	}
>  
> -  if (val != NULL && strcmp (val, PARENT_VALSTRING_VALUE) == 0)
> -    return 0;
> +      int ret = test_child (atoi (argv[1]));
>  
> -  if (val != NULL)
> -    printf ("Unexpected GLIBC_TUNABLES VALUE %s\n", val);
> +      if (ret != 0)
> +	exit (1);
>  
> -  return 1;
> +      exit (CHILD_STATUS);
> +    }
> +  else
> +    {
> +      int ret = 0;
> +
> +      /* Try running a setgid program.  */
> +      gid_t target = choose_gid ();
> +      if (target == 0)
> +	{
> +	  fprintf (stderr,
> +		   "Could not find a suitable GID for user %jd, skipping test\n",
> +		   (intmax_t) getuid ());
> +	  exit (0);
> +	}
> +
> +      /* Spawn tests.  */
> +      for (int i = 0; i < sizeof (teststrings) / sizeof (char *); i++)

Use array_length.

> +	{
> +	  char buf[8];

Use INT_BUFSIZE_BOUND (int).

> +
> +	  printf ("Spawned test for %s (%d)\n", teststrings[i], i);
> +	  snprintf (buf, sizeof (buf), "%d\n", i);
> +	  if (setenv ("GLIBC_TUNABLES", teststrings[i], 1) != 0)
> +	    exit (1);
> +	  ret |= run_executable_sgid (target, buf);
> +	}
> +      return ret;
> +    }
> +
> +  /* Something went wrong and our argv was corrupted.  */
> +  _exit (1);
>  }
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>

Ok.

> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> index 60ae0ca380..9414d5dfaa 100644
> --- a/elf/tst-env-setuid.c
> +++ b/elf/tst-env-setuid.c
> @@ -19,185 +19,10 @@
>     MALLOC_CHECK_ and MALLOC_MMAP_THRESHOLD_ but also retain
>     MALLOC_MMAP_THRESHOLD_ in an unprivileged child.  */
>  
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <stdlib.h>
> -#include <stdint.h>
> -#include <stdio.h>
> -#include <string.h>
> -#include <sys/stat.h>
> -#include <sys/wait.h>
> -#include <unistd.h>
> -
> -#include <support/support.h>
> -#include <support/test-driver.h>
> +#include "tst-env-setuid-common.c"
>  
>  static char SETGID_CHILD[] = "setgid-child";
> -#define CHILD_STATUS 42
> -
> -/* Return a GID which is not our current GID, but is present in the
> -   supplementary group list.  */
> -static gid_t
> -choose_gid (void)
> -{
> -  const int count = 64;
> -  gid_t groups[count];
> -  int ret = getgroups (count, groups);
> -  if (ret < 0)
> -    {
> -      printf ("getgroups: %m\n");
> -      exit (1);
> -    }
> -  gid_t current = getgid ();
> -  for (int i = 0; i < ret; ++i)
> -    {
> -      if (groups[i] != current)
> -	return groups[i];
> -    }
> -  return 0;
> -}
> -
> -/* Spawn and execute a program and verify that it returns the CHILD_STATUS.  */
> -static pid_t
> -do_execve (char **args)
> -{
> -  pid_t kid = vfork ();
> -
> -  if (kid < 0)
> -    {
> -      printf ("vfork: %m\n");
> -      return -1;
> -    }
> -
> -  if (kid == 0)
> -    {
> -      /* Child process.  */
> -      execve (args[0], args, environ);
> -      _exit (-errno);
> -    }
> -
> -  if (kid < 0)
> -    return 1;
> -
> -  int status;
> -
> -  if (waitpid (kid, &status, 0) < 0)
> -    {
> -      printf ("waitpid: %m\n");
> -      return 1;
> -    }
> -
> -  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> -    return EXIT_UNSUPPORTED;
> -
> -  if (!WIFEXITED (status) || WEXITSTATUS (status) != CHILD_STATUS)
> -    {
> -      printf ("Unexpected exit status %d from child process\n",
> -	      WEXITSTATUS (status));
> -      return 1;
> -    }
> -  return 0;
> -}
> -
> -/* Copies the executable into a restricted directory, so that we can
> -   safely make it SGID with the TARGET group ID.  Then runs the
> -   executable.  */
> -static int
> -run_executable_sgid (gid_t target)
> -{
> -  char *dirname = xasprintf ("%s/tst-tunables-setuid.%jd",
> -			     test_dir, (intmax_t) getpid ());
> -  char *execname = xasprintf ("%s/bin", dirname);
> -  int infd = -1;
> -  int outfd = -1;
> -  int ret = 0;
> -  if (mkdir (dirname, 0700) < 0)
> -    {
> -      printf ("mkdir: %m\n");
> -      goto err;
> -    }
> -  infd = open ("/proc/self/exe", O_RDONLY);
> -  if (infd < 0)
> -    {
> -      printf ("open (/proc/self/exe): %m\n");
> -      goto err;
> -    }
> -  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
> -  if (outfd < 0)
> -    {
> -      printf ("open (%s): %m\n", execname);
> -      goto err;
> -    }
> -  char buf[4096];
> -  for (;;)
> -    {
> -      ssize_t rdcount = read (infd, buf, sizeof (buf));
> -      if (rdcount < 0)
> -	{
> -	  printf ("read: %m\n");
> -	  goto err;
> -	}
> -      if (rdcount == 0)
> -	break;
> -      char *p = buf;
> -      char *end = buf + rdcount;
> -      while (p != end)
> -	{
> -	  ssize_t wrcount = write (outfd, buf, end - p);
> -	  if (wrcount == 0)
> -	    errno = ENOSPC;
> -	  if (wrcount <= 0)
> -	    {
> -	      printf ("write: %m\n");
> -	      goto err;
> -	    }
> -	  p += wrcount;
> -	}
> -    }
> -  if (fchown (outfd, getuid (), target) < 0)
> -    {
> -      printf ("fchown (%s): %m\n", execname);
> -      goto err;
> -    }
> -  if (fchmod (outfd, 02750) < 0)
> -    {
> -      printf ("fchmod (%s): %m\n", execname);
> -      goto err;
> -    }
> -  if (close (outfd) < 0)
> -    {
> -      printf ("close (outfd): %m\n");
> -      goto err;
> -    }
> -  if (close (infd) < 0)
> -    {
> -      printf ("close (infd): %m\n");
> -      goto err;
> -    }
> -
> -  char *args[] = {execname, SETGID_CHILD, NULL};
> -
> -  ret = do_execve (args);
> -
> -err:
> -  if (outfd >= 0)
> -    close (outfd);
> -  if (infd >= 0)
> -    close (infd);
> -  if (execname)
> -    {
> -      unlink (execname);
> -      free (execname);
> -    }
> -  if (dirname)
> -    {
> -      rmdir (dirname);
> -      free (dirname);
> -    }
> -  return ret;
> -}
>  
> -#ifndef test_child
>  static int
>  test_child (void)
>  {
> @@ -221,9 +46,7 @@ test_child (void)
>  
>    return 0;
>  }
> -#endif
>  
> -#ifndef test_parent
>  static int
>  test_parent (void)
>  {
> @@ -247,7 +70,6 @@ test_parent (void)
>  
>    return 0;
>  }
> -#endif
>  
>  static int
>  do_test (int argc, char **argv)
> @@ -285,7 +107,7 @@ do_test (int argc, char **argv)
>  	  exit (0);
>  	}
>  
> -      return run_executable_sgid (target);
> +      return run_executable_sgid (target, SETGID_CHILD);
>      }
>  
>    /* Something went wrong and our argv was corrupted.  */
> 

Ok.