[PATCH v2 0/4] tunables and setxid programs

[PATCH v2 0/4] tunables and setxid programs

[Siddhesh Poyarekar]

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.

Siddhesh Poyarekar (4):
  support: Add capability to fork an sgid child
  tst-env-setuid: Use support_capture_subprogram_self_sgid
  Enhance setuid-tunables test
  Fix SXID_ERASE behavior in setuid programs (BZ #27471)

 elf/Makefile                         |   2 -
 elf/dl-tunables.c                    |  56 ++++----
 elf/tst-env-setuid-tunables.c        | 118 +++++++++++++---
 elf/tst-env-setuid.c                 | 197 ++------------------------
 stdlib/tst-secure-getenv.c           | 199 +++------------------------
 support/capture_subprocess.h         |   6 +
 support/check.h                      |  12 ++
 support/subprocess.h                 |   5 +
 support/support_capture_subprocess.c | 114 +++++++++++++++
 support/support_subprocess.c         |  13 ++
 10 files changed, 304 insertions(+), 418 deletions(-)

-- 
2.29.2

[PATCH 1/4] support: Add capability to fork an sgid child

[Siddhesh Poyarekar]

Add a new function support_capture_subprogram_self_sgid that spawns an
sgid child of the running program with its own image and returns the
exit code of the child process.  This functionality is used by at
least three tests in the testsuite at the moment, so it makes sense to
consolidate.

There is also a new function support_subprogram_wait which should
provide simple system() like functionality that does not set up file
actions.  This is useful in cases where only the return code of the
spawned subprocess is interesting.

This patch also ports tst-secure-getenv to this new function.  A
subsequent patch will port other tests.  This also brings an important
change to tst-secure-getenv behaviour.  Now instead of succeeding, the
test fails as UNSUPPORTED if it is unable to spawn a setgid child,
which is how it should have been in the first place.
---
 stdlib/tst-secure-getenv.c           | 199 +++------------------------
 support/capture_subprocess.h         |   6 +
 support/check.h                      |  12 ++
 support/subprocess.h                 |   5 +
 support/support_capture_subprocess.c | 114 +++++++++++++++
 support/support_subprocess.c         |  13 ++
 6 files changed, 168 insertions(+), 181 deletions(-)

diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
index c9ec03866f..5567c9ae21 100644
--- a/stdlib/tst-secure-getenv.c
+++ b/stdlib/tst-secure-getenv.c
@@ -30,167 +30,12 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include <support/check.h>
 #include <support/support.h>
+#include <support/capture_subprocess.h>
 #include <support/test-driver.h>
 
 static char MAGIC_ARGUMENT[] = "run-actual-test";
-#define MAGIC_STATUS 19
-
-/* Return a GID which is not our current GID, but is present in the
-   supplementary group list.  */
-static gid_t
-choose_gid (void)
-{
-  int count = getgroups (0, NULL);
-  if (count < 0)
-    {
-      printf ("getgroups: %m\n");
-      exit (1);
-    }
-  gid_t *groups;
-  groups = xcalloc (count, sizeof (*groups));
-  int ret = getgroups (count, groups);
-  if (ret < 0)
-    {
-      printf ("getgroups: %m\n");
-      exit (1);
-    }
-  gid_t current = getgid ();
-  gid_t not_current = 0;
-  for (int i = 0; i < ret; ++i)
-    {
-      if (groups[i] != current)
-        {
-          not_current = groups[i];
-          break;
-        }
-    }
-  free (groups);
-  return not_current;
-}
-
-
-/* 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/secure-getenv.%jd",
-			     test_dir, (intmax_t) getpid ());
-  char *execname = xasprintf ("%s/bin", dirname);
-  int infd = -1;
-  int outfd = -1;
-  int ret = -1;
-  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;
-    }
-
-  int kid = fork ();
-  if (kid < 0)
-    {
-      printf ("fork: %m\n");
-      goto err;
-    }
-  if (kid == 0)
-    {
-      /* Child process.  */
-      char *args[] = { execname, MAGIC_ARGUMENT, NULL };
-      execve (execname, args, environ);
-      printf ("execve (%s): %m\n", execname);
-      _exit (1);
-    }
-  int status;
-  if (waitpid (kid, &status, 0) < 0)
-    {
-      printf ("waitpid: %m\n");
-      goto err;
-    }
-  if (!WIFEXITED (status) || WEXITSTATUS (status) != MAGIC_STATUS)
-    {
-      printf ("Unexpected exit status %d from child process\n",
-	      status);
-      goto err;
-    }
-  ret = 0;
-
-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;
-}
 
 static int
 do_test (void)
@@ -212,15 +57,15 @@ do_test (void)
       exit (1);
     }
 
-  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);
-    }
-  return run_executable_sgid (target);
+  int status = support_capture_subprogram_self_sgid (MAGIC_ARGUMENT);
+
+  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
+    return EXIT_UNSUPPORTED;
+
+  if (!WIFEXITED (status))
+    FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
+
+  return 0;
 }
 
 static void
@@ -229,23 +74,15 @@ alternative_main (int argc, char **argv)
   if (argc == 2 && strcmp (argv[1], MAGIC_ARGUMENT) == 0)
     {
       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 (MAGIC_STATUS);
-	}
+	/* This can happen if the file system is mounted nosuid.  */
+	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
+		   (intmax_t) getgid ());
       if (getenv ("PATH") == NULL)
-	{
-	  printf ("PATH variable not present\n");
-	  exit (3);
-	}
+	FAIL_EXIT (3, "PATH variable not present\n");
       if (secure_getenv ("PATH") != NULL)
-	{
-	  printf ("PATH variable not filtered out\n");
-	  exit (4);
-	}
-      exit (MAGIC_STATUS);
+	FAIL_EXIT (4, "PATH variable not filtered out\n");
+
+      exit (EXIT_SUCCESS);
     }
 }
 
diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
index 8969d4a99a..4be430f099 100644
--- a/support/capture_subprocess.h
+++ b/support/capture_subprocess.h
@@ -41,6 +41,12 @@ struct support_capture_subprocess support_capture_subprocess
 struct support_capture_subprocess support_capture_subprogram
   (const char *file, char *const argv[]);
 
+/* Copy the running program into a setgid binary and run it with CHILD_ID
+   argument.  If execution is successful, return the exit status of the child
+   program, otherwise return a non-zero failure exit code.  */
+int support_capture_subprogram_self_sgid
+  (char *child_id);
+
 /* Deallocate the subprocess data captured by
    support_capture_subprocess.  */
 void support_capture_subprocess_free (struct support_capture_subprocess *);
diff --git a/support/check.h b/support/check.h
index 83662b2d10..801e34e943 100644
--- a/support/check.h
+++ b/support/check.h
@@ -54,6 +54,18 @@ __BEGIN_DECLS
       support_test_verify_impl (__FILE__, __LINE__, #expr);     \
   })
 
