| Message ID | 20231017130526.2216827-5-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve loader environment variable handling | expand |
On 2023-10-17 09:05, Adhemerval Zanella wrote: > Some environment variables allow alteration of allocator behavior > across setuid boundaries, where a setuid program may ignore the > tunable, but its non-setuid child can read it and adjust the memory > allocator behavior accordingly. > > Most library behavior tunings is limited to the current process and does > not bleed in scope; so it is unclear how pratical this misfeature is. > If behavior change across privilege boundaries is desirable, it would be > better done with a wrapper program around the non-setuid child that sets > these envvars, instead of using the setuid process as the messenger. > > The patch as fixes tst-env-setuid, where it fail if any unsecvars is > set. It also adds a dynamic test, although it requires > --enable-hardcoded-path-in-tests so kernel correctly sets the setuid > bit (using the loader command directly would require to set the > setuid bit on the loader itself, which is not a usual deployment). > > Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org> Likewise, I'll leave this for someone else since I am a co-author. Thanks, Sid > > Checked on x86_64-linux-gnu. > --- > elf/Makefile | 8 +-- > elf/tst-env-setuid-static.c | 1 + > elf/tst-env-setuid.c | 128 +++++++++++++++++++++--------------- > sysdeps/generic/unsecvars.h | 7 ++ > 4 files changed, 86 insertions(+), 58 deletions(-) > create mode 100644 elf/tst-env-setuid-static.c > > 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 \ > @@ -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 > - > $(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so > > $(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so > @@ -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) > 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" > 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. */ > > -#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> > > #include <support/check.h> > @@ -36,61 +32,72 @@ > > static char SETGID_CHILD[] = "setgid-child"; > > -#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; > - } > +#define FILTERED_VALUE "some-filtered-value" > +#define UNFILTERED_VALUE "some-unfiltered-value" > > - if (getenv ("LD_HWCAP_MASK") != NULL) > - { > - printf ("LD_HWCAP_MASK still set\n"); > - return 1; > - } > +struct envvar_t > +{ > + const char *env; > + const char *value; > +}; > > - return 0; > -} > -#endif > +/* 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 }, > +}; > > -#ifndef test_parent > static int > -test_parent (void) > +test_child (void) > { > - if (getenv ("MALLOC_CHECK_") == NULL) > - { > - printf ("MALLOC_CHECK_ lost\n"); > - return 1; > - } > + 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; > } > > - 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); > } > > - return 0; > + return ret; > } > -#endif > > 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"); > + > /* Setgid child process. */ > if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0) > { > @@ -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); > } > 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); > > 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); > + > + if (WEXITSTATUS (status) != 42) > + { > + printf (" child failed with status %d\n", > + WEXITSTATUS (status)); > + support_record_failure (); > + } > > return 0; > } > 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" \
On 2023-10-18 09:04, Siddhesh Poyarekar wrote: > > > On 2023-10-17 09:05, Adhemerval Zanella wrote: >> Some environment variables allow alteration of allocator behavior >> across setuid boundaries, where a setuid program may ignore the >> tunable, but its non-setuid child can read it and adjust the memory >> allocator behavior accordingly. >> >> Most library behavior tunings is limited to the current process and does >> not bleed in scope; so it is unclear how pratical this misfeature is. >> If behavior change across privilege boundaries is desirable, it would be >> better done with a wrapper program around the non-setuid child that sets >> these envvars, instead of using the setuid process as the messenger. >> >> The patch as fixes tst-env-setuid, where it fail if any unsecvars is >> set. It also adds a dynamic test, although it requires >> --enable-hardcoded-path-in-tests so kernel correctly sets the setuid >> bit (using the loader command directly would require to set the >> setuid bit on the loader itself, which is not a usual deployment). >> >> Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > Likewise, I'll leave this for someone else since I am a co-author. Carlos, can you please review this? Thanks, Sid > > Thanks, > Sid > >> >> Checked on x86_64-linux-gnu. >> --- >> elf/Makefile | 8 +-- >> elf/tst-env-setuid-static.c | 1 + >> elf/tst-env-setuid.c | 128 +++++++++++++++++++++--------------- >> sysdeps/generic/unsecvars.h | 7 ++ >> 4 files changed, 86 insertions(+), 58 deletions(-) >> create mode 100644 elf/tst-env-setuid-static.c >> >> 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 \ >> @@ -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 >> - >> $(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so >> $(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so >> @@ -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) >> 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" >> 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. */ >> -#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> >> #include <support/check.h> >> @@ -36,61 +32,72 @@ >> static char SETGID_CHILD[] = "setgid-child"; >> -#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; >> - } >> +#define FILTERED_VALUE "some-filtered-value" >> +#define UNFILTERED_VALUE "some-unfiltered-value" >> - if (getenv ("LD_HWCAP_MASK") != NULL) >> - { >> - printf ("LD_HWCAP_MASK still set\n"); >> - return 1; >> - } >> +struct envvar_t >> +{ >> + const char *env; >> + const char *value; >> +}; >> - return 0; >> -} >> -#endif >> +/* 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 }, >> +}; >> -#ifndef test_parent >> static int >> -test_parent (void) >> +test_child (void) >> { >> - if (getenv ("MALLOC_CHECK_") == NULL) >> - { >> - printf ("MALLOC_CHECK_ lost\n"); >> - return 1; >> - } >> + 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; >> } >> - 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); >> } >> - return 0; >> + return ret; >> } >> -#endif >> 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"); >> + >> /* Setgid child process. */ >> if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0) >> { >> @@ -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); >> } >> 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); >> 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); >> + >> + if (WEXITSTATUS (status) != 42) >> + { >> + printf (" child failed with status %d\n", >> + WEXITSTATUS (status)); >> + support_record_failure (); >> + } >> return 0; >> } >> 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" \ >
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.
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 \ @@ -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 - $(objpfx)tst-debug1.out: $(objpfx)tst-debug1mod1.so $(objpfx)tst-debug1mod1.so: $(objpfx)testobj1.so @@ -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) 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" 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. */ -#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> #include <support/check.h> @@ -36,61 +32,72 @@ static char SETGID_CHILD[] = "setgid-child"; -#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; - } +#define FILTERED_VALUE "some-filtered-value" +#define UNFILTERED_VALUE "some-unfiltered-value" - if (getenv ("LD_HWCAP_MASK") != NULL) - { - printf ("LD_HWCAP_MASK still set\n"); - return 1; - } +struct envvar_t +{ + const char *env; + const char *value; +}; - return 0; -} -#endif +/* 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 }, +}; -#ifndef test_parent static int -test_parent (void) +test_child (void) { - if (getenv ("MALLOC_CHECK_") == NULL) - { - printf ("MALLOC_CHECK_ lost\n"); - return 1; - } + 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; } - 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); } - return 0; + return ret; } -#endif 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"); + /* Setgid child process. */ if (argc == 2 && strcmp (argv[1], SETGID_CHILD) == 0) { @@ -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); } 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); 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); + + if (WEXITSTATUS (status) != 42) + { + printf (" child failed with status %d\n", + WEXITSTATUS (status)); + support_record_failure (); + } return 0; } 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" \