From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by sourceware.org (Postfix) with ESMTP id F39A2384403C for ; Tue, 6 Apr 2021 16:35:37 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F39A2384403C Received: from mail-qk1-f198.google.com (mail-qk1-f198.google.com [209.85.222.198]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-409-xe95JrptNBCGVRR5nygioA-1; Tue, 06 Apr 2021 12:35:36 -0400 X-MC-Unique: xe95JrptNBCGVRR5nygioA-1 Received: by mail-qk1-f198.google.com with SMTP id h134so12983727qke.1 for ; Tue, 06 Apr 2021 09:35:36 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:from:organization :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=CZ9WhaiWUGWSbJ8kkprsqGB4AjCIGVHvdKWDONwUwAs=; b=rpnsRxVV2wcpJIrmIwZS3scuyrEQMEUAcTidNywIrpNRyrEFywDPOlNn2GRvSSZwo6 CEuwEkXrJQpD1OQJ2wiuKvtH1ptHAuw1xmr7JcjCLvpNHeWz4P2G4Js7Aa/IJGhAjlql JNibX38ckyHT6uKW7jFSwk3Yvqobqmyv5N7tzKzVh6aMw6WTaHZcUDwAFaJ/q4BAP+vB MjKAR+oXGBSbIwmlopE9IFMZHAQfKfU5EASh5XD34bMUnR4YYJegrghmoNtHg6a8wJBK Db6pysUKtPO/DYPuTSQwW4EN677wkXGoEuc9It7mR4pHWmW87Dz92qENEJIi2TEubE8S wHIQ== X-Gm-Message-State: AOAM530h59EYqkhBk+qiBHbY//gUnruK51rEDMQC2S3Jm7uZ/bavjfbQ QnICM5IY5V/TN1lLKCJ24PFNHAmczgQU44+T1Vvw7QcRUg9mioY8rLuQPuSK1Mk8smb3jCWKtrH YQXQulZxVKU39DtD9hiylZOanmkEj/CPRzz6A7m10/iK5Rkbv3vS1Xh+ju9EAxmSHCJXGNA== X-Received: by 2002:a05:622a:3c8:: with SMTP id k8mr28484292qtx.101.1617726935128; Tue, 06 Apr 2021 09:35:35 -0700 (PDT) X-Google-Smtp-Source: ABdhPJw/kcU3bbzdPDi+FArtQZqxLkv9lhN6hXpf5xkkRR12X/QquDe4rWPVLJ3QW2JCZRdgY8B9Qg== X-Received: by 2002:a05:622a:3c8:: with SMTP id k8mr28484254qtx.101.1617726934726; Tue, 06 Apr 2021 09:35:34 -0700 (PDT) Received: from [192.168.1.16] (198-84-214-74.cpe.teksavvy.com. [198.84.214.74]) by smtp.gmail.com with ESMTPSA id n140sm15929217qka.124.2021.04.06.09.35.33 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 06 Apr 2021 09:35:34 -0700 (PDT) Subject: Re: [PATCH 1/4] support: Add capability to fork an sgid child To: Siddhesh Poyarekar , libc-alpha@sourceware.org References: <20210316070755.330084-1-siddhesh@sourceware.org> <20210316070755.330084-2-siddhesh@sourceware.org> From: Carlos O'Donell Organization: Red Hat Message-ID: Date: Tue, 6 Apr 2021 12:35:32 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20210316070755.330084-2-siddhesh@sourceware.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Apr 2021 16:35:40 -0000 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 > #include > > +#include > #include > +#include > #include > > 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 > > #include > +#include > #include > #include > #include > #include > #include > +#include > +#include > > 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.