[PATCH v2] support: Add capability to fork an sgid child
Carlos O'Donell
carlos@redhat.com
Mon Apr 12 03:15:13 GMT 2021
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.
More information about the Libc-alpha
mailing list