From patchwork Mon Mar 1 14:17:32 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Siddhesh Poyarekar X-Patchwork-Id: 42186 X-Patchwork-Delegate: azanella@linux.vnet.ibm.com Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 5C98738708C0; Mon, 1 Mar 2021 14:17:54 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 5C98738708C0 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1614608274; bh=NVNw4vjNNmVxg0O1u5kNQYkGEX84ZdAvGYM1k/QWzMs=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=dm0S7lDcd2EiSQ0r7PLURv+f5yilgxHYw1Dxu57t5k/ZKAbsEjBTcrfxzWxusFDjU cY3cBH8mqX+9CUQ/ZaQNoYTtOM700/UCW+lU8OUhadZcCR1+F0flUNar+lxaD7MDmF cWGGTmnJzImjM24HCPVuZM6U7kcLB8MkhyqPQgQ4= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from donkey.elm.relay.mailchannels.net (donkey.elm.relay.mailchannels.net [23.83.212.49]) by sourceware.org (Postfix) with ESMTPS id D31B438708C0 for ; Mon, 1 Mar 2021 14:17:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org D31B438708C0 X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from relay.mailchannels.net (localhost [127.0.0.1]) by relay.mailchannels.net (Postfix) with ESMTP id 8399918218A for ; Mon, 1 Mar 2021 14:17:45 +0000 (UTC) Received: from pdx1-sub0-mail-a82.g.dreamhost.com (100-98-118-122.trex.outbound.svc.cluster.local [100.98.118.122]) (Authenticated sender: dreamhost) by relay.mailchannels.net (Postfix) with ESMTPA id E9FDE180167 for ; Mon, 1 Mar 2021 14:17:44 +0000 (UTC) X-Sender-Id: dreamhost|x-authsender|siddhesh@gotplt.org Received: from pdx1-sub0-mail-a82.g.dreamhost.com (pop.dreamhost.com [64.90.62.162]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384) by 100.98.118.122 (trex/6.0.2); Mon, 01 Mar 2021 14:17:45 +0000 X-MC-Relay: Neutral X-MailChannels-SenderId: dreamhost|x-authsender|siddhesh@gotplt.org X-MailChannels-Auth-Id: dreamhost X-Bitter-Left: 5a3a35756745befb_1614608265368_3472276133 X-MC-Loop-Signature: 1614608265368:3509014263 X-MC-Ingress-Time: 1614608265368 Received: from pdx1-sub0-mail-a82.g.dreamhost.com (localhost [127.0.0.1]) by pdx1-sub0-mail-a82.g.dreamhost.com (Postfix) with ESMTP id AA47387E84 for ; Mon, 1 Mar 2021 06:17:44 -0800 (PST) Received: from rhbox.intra.reserved-bit.com (unknown [1.186.101.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) (Authenticated sender: siddhesh@gotplt.org) by pdx1-sub0-mail-a82.g.dreamhost.com (Postfix) with ESMTPSA id D96E97EF93 for ; Mon, 1 Mar 2021 06:17:43 -0800 (PST) X-DH-BACKEND: pdx1-sub0-mail-a82 To: libc-alpha@sourceware.org Subject: [PATCH] Fix SXID_ERASE behavior in setuid programs (BZ #27471) Date: Mon, 1 Mar 2021 19:47:32 +0530 Message-Id: <20210301141732.3433685-1-siddhesh@sourceware.org> X-Mailer: git-send-email 2.29.2 MIME-Version: 1.0 X-Spam-Status: No, score=-3495.1 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, KAM_SHORT, RCVD_IN_BARRACUDACENTRAL, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, 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-Patchwork-Original-From: Siddhesh Poyarekar via Libc-alpha From: Siddhesh Poyarekar Reply-To: Siddhesh Poyarekar Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" 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 + . */ + + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#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 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 -#include -#include -#include -#include -#include -#include -#include -#include - -#include -#include +#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. */