diff mbox series

[01/11] elf: Remove /etc/suid-debug support

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

Commit Message

Adhemerval Zanella Oct. 10, 2023, 6:01 p.m. UTC
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, 2 insertions(+), 25 deletions(-)

Comments

Florian Weimer Oct. 12, 2023, 8:44 a.m. UTC | #1
* 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
Siddhesh Poyarekar Oct. 12, 2023, 10:43 a.m. UTC | #2
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
Siddhesh Poyarekar Oct. 12, 2023, 4:01 p.m. UTC | #3
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
Adhemerval Zanella Oct. 13, 2023, 1:47 p.m. UTC | #4
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 mbox series

Patch

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