From: Siddhesh Poyarekar <siddhesh@sourceware.org>
To: libc-alpha@sourceware.org,
Adhemerval Zanella <adhemerval.zanella@linaro.org>
Subject: [PING][PATCH] Fix SXID_ERASE behavior in setuid programs (BZ #27471)
Date: Mon, 8 Mar 2021 23:09:42 +0530 [thread overview]
Message-ID: <9f450de9-c261-7d89-001e-c84114184191@sourceware.org> (raw)
In-Reply-To: <20210301141732.3433685-1-siddhesh@sourceware.org>
Ping!
On 3/1/21 7:47 PM, Siddhesh Poyarekar via Libc-alpha wrote:
> 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.
>
> The setuid test for tunables has been bolstered to test different
> combinations of tunable values to ensure that the behaviour is now
> consistent.
> ---
> elf/Makefile | 2 -
> elf/dl-tunables.c | 56 +++++-----
> elf/tst-env-setuid-common.c | 195 ++++++++++++++++++++++++++++++++++
> elf/tst-env-setuid-tunables.c | 96 +++++++++++++----
> elf/tst-env-setuid.c | 182 +------------------------------
> 5 files changed, 298 insertions(+), 233 deletions(-)
> create mode 100644 elf/tst-env-setuid-common.c
>
> diff --git a/elf/Makefile b/elf/Makefile
> index 16c89b6d07..21eed715ee 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -1652,8 +1652,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/dl-tunables.c b/elf/dl-tunables.c
> index a2be9cde2f..f67a09605e 100644
> --- a/elf/dl-tunables.c
> +++ b/elf/dl-tunables.c
> @@ -171,6 +171,7 @@ parse_tunables (char *tunestr, char *valstring)
> return;
>
> char *p = tunestr;
> + size_t off = 0;
>
> while (true)
> {
> @@ -184,7 +185,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. */
> @@ -210,35 +215,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)
> @@ -251,9 +249,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-common.c b/elf/tst-env-setuid-common.c
> new file mode 100644
> index 0000000000..0145599e30
> --- /dev/null
> +++ b/elf/tst-env-setuid-common.c
> @@ -0,0 +1,195 @@
> +/* Common routines for the tst-env-setuid tests.
> +
> + Copyright (C) 2021 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +
> +#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 <support/support.h>
> +#include <support/test-driver.h>
> +
> +#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 *argv1)
> +{
> + 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, argv1, 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;
> +}
> diff --git a/elf/tst-env-setuid-tunables.c b/elf/tst-env-setuid-tunables.c
> index 50bef8683d..0363622026 100644
> --- a/elf/tst-env-setuid-tunables.c
> +++ b/elf/tst-env-setuid-tunables.c
> @@ -25,35 +25,47 @@
> #include "config.h"
> #undef _LIBC
>
> -#define test_parent test_parent_tunables
> -#define test_child test_child_tunables
> +#include "tst-env-setuid-common.c"
>
> -static int test_child_tunables (void);
> -static int test_parent_tunables (void);
> -
> -#include "tst-env-setuid.c"
> -
> -#define CHILD_VALSTRING_VALUE "glibc.malloc.mmap_threshold=4096"
> -#define PARENT_VALSTRING_VALUE \
> - "glibc.malloc.check=2:glibc.malloc.mmap_threshold=4096"
> +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.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",
> +};
> +
> +const char *resultstrings[] =
> +{
> + "glibc.malloc.mmap_threshold=4096",
> + "glibc.malloc.mmap_threshold=4096",
> + "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 +73,57 @@ 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. */
> + fprintf (stderr, "SGID failed: GID and EGID match (%jd)\n",
> + (intmax_t) getgid ());
> + exit (EXIT_UNSUPPORTED);
> + }
>
> - 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 (CHILD_STATUS);
> + }
> + else
> + {
> + int ret = 0;
> +
> + /* 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);
> + }
> +
> + /* Spawn tests. */
> + for (int i = 0; i < sizeof (teststrings) / sizeof (char *); i++)
> + {
> + char buf[8];
> +
> + 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);
> + ret |= run_executable_sgid (target, buf);
> + }
> + return ret;
> + }
> +
> + /* Something went wrong and our argv was corrupted. */
> + _exit (1);
> }
> +
> +#define TEST_FUNCTION_ARGV do_test
> +#include <support/test-driver.c>
> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> index 60ae0ca380..9414d5dfaa 100644
> --- a/elf/tst-env-setuid.c
> +++ b/elf/tst-env-setuid.c
> @@ -19,185 +19,10 @@
> MALLOC_CHECK_ and MALLOC_MMAP_THRESHOLD_ but also retain
> MALLOC_MMAP_THRESHOLD_ in an unprivileged child. */
>
> -#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 <support/support.h>
> -#include <support/test-driver.h>
> +#include "tst-env-setuid-common.c"
>
> 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
> test_child (void)
> {
> @@ -221,9 +46,7 @@ test_child (void)
>
> return 0;
> }
> -#endif
>
> -#ifndef test_parent
> static int
> test_parent (void)
> {
> @@ -247,7 +70,6 @@ test_parent (void)
>
> return 0;
> }
> -#endif
>
> static int
> do_test (int argc, char **argv)
> @@ -285,7 +107,7 @@ do_test (int argc, char **argv)
> exit (0);
> }
>
> - return run_executable_sgid (target);
> + return run_executable_sgid (target, SETGID_CHILD);
> }
>
> /* Something went wrong and our argv was corrupted. */
>
next prev parent reply other threads:[~2021-03-08 17:39 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-01 14:17 [PATCH] " Siddhesh Poyarekar
2021-03-08 17:39 ` Siddhesh Poyarekar [this message]
2021-03-08 20:45 ` Adhemerval Zanella
2021-03-16 6:58 ` Siddhesh Poyarekar
2021-03-16 11:12 ` Adhemerval Zanella
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=9f450de9-c261-7d89-001e-c84114184191@sourceware.org \
--to=siddhesh@sourceware.org \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@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).