+/* Record a test failure and jump to LABEL if EXPR evaluates to false.  */
+#define TEST_VERIFY_GOTO(expr, label)                           \
+  ({                                                            \
+    if ((expr))                                                   \
+      ;                                                         \
+    else                                                        \
+      {                                                         \
+        support_test_verify_impl (__FILE__, __LINE__, #expr);   \
+        goto label;                                             \
+      }                                                         \
+  })
+
 /* Record a test failure and exit if EXPR evaluates to false.  */
 #define TEST_VERIFY_EXIT(expr)                                  \
   ({                                                            \
diff --git a/support/subprocess.h b/support/subprocess.h
index 11cfc6a07f..40d82c7e4d 100644
--- a/support/subprocess.h
+++ b/support/subprocess.h
@@ -38,6 +38,11 @@ struct support_subprocess support_subprocess
 struct support_subprocess support_subprogram
   (const char *file, char *const argv[]);
 
+/* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
+   to complete.  Return program exit status.  */
+int support_subprogram_wait
+  (const char *file, char *const argv[]);
+
 /* Wait for the subprocess indicated by PROC::PID.  Return the status
    indicate by waitpid call.  */
 int support_process_wait (struct support_subprocess *proc);
diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index 3eb825b9af..bb61fecaaf 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -20,11 +20,14 @@
 #include <support/capture_subprocess.h>
 
 #include <errno.h>
+#include <fcntl.h>
 #include <stdlib.h>
 #include <support/check.h>
 #include <support/xunistd.h>
 #include <support/xsocket.h>
 #include <support/xspawn.h>
+#include <support/support.h>
+#include <support/test-driver.h>
 
 static void
 transfer (const char *what, struct pollfd *pfd, struct xmemstream *stream)
@@ -102,6 +105,117 @@ support_capture_subprogram (const char *file, char *const argv[])
   return result;
 }
 
+/* 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
+copy_and_spawn_sgid (char *child_id, gid_t gid)
+{
+  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 = 1, status = 1;
+
+  TEST_VERIFY_GOTO (mkdir (dirname, 0700) >= 0, err);
+
+  infd = open ("/proc/self/exe", O_RDONLY);
+  if (infd < 0)
+    FAIL_UNSUPPORTED ("unsupported: Cannot read binary from procfs\n");
+
+  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
+  TEST_VERIFY_GOTO (outfd >= 0, err);
+
+  char buf[4096];
+  for (;;)
+    {
+      ssize_t rdcount = read (infd, buf, sizeof (buf));
+      TEST_VERIFY_GOTO (rdcount >= 0, 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;
+	  TEST_VERIFY_GOTO (wrcount > 0, err);
+	  p += wrcount;
+	}
+    }
+  int goto_ret = fchown (outfd, getuid (), gid);
+  TEST_VERIFY_GOTO (goto_ret == 0, err);
+  goto_ret = fchmod (outfd, 02750);
+  TEST_VERIFY_GOTO (goto_ret == 0, err);
+  goto_ret = close (outfd);
+  TEST_VERIFY_GOTO (goto_ret == 0, err);
+  goto_ret = close (infd);
+  TEST_VERIFY_GOTO (goto_ret == 0, err);
+
+  /* We have the binary, now spawn the subprocess.  Avoid using
+     support_subprogram because we only want the program exit status, not the
+     contents.  */
+  ret = 0;
+
+  char * const args[] = {execname, child_id, NULL};
+
+  status = support_subprogram_wait (args[0], args);
+
+err:
+  if (outfd >= 0)
+    close (outfd);
+  if (infd >= 0)
+    close (infd);
+  if (execname != NULL)
+    {
+      unlink (execname);
+      free (execname);
+    }
+  if (dirname != NULL)
+    {
+      rmdir (dirname);
+      free (dirname);
+    }
+
+  if (ret != 0)
+    FAIL_EXIT1("Failed to make sgid executable for test\n");
+
+  return status;
+}
+
+int
+support_capture_subprogram_self_sgid (char *child_id)
+{
+  gid_t target = 0;
+  const int count = 64;
+  gid_t groups[count];
+
+  /* Get a GID which is not our current GID, but is present in the
+     supplementary group list.  */
+  int ret = getgroups (count, groups);
+  if (ret < 0)
+    FAIL_UNSUPPORTED("Could not get group list for user %jd\n",
+		     (intmax_t) getuid ());
+
+  gid_t current = getgid ();
+  for (int i = 0; i < ret; ++i)
+    {
+      if (groups[i] != current)
+	{
+	  target = groups[i];
+	  break;
+	}
+    }
+
+  if (target == 0)
+    FAIL_UNSUPPORTED("Could not find a suitable GID for user %jd\n",
+		     (intmax_t) getuid ());
+
+  return copy_and_spawn_sgid (child_id, target);
+}
+
 void
 support_capture_subprocess_free (struct support_capture_subprocess *p)
 {
diff --git a/support/support_subprocess.c b/support/support_subprocess.c
index 2acfc57b7e..89e767ae47 100644
--- a/support/support_subprocess.c
+++ b/support/support_subprocess.c
@@ -92,6 +92,19 @@ support_subprogram (const char *file, char *const argv[])
   return result;
 }
 
+int
+support_subprogram_wait (const char *file, char *const argv[])
+{
+  posix_spawn_file_actions_t fa;
+
+  posix_spawn_file_actions_init (&fa);
+  struct support_subprocess res = support_subprocess_init ();
+
+  res.pid = xposix_spawn (file, &fa, NULL, argv, environ);
+
+  return support_process_wait (&res);
+}
+
 int
 support_process_wait (struct support_subprocess *proc)
 {
-- 
2.29.2

[PATCH 2/4] tst-env-setuid: Use support_capture_subprogram_self_sgid

[Siddhesh Poyarekar]

Use the support_capture_subprogram_self_sgid to spawn an sgid child.
---
 elf/tst-env-setuid.c | 197 +++----------------------------------------
 1 file changed, 14 insertions(+), 183 deletions(-)

diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
index 60ae0ca380..49b5e319e2 100644
--- a/elf/tst-env-setuid.c
+++ b/elf/tst-env-setuid.c
@@ -29,173 +29,12 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include <support/check.h>
 #include <support/support.h>
 #include <support/test-driver.h>
+#include <support/capture_subprocess.h>
 
 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
@@ -256,40 +95,32 @@ do_test (int argc, char **argv)
   if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0)
     {
       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);
-	}
+	/* This can happen if the file system is mounted nosuid.  */
+	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
+			  (intmax_t) getgid ());
 
       int ret = test_child ();
 
       if (ret != 0)
 	exit (1);
 
-      exit (CHILD_STATUS);
+      exit (EXIT_SUCCESS);
     }
   else
     {
       if (test_parent () != 0)
 	exit (1);
 
-      /* 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);
-	}
+      int status = support_capture_subprogram_self_sgid (SETGID_CHILD);
 
-      return run_executable_sgid (target);
-    }
+      if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
+	return EXIT_UNSUPPORTED;
+
+      if (!WIFEXITED (status))
+	FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
 
-  /* Something went wrong and our argv was corrupted.  */
-  _exit (1);
+      return 0;
+    }
 }
 
 #define TEST_FUNCTION_ARGV do_test
-- 
2.29.2

[PATCH 3/4] Enhance setuid-tunables test

[Siddhesh Poyarekar]

Instead of passing GLIBC_TUNABLES via the environment, pass the
environment variable from parent to child.  This allows us to test
multiple variables to ensure better coverage.

The test list currently only includes the case that's already being
tested.  More tests will be added later.
---
 elf/Makefile                  |  2 -
 elf/tst-env-setuid-tunables.c | 90 +++++++++++++++++++++++++++--------
 2 files changed, 69 insertions(+), 23 deletions(-)

diff --git a/elf/Makefile b/elf/Makefile
index 3b8e13e066..4e04c26eea 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1653,8 +1653,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/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
index 50bef8683d..3d523875b1 100644
--- a/elf/tst-env-setuid-tunables.c
+++ b/elf/tst-env-setuid-tunables.c
@@ -25,35 +25,50 @@
 #include "config.h"
 #undef _LIBC
 
-#define test_parent test_parent_tunables
-#define test_child test_child_tunables
-
-static int test_child_tunables (void);
-static int test_parent_tunables (void);
-
-#include "tst-env-setuid.c"
+#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 <intprops.h>
+#include <array_length.h>
+
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <support/capture_subprocess.h>
+
+const char *teststrings[] =
+{
+  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+};
 
-#define CHILD_VALSTRING_VALUE "glibc.malloc.mmap_threshold=4096"
-#define PARENT_VALSTRING_VALUE \
-  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096"
+const char *resultstrings[] =
+{
+  "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 +76,48 @@ 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.  */
+	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
+			  (intmax_t) getgid ());
 
-  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 (EXIT_SUCCESS);
+    }
+  else
+    {
+      int ret = 0;
+
+      /* Spawn tests.  */
+      for (int i = 0; i < array_length (teststrings); i++)
+	{
+	  char buf[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);
+
+	  int status = support_capture_subprogram_self_sgid (buf);
+
+	  /* Bail out early if unsupported.  */
+	  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
+	    return EXIT_UNSUPPORTED;
+
+	  ret |= status;
+	}
+      return ret;
+    }
 }
+
+#define TEST_FUNCTION_ARGV do_test
+#include <support/test-driver.c>
-- 
2.29.2

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

[Siddhesh Poyarekar]

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.
---
 elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
 elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
 2 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
index a2be9cde2f..1aedb9bd36 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-tunables.c b/elf/tst-env-setuid-tunables.c
index 3d523875b1..05619c9adc 100644
--- a/elf/tst-env-setuid-tunables.c
+++ b/elf/tst-env-setuid-tunables.c
@@ -45,11 +45,37 @@
 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.perturb=0x800",
+  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+  "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
+  "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
+  "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",
+  "not_valid.malloc.check=2",
+  "glibc.not_valid.check=2",
 };
 
 const char *resultstrings[] =
 {
   "glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.perturb=0x800",
+  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.mmap_threshold=4096",
+  "glibc.malloc.mmap_threshold=4096",
+  "",
+  "",
+  "",
+  "",
+  "",
+  "",
 };
 
 static int
-- 
2.29.2

[PING][PATCH v2 0/4] tunables and setxid programs

[Siddhesh Poyarekar]

