diff mbox series

[v2,04/19] elf: Add all malloc tunable to unsecvars

Message ID 20231017130526.2216827-5-adhemerval.zanella@linaro.org
State Superseded
Headers show
Series Improve loader environment variable handling | expand

Commit Message

Adhemerval Zanella Netto Oct. 17, 2023, 1:05 p.m. UTC 
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>

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

Comments

Siddhesh Poyarekar Oct. 18, 2023, 1:04 p.m. UTC | #1 
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"							      \
Siddhesh Poyarekar Oct. 27, 2023, 10:38 a.m. UTC | #2 
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"                                  \
>
DJ Delorie Oct. 28, 2023, 3:45 a.m. UTC | #3 
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 mbox series

Patch

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"							      \