| Message ID | 20231017130526.2216827-2-adhemerval.zanella@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve loader environment variable handling | expand |
On 2023-10-17 09:05, Adhemerval Zanella wrote: > 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. > > To restore its support, it would require to add additional information > and parsing to where to find libc_malloc_debug.so. Could you elaborate slightly in the commit message, like so: To restore its support, we would require to add additional information and parsing to locate libc_malloc_debug.so, which is not worth the additional risk it might pose. LGTM. Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org> > > It is one thing less that might change AT_SECURE binaries' behavior > due to environment configurations. > > Checked on x86_64-linux-gnu. > --- > elf/dl-tunables.c | 16 ---------------- > elf/rtld.c | 3 +-- > manual/memory.texi | 4 +--- > manual/tunables.texi | 4 +--- > 4 files changed, 3 insertions(+), 24 deletions(-) > > 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; > -} > - Dropped function. OK. > /* 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 (); > - Drop malloc checking. OK. Current testsuite run should cover any possible regressions from this and it's clean so we're good. > while ((envp = get_next_env (envp, &envname, &len, &envval, > &prev_envp)) != NULL) > { > diff --git a/elf/rtld.c b/elf/rtld.c > index 5107d16fe3..51b6d9f326 100644 > --- a/elf/rtld.c > +++ b/elf/rtld.c > @@ -2670,8 +2670,7 @@ process_envvars (struct dl_main_state *state) > } > while (*nextp != '\0'); > > - if (__access ("/etc/suid-debug", F_OK) != 0) > - GLRO(dl_debug_mask) = 0; > + GLRO(dl_debug_mask) = 0; Unconditionally set dl_debug_mask. OK. > > 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 Manual updates. OK. Thanks, Sid
On 18/10/23 09:31, Siddhesh Poyarekar wrote: > On 2023-10-17 09:05, Adhemerval Zanella wrote: >> 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. >> >> To restore its support, it would require to add additional information >> and parsing to where to find libc_malloc_debug.so. > > Could you elaborate slightly in the commit message, like so: > > To restore its support, we would require to add additional information > and parsing to locate libc_malloc_debug.so, which is not worth the > additional risk it might pose. Ack. > > LGTM. > > Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
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..51b6d9f326 100644 --- a/elf/rtld.c +++ b/elf/rtld.c @@ -2670,8 +2670,7 @@ process_envvars (struct dl_main_state *state) } while (*nextp != '\0'); - if (__access ("/etc/suid-debug", F_OK) != 0) - GLRO(dl_debug_mask) = 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