[v2,01/19] elf: Remove /etc/suid-debug support

  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.

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(-)
  

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>