On 3/16/21 12:37 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.
> 
> Siddhesh Poyarekar (4):
>    support: Add capability to fork an sgid child
>    tst-env-setuid: Use support_capture_subprogram_self_sgid
>    Enhance setuid-tunables test
>    Fix SXID_ERASE behavior in setuid programs (BZ #27471)
> 
>   elf/Makefile                         |   2 -
>   elf/dl-tunables.c                    |  56 ++++----
>   elf/tst-env-setuid-tunables.c        | 118 +++++++++++++---
>   elf/tst-env-setuid.c                 | 197 ++------------------------
>   stdlib/tst-secure-getenv.c           | 199 +++------------------------
>   support/capture_subprocess.h         |   6 +
>   support/check.h                      |  12 ++
>   support/subprocess.h                 |   5 +
>   support/support_capture_subprocess.c | 114 +++++++++++++++
>   support/support_subprocess.c         |  13 ++
>   10 files changed, 304 insertions(+), 418 deletions(-)
> 

Re: [PATCH 1/4] support: Add capability to fork an sgid child

[Carlos O'Donell]

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> Add a new function support_capture_subprogram_self_sgid that spawns an
> sgid child of the running program with its own image and returns the
> exit code of the child process.  This functionality is used by at
> least three tests in the testsuite at the moment, so it makes sense to
> consolidate.

Agreed.

> There is also a new function support_subprogram_wait which should
> provide simple system() like functionality that does not set up file
> actions.  This is useful in cases where only the return code of the
> spawned subprocess is interesting.

OK.

> This patch also ports tst-secure-getenv to this new function.  A
> subsequent patch will port other tests.  This also brings an important
> change to tst-secure-getenv behaviour.  Now instead of succeeding, the
> test fails as UNSUPPORTED if it is unable to spawn a setgid child,
> which is how it should have been in the first place.

Please fix the control-flow-in-macro issue and repost v2.

> ---
>  stdlib/tst-secure-getenv.c           | 199 +++------------------------
>  support/capture_subprocess.h         |   6 +
>  support/check.h                      |  12 ++
>  support/subprocess.h                 |   5 +
>  support/support_capture_subprocess.c | 114 +++++++++++++++
>  support/support_subprocess.c         |  13 ++
>  6 files changed, 168 insertions(+), 181 deletions(-)
> 
> diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
> index c9ec03866f..5567c9ae21 100644
> --- a/stdlib/tst-secure-getenv.c
> +++ b/stdlib/tst-secure-getenv.c
> @@ -30,167 +30,12 @@
>  #include <sys/wait.h>
>  #include <unistd.h>
>  
> +#include <support/check.h>
>  #include <support/support.h>
> +#include <support/capture_subprocess.h>
>  #include <support/test-driver.h>
>  
>  static char MAGIC_ARGUMENT[] = "run-actual-test";
> -#define MAGIC_STATUS 19
> -
> -/* Return a GID which is not our current GID, but is present in the
> -   supplementary group list.  */
> -static gid_t
> -choose_gid (void)
> -{
> -  int count = getgroups (0, NULL);
> -  if (count < 0)
> -    {
> -      printf ("getgroups: %m\n");
> -      exit (1);
> -    }
> -  gid_t *groups;
> -  groups = xcalloc (count, sizeof (*groups));
> -  int ret = getgroups (count, groups);
> -  if (ret < 0)
> -    {
> -      printf ("getgroups: %m\n");
> -      exit (1);
> -    }
> -  gid_t current = getgid ();
> -  gid_t not_current = 0;
> -  for (int i = 0; i < ret; ++i)
> -    {
> -      if (groups[i] != current)
> -        {
> -          not_current = groups[i];
> -          break;
> -        }
> -    }
> -  free (groups);
> -  return not_current;
> -}
> -
> -
> -/* 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/secure-getenv.%jd",
> -			     test_dir, (intmax_t) getpid ());
> -  char *execname = xasprintf ("%s/bin", dirname);
> -  int infd = -1;
> -  int outfd = -1;
> -  int ret = -1;
> -  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;
> -    }
> -
> -  int kid = fork ();
> -  if (kid < 0)
> -    {
> -      printf ("fork: %m\n");
> -      goto err;
> -    }
> -  if (kid == 0)
> -    {
> -      /* Child process.  */
> -      char *args[] = { execname, MAGIC_ARGUMENT, NULL };
> -      execve (execname, args, environ);
> -      printf ("execve (%s): %m\n", execname);
> -      _exit (1);
> -    }
> -  int status;
> -  if (waitpid (kid, &status, 0) < 0)
> -    {
> -      printf ("waitpid: %m\n");
> -      goto err;
> -    }
> -  if (!WIFEXITED (status) || WEXITSTATUS (status) != MAGIC_STATUS)
> -    {
> -      printf ("Unexpected exit status %d from child process\n",
> -	      status);
> -      goto err;
> -    }
> -  ret = 0;
> -
> -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;
> -}
>  
>  static int
>  do_test (void)
> @@ -212,15 +57,15 @@ do_test (void)
>        exit (1);
>      }
>  
> -  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);
> -    }
> -  return run_executable_sgid (target);
> +  int status = support_capture_subprogram_self_sgid (MAGIC_ARGUMENT);
> +
> +  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> +    return EXIT_UNSUPPORTED;
> +
> +  if (!WIFEXITED (status))
> +    FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
> +
> +  return 0;

OK. Refactor.

>  }
>  
>  static void
> @@ -229,23 +74,15 @@ alternative_main (int argc, char **argv)
>    if (argc == 2 && strcmp (argv[1], MAGIC_ARGUMENT) == 0)
>      {
>        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 (MAGIC_STATUS);
> -	}
> +	/* This can happen if the file system is mounted nosuid.  */
> +	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
> +		   (intmax_t) getgid ());
>        if (getenv ("PATH") == NULL)
> -	{
> -	  printf ("PATH variable not present\n");
> -	  exit (3);
> -	}
> +	FAIL_EXIT (3, "PATH variable not present\n");
>        if (secure_getenv ("PATH") != NULL)
> -	{
> -	  printf ("PATH variable not filtered out\n");
> -	  exit (4);
> -	}
> -      exit (MAGIC_STATUS);
> +	FAIL_EXIT (4, "PATH variable not filtered out\n");
> +
> +      exit (EXIT_SUCCESS);
>      }

OK. Refactor.

>  }
>  
> diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
> index 8969d4a99a..4be430f099 100644
> --- a/support/capture_subprocess.h
> +++ b/support/capture_subprocess.h
> @@ -41,6 +41,12 @@ struct support_capture_subprocess support_capture_subprocess
>  struct support_capture_subprocess support_capture_subprogram
>    (const char *file, char *const argv[]);
>  
> +/* Copy the running program into a setgid binary and run it with CHILD_ID
> +   argument.  If execution is successful, return the exit status of the child
> +   program, otherwise return a non-zero failure exit code.  */
> +int support_capture_subprogram_self_sgid
> +  (char *child_id);
> +

OK.

