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

[Siddhesh Poyarekar <siddhesh@sourceware.org>]

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.  */
-- 
2.29.2