Fix SXID_ERASE behavior in setuid programs (BZ #27471)

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

Ping!

On 3/1/21 7:47 PM, 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
> 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
> +		 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;
> +}
> +
> +/* 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;
> +    }
> +  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);
> +    }
> +  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",
> +};
> +
> +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;
> @@ -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++)
> +	{
> +	  char buf[8];
> +
> +	  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>
> 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.  */
>

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.

On 3/9/21 2:15 AM, Adhemerval Zanella via Libc-alpha wrote:
> 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.

Can I do this as a separate hardening enhancement?  As of now it behaves 
as designed and I also intend to merge SXID_IGNORE into SXID_ERASE so 
that tunables never cross setxid boundaries.

>    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).

I was worried this might be a deeper problem, but turns out it's just a 
typo in your test; it should be glibc.malloc.mmap_threshold, not 
glibc_malloc.mmap_threshold :)

I'm about to post a v2 with the rest of the changes you requested.

Siddhesh

On 16/03/2021 03:58, Siddhesh Poyarekar wrote:
> On 3/9/21 2:15 AM, Adhemerval Zanella via Libc-alpha wrote:
>> 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.
> 
> Can I do this as a separate hardening enhancement?  As of now it behaves as designed and I also intend to merge SXID_IGNORE into SXID_ERASE so that tunables never cross setxid boundaries.

Fair enough.

> 
>>    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).
> 
> I was worried this might be a deeper problem, but turns out it's just a typo in your test; it should be glibc.malloc.mmap_threshold, not glibc_malloc.mmap_threshold :)

Good, one less problem to handle ;)