[03/11] elf: Add all malloc tunable to unsecvars
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
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
* 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
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.
* 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
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.
@@ -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;
}
@@ -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" \