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

[Carlos O'Donell <carlos@redhat.com>]

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.