>  /* Deallocate the subprocess data captured by
>     support_capture_subprocess.  */
>  void support_capture_subprocess_free (struct support_capture_subprocess *);
> diff --git a/support/check.h b/support/check.h
> index 83662b2d10..801e34e943 100644
> --- a/support/check.h
> +++ b/support/check.h
> @@ -54,6 +54,18 @@ __BEGIN_DECLS
>        support_test_verify_impl (__FILE__, __LINE__, #expr);     \
>    })
>  
> +/* Record a test failure and jump to LABEL if EXPR evaluates to false.  */
> +#define TEST_VERIFY_GOTO(expr, label)                           \
> +  ({                                                            \
> +    if ((expr))                                                   \
> +      ;                                                         \
> +    else                                                        \
> +      {                                                         \
> +        support_test_verify_impl (__FILE__, __LINE__, #expr);   \
> +        goto label;                                             \
> +      }                                                         \
> +  })

Please put the control flow outside of the macro and use a normal macro.

> +
>  /* Record a test failure and exit if EXPR evaluates to false.  */
>  #define TEST_VERIFY_EXIT(expr)                                  \
>    ({                                                            \
> diff --git a/support/subprocess.h b/support/subprocess.h
> index 11cfc6a07f..40d82c7e4d 100644
> --- a/support/subprocess.h
> +++ b/support/subprocess.h
> @@ -38,6 +38,11 @@ struct support_subprocess support_subprocess
>  struct support_subprocess support_subprogram
>    (const char *file, char *const argv[]);
>  
> +/* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
> +   to complete.  Return program exit status.  */
> +int support_subprogram_wait
> +  (const char *file, char *const argv[]);
> +
>  /* Wait for the subprocess indicated by PROC::PID.  Return the status
>     indicate by waitpid call.  */
>  int support_process_wait (struct support_subprocess *proc);
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index 3eb825b9af..bb61fecaaf 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -20,11 +20,14 @@
>  #include <support/capture_subprocess.h>
>  
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <stdlib.h>
>  #include <support/check.h>
>  #include <support/xunistd.h>
>  #include <support/xsocket.h>
>  #include <support/xspawn.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
>  
>  static void
>  transfer (const char *what, struct pollfd *pfd, struct xmemstream *stream)
> @@ -102,6 +105,117 @@ support_capture_subprogram (const char *file, char *const argv[])
>    return result;
>  }
>  
> +/* 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
> +copy_and_spawn_sgid (char *child_id, gid_t gid)
> +{
> +  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 = 1, status = 1;
> +
> +  TEST_VERIFY_GOTO (mkdir (dirname, 0700) >= 0, err);
> +
> +  infd = open ("/proc/self/exe", O_RDONLY);
> +  if (infd < 0)
> +    FAIL_UNSUPPORTED ("unsupported: Cannot read binary from procfs\n");
> +
> +  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
> +  TEST_VERIFY_GOTO (outfd >= 0, err);
> +
> +  char buf[4096];

OK. Fixed buffer. 

> +  for (;;)
> +    {
> +      ssize_t rdcount = read (infd, buf, sizeof (buf));
> +      TEST_VERIFY_GOTO (rdcount >= 0, err);

OK. Error out goto.

> +      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;
> +	  TEST_VERIFY_GOTO (wrcount > 0, err);
> +	  p += wrcount;
> +	}
> +    }
> +  int goto_ret = fchown (outfd, getuid (), gid);
> +  TEST_VERIFY_GOTO (goto_ret == 0, err);
> +  goto_ret = fchmod (outfd, 02750);
> +  TEST_VERIFY_GOTO (goto_ret == 0, err);
> +  goto_ret = close (outfd);
> +  TEST_VERIFY_GOTO (goto_ret == 0, err);
> +  goto_ret = close (infd);
> +  TEST_VERIFY_GOTO (goto_ret == 0, err);
> +
> +  /* We have the binary, now spawn the subprocess.  Avoid using
> +     support_subprogram because we only want the program exit status, not the
> +     contents.  */
> +  ret = 0;
> +
> +  char * const args[] = {execname, child_id, NULL};
> +
> +  status = support_subprogram_wait (args[0], args);
> +
> +err:
> +  if (outfd >= 0)
> +    close (outfd);
> +  if (infd >= 0)
> +    close (infd);
> +  if (execname != NULL)
> +    {
> +      unlink (execname);
> +      free (execname);
> +    }
> +  if (dirname != NULL)
> +    {
> +      rmdir (dirname);
> +      free (dirname);
> +    }
> +
> +  if (ret != 0)
> +    FAIL_EXIT1("Failed to make sgid executable for test\n");
> +
> +  return status;
> +}
> +
> +int
> +support_capture_subprogram_self_sgid (char *child_id)
> +{
> +  gid_t target = 0;
> +  const int count = 64;
> +  gid_t groups[count];
> +
> +  /* Get a GID which is not our current GID, but is present in the
> +     supplementary group list.  */
> +  int ret = getgroups (count, groups);
> +  if (ret < 0)
> +    FAIL_UNSUPPORTED("Could not get group list for user %jd\n",
> +		     (intmax_t) getuid ());
> +
> +  gid_t current = getgid ();
> +  for (int i = 0; i < ret; ++i)
> +    {
> +      if (groups[i] != current)
> +	{
> +	  target = groups[i];
> +	  break;
> +	}
> +    }
> +
> +  if (target == 0)
> +    FAIL_UNSUPPORTED("Could not find a suitable GID for user %jd\n",
> +		     (intmax_t) getuid ());
> +
> +  return copy_and_spawn_sgid (child_id, target);
> +}
> +

OK. Refactor.

>  void
>  support_capture_subprocess_free (struct support_capture_subprocess *p)
>  {
> diff --git a/support/support_subprocess.c b/support/support_subprocess.c
> index 2acfc57b7e..89e767ae47 100644
> --- a/support/support_subprocess.c
> +++ b/support/support_subprocess.c
> @@ -92,6 +92,19 @@ support_subprogram (const char *file, char *const argv[])
>    return result;
>  }
>  
> +int
> +support_subprogram_wait (const char *file, char *const argv[])
> +{
> +  posix_spawn_file_actions_t fa;
> +
> +  posix_spawn_file_actions_init (&fa);
> +  struct support_subprocess res = support_subprocess_init ();
> +
> +  res.pid = xposix_spawn (file, &fa, NULL, argv, environ);
> +
> +  return support_process_wait (&res);
> +}

OK. Refactor.

> +
>  int
>  support_process_wait (struct support_subprocess *proc)
>  {
> 


-- 
Cheers,
Carlos.

Re: [PATCH 2/4] tst-env-setuid: Use support_capture_subprogram_self_sgid

[Carlos O'Donell]

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> Use the support_capture_subprogram_self_sgid to spawn an sgid child.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/tst-env-setuid.c | 197 +++----------------------------------------
>  1 file changed, 14 insertions(+), 183 deletions(-)
> 
> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> index 60ae0ca380..49b5e319e2 100644
> --- a/elf/tst-env-setuid.c
> +++ b/elf/tst-env-setuid.c
> @@ -29,173 +29,12 @@
>  #include <sys/wait.h>
>  #include <unistd.h>
>  
> +#include <support/check.h>
>  #include <support/support.h>
>  #include <support/test-driver.h>
> +#include <support/capture_subprocess.h>

OK.

>  
>  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
> @@ -256,40 +95,32 @@ do_test (int argc, char **argv)
>    if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0)
>      {
>        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);
> -	}
> +	/* This can happen if the file system is mounted nosuid.  */
> +	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
> +			  (intmax_t) getgid ());
>  
>        int ret = test_child ();
>  
>        if (ret != 0)
>  	exit (1);
>  
> -      exit (CHILD_STATUS);
> +      exit (EXIT_SUCCESS);
>      }
>    else
>      {
>        if (test_parent () != 0)
>  	exit (1);
>  
> -      /* 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);
> -	}
> +      int status = support_capture_subprogram_self_sgid (SETGID_CHILD);
>  
> -      return run_executable_sgid (target);
> -    }
> +      if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> +	return EXIT_UNSUPPORTED;
> +
> +      if (!WIFEXITED (status))
> +	FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
>  
> -  /* Something went wrong and our argv was corrupted.  */
> -  _exit (1);
> +      return 0;
> +    }

OK. Refactor to use new support_capture_subprogram_self_sgid.

>  }
>  
>  #define TEST_FUNCTION_ARGV do_test
> 


-- 
Cheers,
Carlos.

Re: [PATCH 3/4] Enhance setuid-tunables test

[Carlos O'Donell]

On 3/16/21 3:07 AM, Siddhesh Poyarekar via Libc-alpha wrote:
> Instead of passing GLIBC_TUNABLES via the environment, pass the
> environment variable from parent to child.  This allows us to test
> multiple variables to ensure better coverage.
> 
> The test list currently only includes the case that's already being
> tested.  More tests will be added later.

I like that you turn this into a data-driven test and move the env vars
out of make.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/Makefile                  |  2 -
>  elf/tst-env-setuid-tunables.c | 90 +++++++++++++++++++++++++++--------
>  2 files changed, 69 insertions(+), 23 deletions(-)
> 
> diff --git a/elf/Makefile b/elf/Makefile
> index 3b8e13e066..4e04c26eea 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1653,8 +1653,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

OK.

>  
>  $(objpfx)tst-debug1: $(libdl)
>  $(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 50bef8683d..3d523875b1 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -25,35 +25,50 @@
>  #include "config.h"
>  #undef _LIBC
>  
> -#define test_parent test_parent_tunables
> -#define test_child test_child_tunables
> -
> -static int test_child_tunables (void);
> -static int test_parent_tunables (void);
> -
> -#include "tst-env-setuid.c"
> +#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 <intprops.h>
> +#include <array_length.h>
> +
> +#include <support/check.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
> +#include <support/capture_subprocess.h>
> +
> +const char *teststrings[] =
> +{
> +  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096",

OK.

> +};
>  
> -#define CHILD_VALSTRING_VALUE "glibc.malloc.mmap_threshold=4096"
> -#define PARENT_VALSTRING_VALUE \
> -  "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096"
> +const char *resultstrings[] =
> +{
> +  "glibc.malloc.mmap_threshold=4096",

OK. SXID_IGNORE.

> +};
>  
>  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 +76,48 @@ 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.  */
> +	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
> +			  (intmax_t) getgid ());
>  
> -  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 (EXIT_SUCCESS);
> +    }
> +  else
> +    {
> +      int ret = 0;
> +
> +      /* Spawn tests.  */
> +      for (int i = 0; i < array_length (teststrings); i++)
> +	{
> +	  char buf[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)

OK. Put env vars into test as a data-driven test rather than Make-driven.

> +	    exit (1);
> +
> +	  int status = support_capture_subprogram_self_sgid (buf);
> +
> +	  /* Bail out early if unsupported.  */
> +	  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> +	    return EXIT_UNSUPPORTED;
> +
> +	  ret |= status;
> +	}
> +      return ret;
> +    }
>  }
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> 


-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 3/16/21 3:07 AM, 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.

I have read through bug 27471, thanks for referencing the bug.
 
I'm worried about the fact that we have existing examples from DJ's
nsswitch changes with / changing, that there are real-world examples
where we actively run a child process in a potentially newer glibc
and stripping away tunables for the current hardware means the child
process needs a way to add it back, and doing that isn't entirely
clear e.g. sudo with hardcoded env.

This makes testing and producing tuning difficult or impossible
and I that means to me that we should be more conservative with what
we process.

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

