diff mbox series

[03/11] elf: Add all malloc tunable to unsecvars

Message ID 20231010180111.561793-4-adhemerval.zanella@linaro.org
State Superseded
Headers
Series Improve tunable handling |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed

Commit Message

Adhemerval Zanella Netto Oct. 10, 2023, 6:01 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.

Co-authored-by: Siddhesh Poyarekar <siddhesh@sourceware.org>

Checked on x86_64-linux-gnu.
---
 elf/tst-env-setuid.c        | 87 +++++++++++++------------------------
 sysdeps/generic/unsecvars.h |  7 +++
 2 files changed, 36 insertions(+), 58 deletions(-)

Comments

Florian Weimer Oct. 12, 2023, 8:47 a.m. UTC | #1 
* Adhemerval Zanella:

> @@ -36,57 +31,22 @@
>  
>  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;
> -    }
> +  int ret = 0;
>  
> -  if (getenv ("LD_HWCAP_MASK") != NULL)
> +  const char *nextp = UNSECURE_ENVVARS;
> +  do
>      {
> -      printf ("LD_HWCAP_MASK still set\n");
> -      return 1;
> +      const char *env = getenv (nextp);
> +      ret |= env != NULL;
> +      nextp = strchr (nextp, '\0') + 1;
>      }
> +  while (*nextp != '\0');

I think we should keep some tests that are independent of
UNSECURE_ENVVARS.

Thanks,
Florian
Adhemerval Zanella Netto Oct. 13, 2023, 1:53 p.m. UTC | #2 
On 12/10/23 05:47, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> @@ -36,57 +31,22 @@
>>  
>>  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;
>> -    }
>> +  int ret = 0;
>>  
>> -  if (getenv ("LD_HWCAP_MASK") != NULL)
>> +  const char *nextp = UNSECURE_ENVVARS;
>> +  do
>>      {
>> -      printf ("LD_HWCAP_MASK still set\n");
>> -      return 1;
>> +      const char *env = getenv (nextp);
>> +      ret |= env != NULL;
>> +      nextp = strchr (nextp, '\0') + 1;
>>      }
>> +  while (*nextp != '\0');
> 
> I think we should keep some tests that are independent of
> UNSECURE_ENVVARS.

Not sure what which tests you mean here, my understanding is elf/tst-env-setuid.c
is testing that UNSECURE_ENVVARS is being correctly filtered out.
Florian Weimer Oct. 13, 2023, 2:12 p.m. UTC | #3 
* Adhemerval Zanella Netto:

> On 12/10/23 05:47, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> @@ -36,57 +31,22 @@
>>>  
>>>  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;
>>> -    }
>>> +  int ret = 0;
>>>  
>>> -  if (getenv ("LD_HWCAP_MASK") != NULL)
>>> +  const char *nextp = UNSECURE_ENVVARS;
>>> +  do
>>>      {
>>> -      printf ("LD_HWCAP_MASK still set\n");
>>> -      return 1;
>>> +      const char *env = getenv (nextp);
>>> +      ret |= env != NULL;
>>> +      nextp = strchr (nextp, '\0') + 1;
>>>      }
>>> +  while (*nextp != '\0');
>> 
>> I think we should keep some tests that are independent of
>> UNSECURE_ENVVARS.
>
> Not sure what which tests you mean here, my understanding is
> elf/tst-env-setuid.c is testing that UNSECURE_ENVVARS is being
> correctly filtered out.

I mean that we should test some variables we know should be there in
UNSECURE_ENVVARS, without relying on UNSECURE_ENVVARS to drive the test.

Thanks,
Florian
Adhemerval Zanella Netto Oct. 13, 2023, 2:27 p.m. UTC | #4 
On 13/10/23 11:12, Florian Weimer wrote:
> * Adhemerval Zanella Netto:
> 
>> On 12/10/23 05:47, Florian Weimer wrote:
>>> * Adhemerval Zanella:
>>>
>>>> @@ -36,57 +31,22 @@
>>>>  
>>>>  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;
>>>> -    }
>>>> +  int ret = 0;
>>>>  
>>>> -  if (getenv ("LD_HWCAP_MASK") != NULL)
>>>> +  const char *nextp = UNSECURE_ENVVARS;
>>>> +  do
>>>>      {
>>>> -      printf ("LD_HWCAP_MASK still set\n");
>>>> -      return 1;
>>>> +      const char *env = getenv (nextp);
>>>> +      ret |= env != NULL;
>>>> +      nextp = strchr (nextp, '\0') + 1;
>>>>      }
>>>> +  while (*nextp != '\0');
>>>
>>> I think we should keep some tests that are independent of
>>> UNSECURE_ENVVARS.
>>
>> Not sure what which tests you mean here, my understanding is
>> elf/tst-env-setuid.c is testing that UNSECURE_ENVVARS is being
>> correctly filtered out.
> 
> I mean that we should test some variables we know should be there in
> UNSECURE_ENVVARS, without relying on UNSECURE_ENVVARS to drive the test.

Alright, I will extend the test.
diff mbox series

Patch

diff --git a/elf/tst-env-setuid.c b/elf/tst-env-setuid.c
index 032ab44be2..b9f4b3244d 100644
--- a/elf/tst-env-setuid.c
+++ b/elf/tst-env-setuid.c
@@ -15,19 +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
+   by unsecvars.h.  */
 
-#include <errno.h>
-#include <fcntl.h>
-#include <stdlib.h>
-#include <stdint.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <string.h>
-#include <sys/stat.h>
-#include <sys/wait.h>
 #include <unistd.h>
+#include <unsecvars.h>
 
 #include <support/check.h>
 #include <support/support.h>
@@ -36,57 +31,22 @@ 
 
 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;
-    }
+  int ret = 0;
 
-  if (getenv ("LD_HWCAP_MASK") != NULL)
+  const char *nextp = UNSECURE_ENVVARS;
+  do
     {
-      printf ("LD_HWCAP_MASK still set\n");
-      return 1;
+      const char *env = getenv (nextp);
+      ret |= env != NULL;
+      nextp = strchr (nextp, '\0') + 1;
     }
+  while (*nextp != '\0');
 
   return 0;
 }
-#endif
-
-#ifndef test_parent
-static int
-test_parent (void)
-{
-  if (getenv ("MALLOC_CHECK_") == NULL)
-    {
-      printf ("MALLOC_CHECK_ lost\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 lost\n");
-      return 1;
-    }
-
-  return 0;
-}
-#endif
 
 static int
 do_test (int argc, char **argv)
@@ -104,20 +64,31 @@  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);
+      const char *nextp = UNSECURE_ENVVARS;
+      do
+	{
+	  setenv (nextp, "some-value", 1);
+	  nextp = strchr (nextp, '\0') + 1;
+	}
+      while (*nextp != '\0');
 
       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 8278c50a84..ca70e2e989 100644
--- a/sysdeps/generic/unsecvars.h
+++ b/sysdeps/generic/unsecvars.h
@@ -17,7 +17,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"							      \