From: Carlos O'Donell <carlos@redhat.com>
To: Siddhesh Poyarekar <siddhesh@sourceware.org>, libc-alpha@sourceware.org
Subject: Re: [PATCH 2/4] tst-env-setuid: Use support_capture_subprogram_self_sgid
Date: Tue, 6 Apr 2021 12:39:41 -0400 [thread overview]
Message-ID: <0dae40be-a401-ef2e-499f-563c40dbe86d@redhat.com> (raw)
In-Reply-To: <20210316070755.330084-3-siddhesh@sourceware.org>
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.
next prev parent reply other threads:[~2021-04-06 16:39 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 7:07 [PATCH v2 0/4] tunables and setxid programs Siddhesh Poyarekar
2021-03-16 7:07 ` [PATCH 1/4] support: Add capability to fork an sgid child Siddhesh Poyarekar
2021-04-06 16:35 ` Carlos O'Donell
2021-04-09 15:25 ` [PATCH v2] " Siddhesh Poyarekar
2021-04-12 3:15 ` Carlos O'Donell
2021-03-16 7:07 ` [PATCH 2/4] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
2021-04-06 16:39 ` Carlos O'Donell [this message]
2021-03-16 7:07 ` [PATCH 3/4] Enhance setuid-tunables test Siddhesh Poyarekar
2021-04-06 16:46 ` Carlos O'Donell
2021-03-16 7:07 ` [PATCH 4/4] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Siddhesh Poyarekar
2021-04-06 19:47 ` Carlos O'Donell
2021-04-08 4:38 ` Siddhesh Poyarekar
2021-04-12 3:25 ` Carlos O'Donell
2021-04-12 3:30 ` Carlos O'Donell
2021-03-22 4:32 ` [PING][PATCH v2 0/4] tunables and setxid programs Siddhesh Poyarekar
-- strict thread matches above, loose matches on Subject: below --
2021-03-16 7:06 [PATCH " Siddhesh Poyarekar
2021-03-16 7:06 ` [PATCH 2/4] tst-env-setuid: Use support_capture_subprogram_self_sgid Siddhesh Poyarekar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0dae40be-a401-ef2e-499f-563c40dbe86d@redhat.com \
--to=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
--cc=siddhesh@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).