This prevents passing a tunable through a program to a child that runs
with a newer glibc and accepts different tunables.

> - Unrecognized tunables get scrubbed off from the environment and
>   subsequently from the child environment.

I think we need to be conservative here and allow unknown tunables
to be "passed through" to the child without modification.

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

Why is it wrong?

We can have instances where we chroot or unshare into another glibc
version and we may want the env vars to pass through because they are
related to hardware changes.

If I step back and look at the bigger picture I see the following:

(1) Tunables we have removed.
- Did we record these?
- Did we record their security setting?
- Are we avoiding reusing them by accident?
- Are we cleaning legacy tunables based on their security setting to
  keep the environment clean for a child running with an old glibc?

(2) Tunables we don't know about.
- The child process may need to consume these because it runs a newer glibc.
- We lack /var/glibc.tunables handling of these tunables, or the child may be a
  generic container, or generic chroot, maybe env vars are the only way to pass
  this information.

(3) Tunables we know about.
- These are the straight forward fix, we just fix the code.

The patch handles (1) and (2) and (3) but picks an easier to audit
and verify solution for (3) at the expense of (1) and (2). I don't know if
that is a good tradeoff.

With my developer hat on I expect the following:
- All old legacy tunables are handled as their old security settings.
- All new tunables are handled as their current security settings.
- Unknown tunables are left alone.

Have I justified my expectation?

> ---
>  elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
>  elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
>  2 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..1aedb9bd36 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-tunables.c b/elf/tst-env-setuid-tunables.c
> index 3d523875b1..05619c9adc 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -45,11 +45,37 @@
>  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.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> +  "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "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",
> +  "not_valid.malloc.check=2",
> +  "glibc.not_valid.check=2",
>  };
>  
>  const char *resultstrings[] =
>  {
>    "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
>  };
>  
>  static int
> 


-- 
Cheers,
Carlos.

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

[Siddhesh Poyarekar]

On 4/7/21 1:17 AM, Carlos O'Donell wrote:
> On 3/16/21 3:07 AM, 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.
> 
> I have read through bug 27471, thanks for referencing the bug.
>   
> I'm worried about the fact that we have existing examples from DJ's
> nsswitch changes with / changing, that there are real-world examples
> where we actively run a child process in a potentially newer glibc
> and stripping away tunables for the current hardware means the child
> process needs a way to add it back, and doing that isn't entirely
> clear e.g. sudo with hardcoded env.
> 
> This makes testing and producing tuning difficult or impossible
> and I that means to me that we should be more conservative with what
> we process.
> 
>> 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.
> 
> This prevents passing a tunable through a program to a child that runs
> with a newer glibc and accepts different tunables.
> 
>> - Unrecognized tunables get scrubbed off from the environment and
>>    subsequently from the child environment.
> 
> I think we need to be conservative here and allow unknown tunables
> to be "passed through" to the child without modification.
> 
>> - 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.
> 
> Why is it wrong?

That comment was based on a previous consensus[1] where we agreed that 
we eventually port all SXID_IGNORE tunables into SXID_ERASE but I never 
actually got around to doing it.  However you seem to arguing the 
opposite case, i.e. making tunables SXID_IGNORE by default and porting 
SXID_ERASE over to it.

As I've mentioned in the bug report, I don't see miscategorization of 
tunables resulting in any *new* security issues; at worst they may 
provide additional control over processes across setxid boundaries that 
may otherwise not be possible.  In that sense, sxid categorization is a 
security hardening feature and the discussion is about whether the 
hardening is going too far.

> We can have instances where we chroot or unshare into another glibc
> version and we may want the env vars to pass through because they are
> related to hardware changes.
> 
> If I step back and look at the bigger picture I see the following:
> 
> (1) Tunables we have removed.
> - Did we record these?
> - Did we record their security setting?
> - Are we avoiding reusing them by accident?
> - Are we cleaning legacy tunables based on their security setting to
>    keep the environment clean for a child running with an old glibc?

There was one namespace renaming from glibc.tune to glibc.cpu, but it 
was aarch64-specific.  It defaults to SXID_ERASE, as do most tunables. 
Only tunables that were ported from environment variables, e.g. 
glibc.malloc.mmap_threshold are SXID_IGNORE.

> (2) Tunables we don't know about.
> - The child process may need to consume these because it runs a newer glibc.
> - We lack /var/glibc.tunables handling of these tunables, or the child may be a
>    generic container, or generic chroot, maybe env vars are the only way to pass
>    this information.
> 
> (3) Tunables we know about.
> - These are the straight forward fix, we just fix the code.
> 
> The patch handles (1) and (2) and (3) but picks an easier to audit
> and verify solution for (3) at the expense of (1) and (2). I don't know if
> that is a good tradeoff.
> 
> With my developer hat on I expect the following:
> - All old legacy tunables are handled as their old security settings.
> - All new tunables are handled as their current security settings.
> - Unknown tunables are left alone.
> 
> Have I justified my expectation?

To summarize my understanding, you argue that there may be a legitimate 
need to be able to control environment of processes across the sxid 
boundary.  While I agree that there ought to be ways to control 
processes across sxid boundaries I don't think it should be as trivial 
as setting the program environment of an sxid program.

Maybe the use case you're referring to is better served by systemwide 
and userwide tunables.  For example, if I had to control the environment 
of httpd, it should be through $HTTPD_HOME/.glibc-tunables or 
/etc/glibc-tunables and not through $SIDDHESH_HOME/.glibc-tunables or 
the environment of user siddhesh.  This may not work for legacy tunables 
if the older environment doesn't support systemwide or userwide tunables 
but that shouldn't cause problems in practice.

Siddhesh

[1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html

[PATCH v2] support: Add capability to fork an sgid child

[Siddhesh Poyarekar]

Add a new function support_capture_subprogram_self_sgid that spawns an
sgid child of the running program with its own image and returns the
exit code of the child process.  This functionality is used by at
least three tests in the testsuite at the moment, so it makes sense to
consolidate.

There is also a new function support_subprogram_wait which should
provide simple system() like functionality that does not set up file
actions.  This is useful in cases where only the return code of the
spawned subprocess is interesting.

This patch also ports tst-secure-getenv to this new function.  A
subsequent patch will port other tests.  This also brings an important
change to tst-secure-getenv behaviour.  Now instead of succeeding, the
test fails as UNSUPPORTED if it is unable to spawn a setgid child,
which is how it should have been in the first place.
---
Changes from v1L
- Dropped TEST_VERIFY_GOTO in favour of TEST_VERIFY and use
  support_record_failure_is_failed () to jump to err on failure.

 stdlib/tst-secure-getenv.c           | 199 +++------------------------
 support/capture_subprocess.h         |   6 +
 support/subprocess.h                 |   5 +
 support/support_capture_subprocess.c | 126 +++++++++++++++++
 support/support_subprocess.c         |  13 ++
 5 files changed, 168 insertions(+), 181 deletions(-)

diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
index c9ec03866f..5567c9ae21 100644
--- a/stdlib/tst-secure-getenv.c
+++ b/stdlib/tst-secure-getenv.c
@@ -30,167 +30,12 @@
 #include <sys/wait.h>
 #include <unistd.h>
 
+#include <support/check.h>
 #include <support/support.h>
+#include <support/capture_subprocess.h>
 #include <support/test-driver.h>
 
 static char MAGIC_ARGUMENT[] = "run-actual-test";
-#define MAGIC_STATUS 19
-
-/* Return a GID which is not our current GID, but is present in the
-   supplementary group list.  */
-static gid_t
-choose_gid (void)
-{
-  int count = getgroups (0, NULL);
-  if (count < 0)
-    {
-      printf ("getgroups: %m\n");
-      exit (1);
-    }
-  gid_t *groups;
-  groups = xcalloc (count, sizeof (*groups));
-  int ret = getgroups (count, groups);
-  if (ret < 0)
-    {
-      printf ("getgroups: %m\n");
-      exit (1);
-    }
-  gid_t current = getgid ();
-  gid_t not_current = 0;
-  for (int i = 0; i < ret; ++i)
-    {
-      if (groups[i] != current)
-        {
-          not_current = groups[i];
-          break;
-        }
-    }
-  free (groups);
-  return not_current;
-}
-
-
-/* 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/secure-getenv.%jd",
-			     test_dir, (intmax_t) getpid ());
-  char *execname = xasprintf ("%s/bin", dirname);
-  int infd = -1;
-  int outfd = -1;
-  int ret = -1;
-  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;
-    }
-
-  int kid = fork ();
-  if (kid < 0)
-    {
-      printf ("fork: %m\n");
-      goto err;
-    }
-  if (kid == 0)
-    {
-      /* Child process.  */
-      char *args[] = { execname, MAGIC_ARGUMENT, NULL };
-      execve (execname, args, environ);
-      printf ("execve (%s): %m\n", execname);
-      _exit (1);
-    }
-  int status;
-  if (waitpid (kid, &status, 0) < 0)
-    {
-      printf ("waitpid: %m\n");
-      goto err;
-    }
-  if (!WIFEXITED (status) || WEXITSTATUS (status) != MAGIC_STATUS)
-    {
-      printf ("Unexpected exit status %d from child process\n",
-	      status);
-      goto err;
-    }
-  ret = 0;
-
-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;
-}
 
 static int
 do_test (void)
