public inbox for libc-stable@sourceware.org
 help / color / mirror / Atom feed
* [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child
@ 2021-04-14  5:16 Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 2/6] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable; +Cc: Carlos O'Donell

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.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

(cherry picked from commit 716a3bdc41b2b4b864dc64475015ba51e35e1273)
---
 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 3cfe9a05c3..d4b1139c5e 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 9808750f80..421f657678 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 8b442fd5c0..34ffd02e8e 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 eeed676e3d..c82c8d465b 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 36e3a77af2..52cea8182a 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [COMMITTED 2.32 2/6] tst-env-setuid: Use support_capture_subprogram_self_sgid
  2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
@ 2021-04-14  5:16 ` Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 3/6] Enhance setuid-tunables test Siddhesh Poyarekar
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable; +Cc: Carlos O'Donell

Use the support_capture_subprogram_self_sgid to spawn an sgid child.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

(cherry picked from commit ca335281068a1ed549a75ee64f90a8310755956f)
---
 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 41dc79e83a..2dbccdb69e 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [COMMITTED 2.32 3/6] Enhance setuid-tunables test
  2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 2/6] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
@ 2021-04-14  5:16 ` Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 4/6] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Siddhesh Poyarekar
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable; +Cc: Carlos O'Donell

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.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

(cherry picked from commit 061fe3f8add46a89b7453e87eabb9c4695005ced)
---
 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 355e70037b..3ba7f4ecfc 100644
--- a/elf/Makefile
+++ b/elf/Makefile
@@ -1632,8 +1632,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 971d5892b1..91db3d7555 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [COMMITTED 2.32 4/6] Fix SXID_ERASE behavior in setuid programs (BZ #27471)
  2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 2/6] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 3/6] Enhance setuid-tunables test Siddhesh Poyarekar
@ 2021-04-14  5:16 ` Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 5/6] support: Pass environ to child process Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 6/6] support: Typo and formatting fixes Siddhesh Poyarekar
  4 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable; +Cc: Carlos O'Donell

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.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

(cherry picked from commit 2ed18c5b534d9e92fc006202a5af0df6b72e7aca)
---
 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 26e6e26612..15b29bcb90 100644
--- a/elf/dl-tunables.c
+++ b/elf/dl-tunables.c
@@ -177,6 +177,7 @@ parse_tunables (char *tunestr, char *valstring)
     return;
 
   char *p = tunestr;
+  size_t off = 0;
 
   while (true)
     {
@@ -190,7 +191,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.  */
@@ -216,35 +221,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)
@@ -257,9 +255,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 91db3d7555..ca0c8c245c 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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [COMMITTED 2.32 5/6] support: Pass environ to child process
  2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
                   ` (2 preceding siblings ...)
  2021-04-14  5:16 ` [COMMITTED 2.32 4/6] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Siddhesh Poyarekar
@ 2021-04-14  5:16 ` Siddhesh Poyarekar
  2021-04-14  5:16 ` [COMMITTED 2.32 6/6] support: Typo and formatting fixes Siddhesh Poyarekar
  4 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable

Pass environ to posix_spawn so that the child process can inherit
environment of the test.

(cherry picked from commit e958490f8c74e660bd93c128b3bea746e268f3f6)
---
 support/support_subprocess.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/support/support_subprocess.c b/support/support_subprocess.c
index 52cea8182a..0d007fbb2d 100644
--- a/support/support_subprocess.c
+++ b/support/support_subprocess.c
@@ -84,7 +84,7 @@ support_subprogram (const char *file, char *const argv[])
   xposix_spawn_file_actions_addclose (&fa, result.stdout_pipe[1]);
   xposix_spawn_file_actions_addclose (&fa, result.stderr_pipe[1]);
 
-  result.pid = xposix_spawn (file, &fa, NULL, argv, NULL);
+  result.pid = xposix_spawn (file, &fa, NULL, argv, environ);
 
   xclose (result.stdout_pipe[1]);
   xclose (result.stderr_pipe[1]);
-- 
2.29.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [COMMITTED 2.32 6/6] support: Typo and formatting fixes
  2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
                   ` (3 preceding siblings ...)
  2021-04-14  5:16 ` [COMMITTED 2.32 5/6] support: Pass environ to child process Siddhesh Poyarekar
@ 2021-04-14  5:16 ` Siddhesh Poyarekar
  4 siblings, 0 replies; 6+ messages in thread
From: Siddhesh Poyarekar @ 2021-04-14  5:16 UTC (permalink / raw)
  To: libc-stable

- Add a newline to the end of error messages in transfer().
- Fixed the name of support_subprocess_init().

(cherry picked from commit 95c68080a3ded882789b1629f872c3ad531efda0)
---
 support/support_capture_subprocess.c | 2 +-
 support/support_subprocess.c         | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/support/support_capture_subprocess.c b/support/support_capture_subprocess.c
index c82c8d465b..28a37df67f 100644
--- a/support/support_capture_subprocess.c
+++ b/support/support_capture_subprocess.c
@@ -39,7 +39,7 @@ transfer (const char *what, struct pollfd *pfd, struct xmemstream *stream)
       if (ret < 0)
         {
           support_record_failure ();
-          printf ("error: reading from subprocess %s: %m", what);
+          printf ("error: reading from subprocess %s: %m\n", what);
           pfd->events = 0;
           pfd->revents = 0;
         }
diff --git a/support/support_subprocess.c b/support/support_subprocess.c
index 0d007fbb2d..4a25828111 100644
--- a/support/support_subprocess.c
+++ b/support/support_subprocess.c
@@ -27,7 +27,7 @@
 #include <support/subprocess.h>
 
 static struct support_subprocess
-support_suprocess_init (void)
+support_subprocess_init (void)
 {
   struct support_subprocess result;
 
@@ -48,7 +48,7 @@ support_suprocess_init (void)
 struct support_subprocess
 support_subprocess (void (*callback) (void *), void *closure)
 {
-  struct support_subprocess result = support_suprocess_init ();
+  struct support_subprocess result = support_subprocess_init ();
 
   result.pid = xfork ();
   if (result.pid == 0)
@@ -71,7 +71,7 @@ support_subprocess (void (*callback) (void *), void *closure)
 struct support_subprocess
 support_subprogram (const char *file, char *const argv[])
 {
-  struct support_subprocess result = support_suprocess_init ();
+  struct support_subprocess result = support_subprocess_init ();
 
   posix_spawn_file_actions_t fa;
   /* posix_spawn_file_actions_init does not fail.  */
-- 
2.29.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-04-14  5:16 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-14  5:16 [COMMITTED 2.32 1/6] support: Add capability to fork an sgid child Siddhesh Poyarekar
2021-04-14  5:16 ` [COMMITTED 2.32 2/6] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
2021-04-14  5:16 ` [COMMITTED 2.32 3/6] Enhance setuid-tunables test Siddhesh Poyarekar
2021-04-14  5:16 ` [COMMITTED 2.32 4/6] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Siddhesh Poyarekar
2021-04-14  5:16 ` [COMMITTED 2.32 5/6] support: Pass environ to child process Siddhesh Poyarekar
2021-04-14  5:16 ` [COMMITTED 2.32 6/6] support: Typo and formatting fixes Siddhesh Poyarekar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).