| Message ID | 20231010180111.561793-2-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve tunable handling | expand |
* Adhemerval Zanella: > Since malloc debug support moved to a different library > (libc_malloc_debug.so), the glibc.malloc.check requires preloading the > debug library to enable it. It means that suid-debug support has not > been working since 2.34. Theoretically, it would be possible to get this working again using /etc/ld.so.preload. But I'm okay with removing this feature. > diff --git a/elf/rtld.c b/elf/rtld.c > index 5107d16fe3..318a3661f0 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -2670,9 +2670,6 @@ process_envvars (struct dl_main_state *state) > } > while (*nextp != '\0'); > > - if (__access ("/etc/suid-debug", F_OK) != 0) > - GLRO(dl_debug_mask) = 0; > - This change looks wrong. The assignment to GLRO(dl_debug_mask) should remain. Thanks, Florian
On 2023-10-12 04:44, Florian Weimer wrote: > * Adhemerval Zanella: > >> Since malloc debug support moved to a different library >> (libc_malloc_debug.so), the glibc.malloc.check requires preloading the >> debug library to enable it. It means that suid-debug support has not >> been working since 2.34. > > Theoretically, it would be possible to get this working again using > /etc/ld.so.preload. > > But I'm okay with removing this feature. > >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 5107d16fe3..318a3661f0 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -2670,9 +2670,6 @@ process_envvars (struct dl_main_state *state) >> } >> while (*nextp != '\0'); >> >> - if (__access ("/etc/suid-debug", F_OK) != 0) >> - GLRO(dl_debug_mask) = 0; >> - > > This change looks wrong. The assignment to GLRO(dl_debug_mask) should > remain. Or maybe process_dl_debug ought to be fixed to bail out on __libc_enable_secure? Sid
On 2023-10-12 06:43, Siddhesh Poyarekar wrote: > On 2023-10-12 04:44, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> Since malloc debug support moved to a different library >>> (libc_malloc_debug.so), the glibc.malloc.check requires preloading the >>> debug library to enable it. It means that suid-debug support has not >>> been working since 2.34. >> >> Theoretically, it would be possible to get this working again using >> /etc/ld.so.preload. >> >> But I'm okay with removing this feature. >> >>> diff --git a/elf/rtld.c b/elf/rtld.c >>> index 5107d16fe3..318a3661f0 100644 >>> --- a/elf/rtld.c >>> +++ b/elf/rtld.c >>> @@ -2670,9 +2670,6 @@ process_envvars (struct dl_main_state *state) >>> } >>> while (*nextp != '\0'); >>> - if (__access ("/etc/suid-debug", F_OK) != 0) >>> - GLRO(dl_debug_mask) = 0; >>> - >> >> This change looks wrong. The assignment to GLRO(dl_debug_mask) should >> remain. > > Or maybe process_dl_debug ought to be fixed to bail out on > __libc_enable_secure? Also another related exercise could be to look at the various LD_* variables that are processed above and avoid processing those that also figure in unsecvars.h, thus making sure that not only do they get erased, but also get ignored in setuid programs. Thanks, Sid
On 12/10/23 05:44, Florian Weimer wrote: > * Adhemerval Zanella: > >> Since malloc debug support moved to a different library >> (libc_malloc_debug.so), the glibc.malloc.check requires preloading the >> debug library to enable it. It means that suid-debug support has not >> been working since 2.34. > > Theoretically, it would be possible to get this working again using > /etc/ld.so.preload. > > But I'm okay with removing this feature. Indeed, but requiring ld.so.preload setup would end up with a clunky interface (two different files with possible multiple way of failures). I think this interface made sense back when malloc debugging have limit options, specially in some environments. > >> diff --git a/elf/rtld.c b/elf/rtld.c >> index 5107d16fe3..318a3661f0 100644 >> --- a/elf/rtld.c >> +++ b/elf/rtld.c >> @@ -2670,9 +2670,6 @@ process_envvars (struct dl_main_state *state) >> } >> while (*nextp != '\0'); >> >> - if (__access ("/etc/suid-debug", F_OK) != 0) >> - GLRO(dl_debug_mask) = 0; >> - > > This change looks wrong. The assignment to GLRO(dl_debug_mask) should > remain. Right. > Or maybe process_dl_debug ought to be fixed to bail out on __libc_enable_secure? It is an option and I do like this, but for this patchset I think the simple solution would to keep 'GLRO(dl_debug_mask) = 0;' for setuid on process_envvars. I will send another patch that just skip the LD_DEBUG parsing for setuid. > Also another related exercise could be to look at the various LD_* variables that are processed above and avoid processing those that also figure in unsecvars.h, thus making sure that not only do they get erased, but also get ignored in setuid programs. It is on my plan to check on this.
diff --git a/elf/dl-tunables.c b/elf/dl-tunables.c index cae67efa0a..24252af22c 100644 --- a/elf/dl-tunables.c +++ b/elf/dl-tunables.c @@ -252,20 +252,6 @@ parse_tunables (char *tunestr, char *valstring) tunestr[off] = '\0'; } -/* Enable the glibc.malloc.check tunable in SETUID/SETGID programs only when - the system administrator has created the /etc/suid-debug file. This is a - special case where we want to conditionally enable/disable a tunable even - for setuid binaries. We use the special version of access() to avoid - setting ERRNO, which is a TLS variable since TLS has not yet been set - up. */ -static __always_inline void -maybe_enable_malloc_check (void) -{ - tunable_id_t id = TUNABLE_ENUM_NAME (glibc, malloc, check); - if (__libc_enable_secure && __access_noerrno ("/etc/suid-debug", F_OK) == 0) - tunable_list[id].security_level = TUNABLE_SECLEVEL_NONE; -} - /* Initialize the tunables list from the environment. For now we only use the ENV_ALIAS to find values. Later we will also use the tunable names to find values. */ @@ -277,8 +263,6 @@ __tunables_init (char **envp) size_t len = 0; char **prev_envp = envp; - maybe_enable_malloc_check (); - while ((envp = get_next_env (envp, &envname, &len, &envval, &prev_envp)) != NULL) { diff --git a/elf/rtld.c b/elf/rtld.c index 5107d16fe3..318a3661f0 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -2670,9 +2670,6 @@ process_envvars (struct dl_main_state *state) } while (*nextp != '\0'); - if (__access ("/etc/suid-debug", F_OK) != 0) - GLRO(dl_debug_mask) = 0; - if (state->mode != rtld_mode_normal) _exit (5); } diff --git a/manual/memory.texi b/manual/memory.texi index 5781a64f35..258fdbd3a0 100644 --- a/manual/memory.texi +++ b/manual/memory.texi @@ -1379,9 +1379,7 @@ There is one problem with @code{MALLOC_CHECK_}: in SUID or SGID binaries it could possibly be exploited since diverging from the normal programs behavior it now writes something to the standard error descriptor. Therefore the use of @code{MALLOC_CHECK_} is disabled by default for -SUID and SGID binaries. It can be enabled again by the system -administrator by adding a file @file{/etc/suid-debug} (the content is -not important it could be empty). +SUID and SGID binaries. So, what's the difference between using @code{MALLOC_CHECK_} and linking with @samp{-lmcheck}? @code{MALLOC_CHECK_} is orthogonal with respect to diff --git a/manual/tunables.texi b/manual/tunables.texi index 776fd93fd9..347b5698b5 100644 --- a/manual/tunables.texi +++ b/manual/tunables.texi @@ -136,9 +136,7 @@ termination of the process. Like @env{MALLOC_CHECK_}, @code{glibc.malloc.check} has a problem in that it diverges from normal program behavior by writing to @code{stderr}, which could by exploited in SUID and SGID binaries. Therefore, @code{glibc.malloc.check} -is disabled by default for SUID and SGID binaries. This can be enabled again -by the system administrator by adding a file @file{/etc/suid-debug}; the -content of the file could be anything or even empty. +is disabled by default for SUID and SGID binaries. @end deftp @deftp Tunable glibc.malloc.top_pad