@@ -212,15 +57,15 @@ do_test (void)
       exit (1);
     }
 
-  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);
-    }
-  return run_executable_sgid (target);
+  int status = support_capture_subprogram_self_sgid (MAGIC_ARGUMENT);
+
+  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
+    return EXIT_UNSUPPORTED;
+
+  if (!WIFEXITED (status))
+    FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
+
+  return 0;
 }
 
 static void
@@ -229,23 +74,15 @@ alternative_main (int argc, char **argv)
   if (argc == 2 && strcmp (argv[1], MAGIC_ARGUMENT) == 0)
     {
       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 (MAGIC_STATUS);
-	}
+	/* This can happen if the file system is mounted nosuid.  */
+	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
+		   (intmax_t) getgid ());
       if (getenv ("PATH") == NULL)
-	{
-	  printf ("PATH variable not present\n");
-	  exit (3);
-	}
+	FAIL_EXIT (3, "PATH variable not present\n");
       if (secure_getenv ("PATH") != NULL)
-	{
-	  printf ("PATH variable not filtered out\n");
-	  exit (4);
-	}
-      exit (MAGIC_STATUS);
+	FAIL_EXIT (4, "PATH variable not filtered out\n");
+
+      exit (EXIT_SUCCESS);
     }
 }
 
diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
index 8969d4a99a..4be430f099 100644
--- a/support/capture_subprocess.h
+++ b/support/capture_subprocess.h
@@ -41,6 +41,12 @@ struct support_capture_subprocess support_capture_subprocess
 struct support_capture_subprocess support_capture_subprogram
   (const char *file, char *const argv[]);
 
+/* Copy the running program into a setgid binary and run it with CHILD_ID
+   argument.  If execution is successful, return the exit status of the child
+   program, otherwise return a non-zero failure exit code.  */
+int support_capture_subprogram_self_sgid
+  (char *child_id);
+
 /* Deallocate the subprocess data captured by
    support_capture_subprocess.  */
 void support_capture_subprocess_free (struct support_capture_subprocess *);
diff --git a/support/subprocess.h b/support/subprocess.h
index 11cfc6a07f..40d82c7e4d 100644
--- a/support/subprocess.h
+++ b/support/subprocess.h
@@ -38,6 +38,11 @@ struct support_subprocess support_subprocess
 struct support_subprocess support_subprogram
   (const char *file, char *const argv[]);
 
+/* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
+   to complete.  Return program exit status.  */
+int support_subprogram_wait
+  (const char *file, char *const argv[]);
+
 /* Wait for the subprocess indicated by PROC::PID.  Return the status
    indicate by waitpid call.  */
 int support_process_wait (struct support_subprocess *proc);
diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index 3eb825b9af..27bfd19c93 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -20,11 +20,14 @@
 #include <support/capture_subprocess.h>
 
 #include <errno.h>
+#include <fcntl.h>
 #include <stdlib.h>
 #include <support/check.h>
 #include <support/xunistd.h>
 #include <support/xsocket.h>
 #include <support/xspawn.h>
+#include <support/support.h>
+#include <support/test-driver.h>
 
 static void
 transfer (const char *what, struct pollfd *pfd, struct xmemstream *stream)
@@ -102,6 +105,129 @@ support_capture_subprogram (const char *file, char *const argv[])
   return result;
 }
 
+/* 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
+copy_and_spawn_sgid (char *child_id, gid_t gid)
+{
+  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 = 1, status = 1;
+
+  TEST_VERIFY (mkdir (dirname, 0700) == 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+
+  infd = open ("/proc/self/exe", O_RDONLY);
+  if (infd < 0)
+    FAIL_UNSUPPORTED ("unsupported: Cannot read binary from procfs\n");
+
+  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
+  TEST_VERIFY (outfd >= 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+
+  char buf[4096];
+  for (;;)
+    {
+      ssize_t rdcount = read (infd, buf, sizeof (buf));
+      TEST_VERIFY (rdcount >= 0);
+      if (support_record_failure_is_failed ())
+	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;
+	  TEST_VERIFY (wrcount > 0);
+	  if (support_record_failure_is_failed ())
+	    goto err;
+	  p += wrcount;
+	}
+    }
+  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+  TEST_VERIFY (fchmod (outfd, 02750) == 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+  TEST_VERIFY (close (outfd) == 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+  TEST_VERIFY (close (infd) == 0);
+  if (support_record_failure_is_failed ())
+    goto err;
+
+  /* We have the binary, now spawn the subprocess.  Avoid using
+     support_subprogram because we only want the program exit status, not the
+     contents.  */
+  ret = 0;
+
+  char * const args[] = {execname, child_id, NULL};
+
+  status = support_subprogram_wait (args[0], args);
+
+err:
+  if (outfd >= 0)
+    close (outfd);
+  if (infd >= 0)
+    close (infd);
+  if (execname != NULL)
+    {
+      unlink (execname);
+      free (execname);
+    }
+  if (dirname != NULL)
+    {
+      rmdir (dirname);
+      free (dirname);
+    }
+
+  if (ret != 0)
+    FAIL_EXIT1("Failed to make sgid executable for test\n");
+
+  return status;
+}
+
+int
+support_capture_subprogram_self_sgid (char *child_id)
+{
+  gid_t target = 0;
+  const int count = 64;
+  gid_t groups[count];
+
+  /* Get a GID which is not our current GID, but is present in the
+     supplementary group list.  */
+  int ret = getgroups (count, groups);
+  if (ret < 0)
+    FAIL_UNSUPPORTED("Could not get group list for user %jd\n",
+		     (intmax_t) getuid ());
+
+  gid_t current = getgid ();
+  for (int i = 0; i < ret; ++i)
+    {
+      if (groups[i] != current)
+	{
+	  target = groups[i];
+	  break;
+	}
+    }
+
+  if (target == 0)
+    FAIL_UNSUPPORTED("Could not find a suitable GID for user %jd\n",
+		     (intmax_t) getuid ());
+
+  return copy_and_spawn_sgid (child_id, target);
+}
+
 void
 support_capture_subprocess_free (struct support_capture_subprocess *p)
 {
diff --git a/support/support_subprocess.c b/support/support_subprocess.c
index 2acfc57b7e..89e767ae47 100644
--- a/support/support_subprocess.c
+++ b/support/support_subprocess.c
@@ -92,6 +92,19 @@ support_subprogram (const char *file, char *const argv[])
   return result;
 }
 
