From: DJ Delorie <dj@redhat.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Cc: libc-alpha@sourceware.org, siddhesh@sourceware.org
Subject: Re: [PATCH v2 04/19] elf: Add all malloc tunable to unsecvars
Date: Fri, 27 Oct 2023 23:45:07 -0400 [thread overview]
Message-ID: <xn34xvtnp8.fsf@greed.delorie.com> (raw)
In-Reply-To: <20231017130526.2216827-5-adhemerval.zanella@linaro.org>
LGTM.
Reviewed-by: DJ Delorie <dj@redhat.com>
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> diff --git a/elf/Makefile b/elf/Makefile
> index c824f7dab7..f1cd6e13fa 100644
> --- a/elf/Makefile
> +++ b/elf/Makefile
> @@ -262,7 +262,7 @@ tests-static-normal := \
> tst-array5-static \
> tst-dl-iter-static \
> tst-dst-static \
> - tst-env-setuid \
> + tst-env-setuid-static \
> tst-getauxval-static \
> tst-linkall-static \
> tst-single_threaded-pthread-static \
> @@ -305,6 +305,7 @@ tests := \
> tst-array5 \
> tst-auxv \
> tst-dl-hash \
> + tst-env-setuid \
> tst-leaks1 \
> tst-stringtable \
> tst-tls9 \
Ok.
> @@ -2467,9 +2468,6 @@ $(objpfx)tst-nodelete-dlclose: $(objpfx)tst-nodelete-dlclose-dso.so
> $(objpfx)tst-nodelete-dlclose.out: $(objpfx)tst-nodelete-dlclose-dso.so \
> $(objpfx)tst-nodelete-dlclose-plugin.so
>
> -tst-env-setuid-ENV = MALLOC_CHECK_=2 MALLOC_MMAP_THRESHOLD_=4096 \
> - LD_HWCAP_MASK=0x1
> -
Ok.
> @@ -3021,3 +3019,5 @@ LDFLAGS-tst-dlclose-lazy-mod1.so = -Wl,-z,lazy,--no-as-needed
> $(objpfx)tst-dlclose-lazy-mod1.so: $(objpfx)tst-dlclose-lazy-mod2.so
> $(objpfx)tst-dlclose-lazy.out: \
> $(objpfx)tst-dlclose-lazy-mod1.so $(objpfx)tst-dlclose-lazy-mod2.so
> +
> +tst-env-setuid-ARGS = -- $(host-test-program-cmd)
Ok.
> diff --git a/elf/tst-env-setuid-static.c b/elf/tst-env-setuid-static.c
> new file mode 100644
> index 0000000000..0d88ae88b9
> --- /dev/null
> +++ b/elf/tst-env-setuid-static.c
> @@ -0,0 +1 @@
> +#include "tst-env-setuid.c"
Ok.
> diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
> index 032ab44be2..ba295a6a14 100644
> --- a/elf/tst-env-setuid.c
> +++ b/elf/tst-env-setuid.c
> @@ -15,18 +15,14 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> -/* Verify that tunables correctly filter out unsafe environment variables like
> - MALLOC_CHECK_ and MALLOC_MMAP_THRESHOLD_ but also retain
> - MALLOC_MMAP_THRESHOLD_ in an unprivileged child. */
> +/* Verify that correctly filter out unsafe environment variables defined
> + in unsecvars.h. */
Ok.
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <stdlib.h>
> -#include <stdint.h>
> +#include <array_length.h>
> +#include <gnu/lib-names.h>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <string.h>
> -#include <sys/stat.h>
> -#include <sys/wait.h>
> #include <unistd.h>
Ok.
> #include <support/check.h>
> @@ -36,61 +32,72 @@
>
> static char SETGID_CHILD[] = "setgid-child";
>
> +#define FILTERED_VALUE "some-filtered-value"
> +#define UNFILTERED_VALUE "some-unfiltered-value"
Ok.
> +struct envvar_t
> +{
> + const char *env;
> + const char *value;
> +};
Ok.
> -#ifndef test_child
> -static int
> -test_child (void)
> -{
> - if (getenv ("MALLOC_CHECK_") != NULL)
> - {
> - printf ("MALLOC_CHECK_ is still set\n");
> - return 1;
> - }
> -
> - if (getenv ("MALLOC_MMAP_THRESHOLD_") == NULL)
> - {
> - printf ("MALLOC_MMAP_THRESHOLD_ lost\n");
> - return 1;
> - }
>
> - if (getenv ("LD_HWCAP_MASK") != NULL)
> - {
> - printf ("LD_HWCAP_MASK still set\n");
> - return 1;
> - }
> - return 0;
> -}
> -#endif
Ok.
> +/* That is not an extensible list of all filtered out environment
> + variables. */
> +static const struct envvar_t filtered_envvars[] =
> +{
> + { "GLIBC_TUNABLES", FILTERED_VALUE },
> + { "LD_AUDIT", FILTERED_VALUE },
> + { "LD_HWCAP_MASK", FILTERED_VALUE },
> + { "LD_LIBRARY_PATH", FILTERED_VALUE },
> + { "LD_PRELOAD", FILTERED_VALUE },
> + { "LD_PROFILE", FILTERED_VALUE },
> + { "MALLOC_ARENA_MAX", FILTERED_VALUE },
> + { "MALLOC_PERTURB_", FILTERED_VALUE },
> + { "MALLOC_TRACE", FILTERED_VALUE },
> + { "MALLOC_TRIM_THRESHOLD_", FILTERED_VALUE },
> + { "RES_OPTIONS", FILTERED_VALUE },
> +};
> +
> +static const struct envvar_t unfiltered_envvars[] =
> +{
> + { "LD_BIND_NOW", "0" },
> + { "LD_BIND_NOT", "1" },
> + /* Non longer supported option. */
> + { "LD_ASSUME_KERNEL", UNFILTERED_VALUE },
> +};
Ok.
> -#ifndef test_parent
> static int
> -test_parent (void)
> +test_child (void)
Ok.
> {
> - if (getenv ("MALLOC_CHECK_") == NULL)
> - {
> - printf ("MALLOC_CHECK_ lost\n");
> - return 1;
> - }
Ok.
> + int ret = 0;
>
> - if (getenv ("MALLOC_MMAP_THRESHOLD_") == NULL)
> + for (const struct envvar_t *e = filtered_envvars;
> + e != array_end (filtered_envvars);
> + e++)
> {
> - printf ("MALLOC_MMAP_THRESHOLD_ lost\n");
> - return 1;
> + const char *env = getenv (e->env);
> + ret |= env != NULL;
> }
Ok. Diff is not our friend here ;-)
Checking for vars which should be NOT set.
> - if (getenv ("LD_HWCAP_MASK") == NULL)
> + for (const struct envvar_t *e = unfiltered_envvars;
> + e != array_end (unfiltered_envvars);
> + e++)
> {
> - printf ("LD_HWCAP_MASK lost\n");
> - return 1;
> + const char *env = getenv (e->env);
> + ret |= !(env != NULL && strcmp (env, e->value) == 0);
> }
Ok. Checking for vars which should be set.
> - return 0;
> + return ret;
> }
> -#endif
Ok.
> static int
> do_test (int argc, char **argv)
> {
> + /* For dynamic loader, the test requires --enable-hardcoded-path-in-tests so
> + the kernel sets the AT_SECURE on process initialization. */
> + if (argc >= 2 && strstr (argv[1], LD_SO) != 0)
> + FAIL_UNSUPPORTED ("dynamic test requires --enable-hardcoded-path-in-tests");
> +
Does not test for argc < 2 but this is internal so OK.
> @@ -104,20 +111,33 @@ do_test (int argc, char **argv)
> if (ret != 0)
> exit (1);
>
> - exit (EXIT_SUCCESS);
> + /* Special return code to make sure that the child executed all the way
> + through. */
> + exit (42);
> }
Ok.
> else
> {
> - if (test_parent () != 0)
> - exit (1);
> + for (const struct envvar_t *e = filtered_envvars;
> + e != array_end (filtered_envvars);
> + e++)
> + setenv (e->env, e->value, 1);
> +
> + for (const struct envvar_t *e = unfiltered_envvars;
> + e != array_end (unfiltered_envvars);
> + e++)
> + setenv (e->env, e->value, 1);
Ok.
> int status = support_capture_subprogram_self_sgid (SETGID_CHILD);
>
> if (WEXITSTATUS (status) == EXIT_UNSUPPORTED)
> - return EXIT_UNSUPPORTED;
> -
> - if (!WIFEXITED (status))
> - FAIL_EXIT1 ("Unexpected exit status %d from child process\n", status);
> + exit (EXIT_UNSUPPORTED);
Ok.
> + if (WEXITSTATUS (status) != 42)
> + {
> + printf (" child failed with status %d\n",
> + WEXITSTATUS (status));
> + support_record_failure ();
> + }
Ok.
> return 0;
> }
Ok.
> diff --git a/sysdeps/generic/unsecvars.h b/sysdeps/generic/unsecvars.h
> index 81397fb90b..f7ebed60e5 100644
> --- a/sysdeps/generic/unsecvars.h
> +++ b/sysdeps/generic/unsecvars.h
> @@ -18,7 +18,14 @@
> "LD_SHOW_AUXV\0" \
> "LOCALDOMAIN\0" \
> "LOCPATH\0" \
> + "MALLOC_ARENA_MAX\0" \
> + "MALLOC_ARENA_TEST\0" \
> + "MALLOC_MMAP_MAX_\0" \
> + "MALLOC_MMAP_THRESHOLD_\0" \
> + "MALLOC_PERTURB_\0" \
> + "MALLOC_TOP_PAD_\0" \
> "MALLOC_TRACE\0" \
> + "MALLOC_TRIM_THRESHOLD_\0" \
> "NIS_PATH\0" \
> "NLSPATH\0" \
> "RESOLV_HOST_CONF\0" \
Ok.
next prev parent reply other threads:[~2023-10-28 3:45 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-17 13:05 [PATCH v2 00/19] Improve loader environment variable handling Adhemerval Zanella
2023-10-17 13:05 ` [PATCH v2 01/19] elf: Remove /etc/suid-debug support Adhemerval Zanella
2023-10-18 12:31 ` Siddhesh Poyarekar
2023-10-18 18:27 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 02/19] elf: Add GLIBC_TUNABLES to unsecvars Adhemerval Zanella
2023-10-18 12:52 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 03/19] elf: Ignore GLIBC_TUNABLES for setuid/setgid binaries Adhemerval Zanella
2023-10-18 13:04 ` Siddhesh Poyarekar
2023-10-27 10:38 ` Siddhesh Poyarekar
2023-10-28 2:14 ` DJ Delorie
2023-10-30 16:51 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 04/19] elf: Add all malloc tunable to unsecvars Adhemerval Zanella
2023-10-18 13:04 ` Siddhesh Poyarekar
2023-10-27 10:38 ` Siddhesh Poyarekar
2023-10-28 3:45 ` DJ Delorie [this message]
2023-10-17 13:05 ` [PATCH v2 05/19] elf: Do not process invalid tunable format Adhemerval Zanella
2023-10-18 13:42 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 06/19] elf: Do not parse ill-formatted strings Adhemerval Zanella
2023-10-27 10:25 ` Siddhesh Poyarekar
2023-10-27 12:51 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 07/19] elf: Fix _dl_debug_vdprintf to work before self-relocation Adhemerval Zanella
2023-10-27 10:27 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 08/19] elf: Emit warning if tunable is ill-formatted Adhemerval Zanella
2023-10-27 10:28 ` Siddhesh Poyarekar
2023-10-27 12:51 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 09/19] x86: Use dl-symbol-redir-ifunc.h on cpu-tunables Adhemerval Zanella
2023-10-27 10:32 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 10/19] s390: " Adhemerval Zanella
2023-10-27 10:34 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 11/19] elf: Do not duplicate the GLIBC_TUNABLES string Adhemerval Zanella
2023-10-27 11:54 ` Siddhesh Poyarekar
2023-10-30 18:24 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 12/19] elf: Ignore LD_PROFILE for setuid binaries Adhemerval Zanella
2023-10-27 14:25 ` Siddhesh Poyarekar
2023-12-12 10:11 ` Florian Weimer
2023-10-17 13:05 ` [PATCH v2 13/19] elf: Remove LD_PROFILE for static binaries Adhemerval Zanella
2023-10-17 13:05 ` [PATCH v2 14/19] elf: Ignore loader debug env vars for setuid Adhemerval Zanella
2023-10-17 13:05 ` [PATCH v2 15/19] elf: Remove any_debug from dl_main_state Adhemerval Zanella
2023-10-27 14:26 ` Siddhesh Poyarekar
2023-10-30 19:03 ` Adhemerval Zanella Netto
2023-10-17 13:05 ` [PATCH v2 16/19] elf: Ignore LD_LIBRARY_PATH and debug env var for setuid for static Adhemerval Zanella
2023-10-17 13:05 ` [PATCH v2 17/19] elf: Add comments on how LD_AUDIT and LD_PRELOAD handle __libc_enable_secure Adhemerval Zanella
2023-10-27 14:31 ` Siddhesh Poyarekar
2023-10-17 13:05 ` [PATCH v2 18/19] elf: Ignore LD_BIND_NOW and LD_BIND_NOT for setuid binaries Adhemerval Zanella
2023-10-17 13:05 ` [PATCH v2 19/19] elf: Refactor process_envvars 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=xn34xvtnp8.fsf@greed.delorie.com \
--to=dj@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--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).