+int
+support_subprogram_wait (const char *file, char *const argv[])
+{
+  posix_spawn_file_actions_t fa;
+
+  posix_spawn_file_actions_init (&fa);
+  struct support_subprocess res = support_subprocess_init ();
+
+  res.pid = xposix_spawn (file, &fa, NULL, argv, environ);
+
+  return support_process_wait (&res);
+}
+
 int
 support_process_wait (struct support_subprocess *proc)
 {
-- 
2.29.2

Re: [PATCH v2] support: Add capability to fork an sgid child

[Carlos O'Donell]

On 4/9/21 11:25 AM, Siddhesh Poyarekar wrote:
> Add a new function support_capture_subprogram_self_sgid that spawns an
> sgid child of the running program with its own image and returns the
> exit code of the child process.  This functionality is used by at
> least three tests in the testsuite at the moment, so it makes sense to
> consolidate.
> 
> There is also a new function support_subprogram_wait which should
> provide simple system() like functionality that does not set up file
> actions.  This is useful in cases where only the return code of the
> spawned subprocess is interesting.
> 
> This patch also ports tst-secure-getenv to this new function.  A
> subsequent patch will port other tests.  This also brings an important
> change to tst-secure-getenv behaviour.  Now instead of succeeding, the
> test fails as UNSUPPORTED if it is unable to spawn a setgid child,
> which is how it should have been in the first place.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
> Changes from v1L
> - Dropped TEST_VERIFY_GOTO in favour of TEST_VERIFY and use
>   support_record_failure_is_failed () to jump to err on failure.
> 
>  stdlib/tst-secure-getenv.c           | 199 +++------------------------
>  support/capture_subprocess.h         |   6 +
>  support/subprocess.h                 |   5 +
>  support/support_capture_subprocess.c | 126 +++++++++++++++++
>  support/support_subprocess.c         |  13 ++
>  5 files changed, 168 insertions(+), 181 deletions(-)
> 
> diff --git a/stdlib/tst-secure-getenv.c b/stdlib/tst-secure-getenv.c
> index c9ec03866f..5567c9ae21 100644
> --- a/stdlib/tst-secure-getenv.c
> +++ b/stdlib/tst-secure-getenv.c
> @@ -30,167 +30,12 @@
>  #include <sys/wait.h>
>  #include <unistd.h>
>  
> +#include <support/check.h>
>  #include <support/support.h>
> +#include <support/capture_subprocess.h>
>  #include <support/test-driver.h>
>  
>  static char MAGIC_ARGUMENT[] = "run-actual-test";
> -#define MAGIC_STATUS 19
> -
> -/* Return a GID which is not our current GID, but is present in the
> -   supplementary group list.  */
> -static gid_t
> -choose_gid (void)
> -{
> -  int count = getgroups (0, NULL);
> -  if (count < 0)
> -    {
> -      printf ("getgroups: %m\n");
> -      exit (1);
> -    }
> -  gid_t *groups;
> -  groups = xcalloc (count, sizeof (*groups));
> -  int ret = getgroups (count, groups);
> -  if (ret < 0)
> -    {
> -      printf ("getgroups: %m\n");
> -      exit (1);
> -    }
> -  gid_t current = getgid ();
> -  gid_t not_current = 0;
> -  for (int i = 0; i < ret; ++i)
> -    {
> -      if (groups[i] != current)
> -        {
> -          not_current = groups[i];
> -          break;
> -        }
> -    }
> -  free (groups);
> -  return not_current;
> -}
> -
> -
> -/* 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/secure-getenv.%jd",
> -			     test_dir, (intmax_t) getpid ());
> -  char *execname = xasprintf ("%s/bin", dirname);
> -  int infd = -1;
> -  int outfd = -1;
> -  int ret = -1;
> -  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;
> -    }
> -
> -  int kid = fork ();
> -  if (kid < 0)
> -    {
> -      printf ("fork: %m\n");
> -      goto err;
> -    }
> -  if (kid == 0)
> -    {
> -      /* Child process.  */
> -      char *args[] = { execname, MAGIC_ARGUMENT, NULL };
> -      execve (execname, args, environ);
> -      printf ("execve (%s): %m\n", execname);
> -      _exit (1);
> -    }
> -  int status;
> -  if (waitpid (kid, &status, 0) < 0)
> -    {
> -      printf ("waitpid: %m\n");
> -      goto err;
> -    }
> -  if (!WIFEXITED (status) || WEXITSTATUS (status) != MAGIC_STATUS)
> -    {
> -      printf ("Unexpected exit status %d from child process\n",
> -	      status);
> -      goto err;
> -    }
> -  ret = 0;
> -
> -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;
> -}
>  

OK. Refactor and move out.

>  static int
>  do_test (void)
> @@ -212,15 +57,15 @@ do_test (void)
>        exit (1);
>      }
>  
> -  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);
> -    }
> -  return run_executable_sgid (target);
> +  int status = support_capture_subprogram_self_sgid (MAGIC_ARGUMENT);
> +
> +  if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> +    return EXIT_UNSUPPORTED;
> +
> +  if (!WIFEXITED (status))
> +    FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
> +
> +  return 0;

OK. Use refactored support_capture_subprogram_self_sgid().

>  }
>  
>  static void
> @@ -229,23 +74,15 @@ alternative_main (int argc, char **argv)
>    if (argc == 2 && strcmp (argv[1], MAGIC_ARGUMENT) == 0)
>      {
>        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 (MAGIC_STATUS);
> -	}
> +	/* This can happen if the file system is mounted nosuid.  */
> +	FAIL_UNSUPPORTED ("SGID failed: GID and EGID match (%jd)\n",
> +		   (intmax_t) getgid ());
>        if (getenv ("PATH") == NULL)
> -	{
> -	  printf ("PATH variable not present\n");
> -	  exit (3);
> -	}
> +	FAIL_EXIT (3, "PATH variable not present\n");
>        if (secure_getenv ("PATH") != NULL)
> -	{
> -	  printf ("PATH variable not filtered out\n");
> -	  exit (4);
> -	}
> -      exit (MAGIC_STATUS);
> +	FAIL_EXIT (4, "PATH variable not filtered out\n");
> +
> +      exit (EXIT_SUCCESS);

OK. Cleanup as part of refactor, avoid MAGIC_STATUS use which is moved elsewhere.

>      }
>  }
>  
> diff --git a/support/capture_subprocess.h b/support/capture_subprocess.h
> index 8969d4a99a..4be430f099 100644
> --- a/support/capture_subprocess.h
> +++ b/support/capture_subprocess.h
> @@ -41,6 +41,12 @@ struct support_capture_subprocess support_capture_subprocess
>  struct support_capture_subprocess support_capture_subprogram
>    (const char *file, char *const argv[]);
>  
> +/* Copy the running program into a setgid binary and run it with CHILD_ID
> +   argument.  If execution is successful, return the exit status of the child
> +   program, otherwise return a non-zero failure exit code.  */
> +int support_capture_subprogram_self_sgid
> +  (char *child_id);

OK.

> +
>  /* Deallocate the subprocess data captured by
>     support_capture_subprocess.  */
>  void support_capture_subprocess_free (struct support_capture_subprocess *);
> diff --git a/support/subprocess.h b/support/subprocess.h
> index 11cfc6a07f..40d82c7e4d 100644
> --- a/support/subprocess.h
> +++ b/support/subprocess.h
> @@ -38,6 +38,11 @@ struct support_subprocess support_subprocess
>  struct support_subprocess support_subprogram
>    (const char *file, char *const argv[]);
>  
> +/* Invoke program FILE with ARGV arguments by using posix_spawn and wait for it
> +   to complete.  Return program exit status.  */
> +int support_subprogram_wait
> +  (const char *file, char *const argv[]);
> +
>  /* Wait for the subprocess indicated by PROC::PID.  Return the status
>     indicate by waitpid call.  */
>  int support_process_wait (struct support_subprocess *proc);
> diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
> index 3eb825b9af..27bfd19c93 100644
> --- a/support/support_capture_subprocess.c
> +++ b/support/support_capture_subprocess.c
> @@ -20,11 +20,14 @@
>  #include <support/capture_subprocess.h>
>  
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <stdlib.h>
>  #include <support/check.h>
>  #include <support/xunistd.h>
>  #include <support/xsocket.h>
>  #include <support/xspawn.h>
> +#include <support/support.h>
> +#include <support/test-driver.h>
>  
>  static void
>  transfer (const char *what, struct pollfd *pfd, struct xmemstream *stream)
> @@ -102,6 +105,129 @@ support_capture_subprogram (const char *file, char *const argv[])
>    return result;
>  }
>  
> +/* 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
> +copy_and_spawn_sgid (char *child_id, gid_t gid)
> +{
> +  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 = 1, status = 1;
> +
> +  TEST_VERIFY (mkdir (dirname, 0700) == 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +
> +  infd = open ("/proc/self/exe", O_RDONLY);
> +  if (infd < 0)
> +    FAIL_UNSUPPORTED ("unsupported: Cannot read binary from procfs\n");
> +
> +  outfd = open (execname, O_WRONLY | O_CREAT | O_EXCL, 0700);
> +  TEST_VERIFY (outfd >= 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +
> +  char buf[4096];
> +  for (;;)
> +    {
> +      ssize_t rdcount = read (infd, buf, sizeof (buf));
> +      TEST_VERIFY (rdcount >= 0);
> +      if (support_record_failure_is_failed ())
> +	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;
> +	  TEST_VERIFY (wrcount > 0);
> +	  if (support_record_failure_is_failed ())
> +	    goto err;
> +	  p += wrcount;
> +	}
> +    }
> +  TEST_VERIFY (fchown (outfd, getuid (), gid) == 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +  TEST_VERIFY (fchmod (outfd, 02750) == 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +  TEST_VERIFY (close (outfd) == 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +  TEST_VERIFY (close (infd) == 0);
> +  if (support_record_failure_is_failed ())
> +    goto err;
> +
> +  /* We have the binary, now spawn the subprocess.  Avoid using
> +     support_subprogram because we only want the program exit status, not the
> +     contents.  */
> +  ret = 0;
> +
> +  char * const args[] = {execname, child_id, NULL};
> +
> +  status = support_subprogram_wait (args[0], args);
> +
> +err:
> +  if (outfd >= 0)
> +    close (outfd);
> +  if (infd >= 0)
> +    close (infd);
> +  if (execname != NULL)
> +    {
> +      unlink (execname);
> +      free (execname);
> +    }
> +  if (dirname != NULL)
> +    {
> +      rmdir (dirname);
> +      free (dirname);
> +    }
> +
> +  if (ret != 0)
> +    FAIL_EXIT1("Failed to make sgid executable for test\n");
> +
> +  return status;
> +}
> +
> +int
> +support_capture_subprogram_self_sgid (char *child_id)
> +{
> +  gid_t target = 0;
> +  const int count = 64;
> +  gid_t groups[count];
> +
> +  /* Get a GID which is not our current GID, but is present in the
> +     supplementary group list.  */
> +  int ret = getgroups (count, groups);
> +  if (ret < 0)
> +    FAIL_UNSUPPORTED("Could not get group list for user %jd\n",
> +		     (intmax_t) getuid ());
> +
> +  gid_t current = getgid ();
> +  for (int i = 0; i < ret; ++i)
> +    {
> +      if (groups[i] != current)
> +	{
> +	  target = groups[i];
> +	  break;
> +	}
> +    }
> +
> +  if (target == 0)
> +    FAIL_UNSUPPORTED("Could not find a suitable GID for user %jd\n",
> +		     (intmax_t) getuid ());
> +
> +  return copy_and_spawn_sgid (child_id, target);
> +}

OK.

> +
>  void
>  support_capture_subprocess_free (struct support_capture_subprocess *p)
>  {
> diff --git a/support/support_subprocess.c b/support/support_subprocess.c
> index 2acfc57b7e..89e767ae47 100644
> --- a/support/support_subprocess.c
> +++ b/support/support_subprocess.c
> @@ -92,6 +92,19 @@ support_subprogram (const char *file, char *const argv[])
>    return result;
>  }
>  
> +int
> +support_subprogram_wait (const char *file, char *const argv[])
> +{
> +  posix_spawn_file_actions_t fa;
> +
> +  posix_spawn_file_actions_init (&fa);
> +  struct support_subprocess res = support_subprocess_init ();
> +
> +  res.pid = xposix_spawn (file, &fa, NULL, argv, environ);
> +
> +  return support_process_wait (&res);

OK.

> +}
> +
>  int
>  support_process_wait (struct support_subprocess *proc)
>  {
> 


-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 4/8/21 12:38 AM, Siddhesh Poyarekar wrote:
> On 4/7/21 1:17 AM, Carlos O'Donell wrote:
>> On 3/16/21 3:07 AM, 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.
>>
>> I have read through bug 27471, thanks for referencing the bug.
>>   I'm worried about the fact that we have existing examples from DJ's
>> nsswitch changes with / changing, that there are real-world examples
>> where we actively run a child process in a potentially newer glibc
>> and stripping away tunables for the current hardware means the child
>> process needs a way to add it back, and doing that isn't entirely
>> clear e.g. sudo with hardcoded env.
>>
>> This makes testing and producing tuning difficult or impossible
>> and I that means to me that we should be more conservative with what
>> we process.
>>
>>> 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.
>>
>> This prevents passing a tunable through a program to a child that runs
>> with a newer glibc and accepts different tunables.
>>
>>> - Unrecognized tunables get scrubbed off from the environment and
>>>    subsequently from the child environment.
>>
>> I think we need to be conservative here and allow unknown tunables
>> to be "passed through" to the child without modification.
>>
>>> - 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.
>>
>> Why is it wrong?
> 
> That comment was based on a previous consensus[1] where we agreed
> that we eventually port all SXID_IGNORE tunables into SXID_ERASE but
> I never actually got around to doing it.  However you seem to arguing
> the opposite case, i.e. making tunables SXID_IGNORE by default and
> porting SXID_ERASE over to it.

If a deprecated SXID_IGNORE variable existed, which it doesn't, then
I would have expected it to cross the boundary.

I'm not arguing about defaults just behaviour of existing code and
how it might be used by users.

My argument is that it is not "wrong" for tunables to cross the SXID
boundary if they were marked as being allowed to cross that boundary.

However, I need to review this patch again because:

* By default we set tunables to SXID_ERASE.

* We have no deprecated SXID_IGNORE tunables that I know about.

If we had the latter then we would need to do more work to keep them,
but we don't so your patch might be OK.
 
> As I've mentioned in the bug report, I don't see miscategorization of
> tunables resulting in any *new* security issues; at worst they may
> provide additional control over processes across setxid boundaries
> that may otherwise not be possible.  In that sense, sxid
> categorization is a security hardening feature and the discussion is
> about whether the hardening is going too far.

The hardening, SXID_ERASE, should follow the documented behaviour
for a given variable, including deprecated ones.

>> We can have instances where we chroot or unshare into another glibc
>> version and we may want the env vars to pass through because they are
>> related to hardware changes.
>>
>> If I step back and look at the bigger picture I see the following:
>>
>> (1) Tunables we have removed.
>> - Did we record these?
>> - Did we record their security setting?
>> - Are we avoiding reusing them by accident?
>> - Are we cleaning legacy tunables based on their security setting to
>>    keep the environment clean for a child running with an old glibc?
> 
> There was one namespace renaming from glibc.tune to glibc.cpu, but it
> was aarch64-specific.  It defaults to SXID_ERASE, as do most
> tunables. Only tunables that were ported from environment variables,
> e.g. glibc.malloc.mmap_threshold are SXID_IGNORE.

Right, which is why today your patch should be OK, and I need to redo
the review.

> 
>> (2) Tunables we don't know about.
>> - The child process may need to consume these because it runs a newer glibc.
>> - We lack /var/glibc.tunables handling of these tunables, or the child may be a
>>    generic container, or generic chroot, maybe env vars are the only way to pass
>>    this information.
>>
>> (3) Tunables we know about.
>> - These are the straight forward fix, we just fix the code.
>>
>> The patch handles (1) and (2) and (3) but picks an easier to audit
>> and verify solution for (3) at the expense of (1) and (2). I don't know if
>> that is a good tradeoff.
>>
>> With my developer hat on I expect the following:
>> - All old legacy tunables are handled as their old security settings.
>> - All new tunables are handled as their current security settings.
>> - Unknown tunables are left alone.
>>
>> Have I justified my expectation?
> 
> To summarize my understanding, you argue that there may be a
> legitimate need to be able to control environment of processes across
> the sxid boundary.  While I agree that there ought to be ways to
> control processes across sxid boundaries I don't think it should be
> as trivial as setting the program environment of an sxid program.

I think it should.

Setting values in a file for a container or cross-mount-namespace
requires considerable amount of work.

In k8s and even Red Hat OCP there is *significant* use of environment
variables to express tunables for containers.

Consider a read-only runtime... how do you impact the behaviour? How
do you test a tuning to a hardware feature that only the container
runtime knows about?

In practice I'd like the tuning to be automatic e.g. ifunc, but some
options are difficult and workload specific, and so may need to pass
to a child process across an SXID boundary.

It is *more* dangerous IMO to agree to mount a read-write filesystem
into which you have to write configuration data.

However, I don't think this discussion should or needs to be resolved
today since we have no deprecated SXID_IGNORE env vars that would need
special handling.
 
> Maybe the use case you're referring to is better served by systemwide
> and userwide tunables.  For example, if I had to control the
> environment of httpd, it should be through
> $HTTPD_HOME/.glibc-tunables or /etc/glibc-tunables and not through
> $SIDDHESH_HOME/.glibc-tunables or the environment of user siddhesh.
> This may not work for legacy tunables if the older environment
> doesn't support systemwide or userwide tunables but that shouldn't
> cause problems in practice.

Some tunables I think can be set in that way because you have:

* A known workload tuning.

* A known parameter for your runtime.

But there are much harder parameters to set that depend on the runtime
knowing something more about the host. Today this happens to be
environment variables.

> [1] https://sourceware.org/legacy-ml/libc-alpha/2017-02/msg00042.html
 


-- 
Cheers,
Carlos.

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

[Carlos O'Donell]

On 3/16/21 3:07 AM, 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.

I am reviewing this a second time after our downthread discussion that
there are no SXID_IGNORE tunables that are deprecated and that would
actually need to be handled in a special way. Any deprecated SXID_ERASE
variables would be unknown to a later runtime and be erased anyway
matching the SXID_ERASE requirement.
 
> 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.

Re-review in the context of a limited change to fix the bug.

OK to commit.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/dl-tunables.c             | 56 ++++++++++++++++-------------------
>  elf/tst-env-setuid-tunables.c | 26 ++++++++++++++++
>  2 files changed, 52 insertions(+), 30 deletions(-)
> 
> diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..1aedb9bd36 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;

OK.

>  
>    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;

OK. Terminate the tunable if we are secure.

> +	}
>  
>        /* 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.  */

OK. For today there are no SXID_IGNORE tunables that have been deprecated, and so
all currently deprecated tunables were always SXID_ERASE and so this doesn't
change the current behaviour. Future changes will need to take this into account.

>  	      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];

OK.

>  		    }
>  
>  		  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')

OK.

>  	p += len + 1;
>      }
>  }
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 3d523875b1..05619c9adc 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -45,11 +45,37 @@
>  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.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "glibc.not_valid.check=2:glibc.malloc.mmap_threshold=4096",
> +  "not_valid.malloc.check=2:glibc.malloc.mmap_threshold=4096",
> +  "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",
> +  "not_valid.malloc.check=2",
> +  "glibc.not_valid.check=2",
>  };
>  
>  const char *resultstrings[] =
>  {
>    "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.perturb=0x800:glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "glibc.malloc.mmap_threshold=4096",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",
> +  "",

OK. Adds many more tests, which is great!

>  };
>  
>  static int
> 


-- 
Cheers,
Carlos.