Re: [PATCH v3 19/19] elf: Refactor process_envvars

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 20/11/23 20:09, Siddhesh Poyarekar wrote:
> 
> 
> On 2023-11-06 15:25, Adhemerval Zanella wrote:
>> It splits between process_envvars_secure and process_envvars_default,
>> with the former used to process arguments for __libc_enable_secure.
>> It does not have any semantic change, just simplify the code so there
>> is no need to handle __libc_enable_secure on each len switch.
>>
>> Checked on x86_64-linux-gnu and aarch64-linux-gnu.
>> ---
>>   elf/rtld.c | 132 ++++++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 84 insertions(+), 48 deletions(-)
>>
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index cfba30eba0..95dcd32185 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -2527,7 +2527,67 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n");
>>   }
>>   \f
>>   static void
>> -process_envvars (struct dl_main_state *state)
>> +process_envvars_secure (struct dl_main_state *state)
>> +{
>> +  char **runp = _environ;
>> +  char *envline;
>> +
>> +  while ((envline = _dl_next_ld_env_entry (&runp)) != NULL)
>> +    {
>> +      size_t len = 0;
>> +
>> +      while (envline[len] != '\0' && envline[len] != '=')
>> +    ++len;
>> +
>> +      if (envline[len] != '=')
>> +    /* This is a "LD_" variable at the end of the string without
>> +       a '=' character.  Ignore it since otherwise we will access
>> +       invalid memory below.  */
>> +    continue;
>> +
>> +      switch (len)
>> +    {
>> +    case 5:
>> +      /* For __libc_enable_secure mode, audit pathnames containing slashes
>> +         are ignored.  Also, shared audit objects are only loaded only from
>> +         the standard search directories and only if they have set-user-ID
>> +         mode bit enabled.  */
>> +      if (memcmp (envline, "AUDIT", 5) == 0)
>> +        audit_list_add_string (&state->audit_list, &envline[6]);
>> +      break;
>> +
>> +    case 7:
>> +      /* For __libc_enable_secure mode, preload pathnames containing slashes
>> +         are ignored.  Also, shared objects are only preloaded from the
>> +         standard search directories and only if they have set-user-ID mode
>> +         bit enabled.  */
>> +      if (memcmp (envline, "PRELOAD", 7) == 0)
>> +        state->preloadlist = &envline[8];
>> +      break;
>> +    }
>> +    }
> 
> We need to talk about whether we should get rid of these too in the context of setuid programs, but that's a separate discussion.  LGTM.

Indeed, it is not clear to me the utility of LD_AUDIT and LD_PRELOAD for setuid
binarie.  It requires some specific setup (library in default search list, with
setuid bit set), LD_AUDIT is essentially a debug setup (although some setups
really stretch its original design), and LD_PRELOADS might change the setuid
semantic depending of which library is used.

> 
> Reviewed-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> 
>> +
>> +  /* Extra security for SUID binaries.  Remove all dangerous environment
>> +     variables.  */
>> +  const char *nextp = UNSECURE_ENVVARS;
>> +  do
>> +    {
>> +      unsetenv (nextp);
>> +      nextp = strchr (nextp, '\0') + 1;
>> +    }
>> +  while (*nextp != '\0');
>> +
>> +  if (GLRO(dl_debug_mask) != 0
>> +      || GLRO(dl_verbose) != 0
>> +      || GLRO(dl_lazy) != 1
>> +      || GLRO(dl_bind_not) != 0
>> +      || state->mode != rtld_mode_normal
>> +      || state->version_info)
>> +    _exit (5);
>> +}
>> +
>> +static void
>> +process_envvars_default (struct dl_main_state *state)
>>   {
>>     char **runp = _environ;
>>     char *envline;
> 
> I wondered if we should add an assert here on __libc_enable_secure ==0, but there is only one caller, so maybe we can get away without it.
> 
>> @@ -2550,15 +2610,13 @@ process_envvars (struct dl_main_state *state)
>>       {
>>       case 4:
>>         /* Warning level, verbose or not.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "WARN", 4) == 0)
>> +      if (memcmp (envline, "WARN", 4) == 0)
>>           GLRO(dl_verbose) = envline[5] != '\0';
>>         break;
>>         case 5:
>>         /* Debugging of the dynamic linker?  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "DEBUG", 5) == 0)
>> +      if (memcmp (envline, "DEBUG", 5) == 0)
>>           {
>>             process_dl_debug (state, &envline[6]);
>>             break;
>> @@ -2573,8 +2631,7 @@ process_envvars (struct dl_main_state *state)
>>         case 7:
>>         /* Print information about versions.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "VERBOSE", 7) == 0)
>> +      if (memcmp (envline, "VERBOSE", 7) == 0)
>>           {
>>             state->version_info = envline[8] != '\0';
>>             break;
>> @@ -2591,43 +2648,37 @@ process_envvars (struct dl_main_state *state)
>>           }
>>           /* Which shared object shall be profiled.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0')
>> +      if (memcmp (envline, "PROFILE", 7) == 0 && envline[8] != '\0')
>>           GLRO(dl_profile) = &envline[8];
>>         break;
>>         case 8:
>>         /* Do we bind early?  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "BIND_NOW", 8) == 0)
>> +      if (memcmp (envline, "BIND_NOW", 8) == 0)
>>           {
>>             GLRO(dl_lazy) = envline[9] == '\0';
>>             break;
>>           }
>> -      if (! __libc_enable_secure
>> -          && memcmp (envline, "BIND_NOT", 8) == 0)
>> +      if (memcmp (envline, "BIND_NOT", 8) == 0)
>>           GLRO(dl_bind_not) = envline[9] != '\0';
>>         break;
>>         case 9:
>>         /* Test whether we want to see the content of the auxiliary
>>            array passed up from the kernel.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "SHOW_AUXV", 9) == 0)
>> +      if (memcmp (envline, "SHOW_AUXV", 9) == 0)
>>           _dl_show_auxv ();
>>         break;
>>         case 11:
>>         /* Path where the binary is found.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "ORIGIN_PATH", 11) == 0)
>> +      if (memcmp (envline, "ORIGIN_PATH", 11) == 0)
>>           GLRO(dl_origin_path) = &envline[12];
>>         break;
>>         case 12:
>>         /* The library search path.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "LIBRARY_PATH", 12) == 0)
>> +      if (memcmp (envline, "LIBRARY_PATH", 12) == 0)
>>           {
>>             state->library_path = &envline[13];
>>             state->library_path_source = "LD_LIBRARY_PATH";
>> @@ -2635,30 +2686,26 @@ process_envvars (struct dl_main_state *state)
>>           }
>>           /* Where to place the profiling data file.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "DEBUG_OUTPUT", 12) == 0)
>> +      if (memcmp (envline, "DEBUG_OUTPUT", 12) == 0)
>>           {
>>             debug_output = &envline[13];
>>             break;
>>           }
>>   -      if (!__libc_enable_secure
>> -          && memcmp (envline, "DYNAMIC_WEAK", 12) == 0)
>> +      if (memcmp (envline, "DYNAMIC_WEAK", 12) == 0)
>>           GLRO(dl_dynamic_weak) = 1;
>>         break;
>>         case 14:
>>         /* Where to place the profiling data file.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "PROFILE_OUTPUT", 14) == 0
>> +      if (memcmp (envline, "PROFILE_OUTPUT", 14) == 0
>>             && envline[15] != '\0')
>>           GLRO(dl_profile_output) = &envline[15];
>>         break;
>>         case 20:
>>         /* The mode of the dynamic linker can be set.  */
>> -      if (!__libc_enable_secure
>> -          && memcmp (envline, "TRACE_LOADED_OBJECTS", 20) == 0)
>> +      if (memcmp (envline, "TRACE_LOADED_OBJECTS", 20) == 0)
>>           {
>>             state->mode = rtld_mode_trace;
>>             state->mode_trace_program
>> @@ -2668,30 +2715,10 @@ process_envvars (struct dl_main_state *state)
>>       }
>>       }
>>   -  /* Extra security for SUID binaries.  Remove all dangerous environment
>> -     variables.  */
>> -  if (__glibc_unlikely (__libc_enable_secure))
>> -    {
>> -      const char *nextp = UNSECURE_ENVVARS;
>> -      do
>> -    {
>> -      unsetenv (nextp);
>> -      nextp = strchr (nextp, '\0') + 1;
>> -    }
>> -      while (*nextp != '\0');
>> -
>> -      if (GLRO(dl_debug_mask) != 0
>> -      || GLRO(dl_verbose) != 0
>> -      || GLRO(dl_lazy) != 1
>> -      || GLRO(dl_bind_not) != 0
>> -      || state->mode != rtld_mode_normal
>> -      || state->version_info)
>> -    _exit (5);
>> -    }
>>     /* If we have to run the dynamic linker in debugging mode and the
>>        LD_DEBUG_OUTPUT environment variable is given, we write the debug
>>        messages to this file.  */
>> -  else if (GLRO(dl_debug_mask) != 0 && debug_output != NULL)
>> +  if (GLRO(dl_debug_mask) != 0 && debug_output != NULL)
>>       {
>>         const int flags = O_WRONLY | O_APPEND | O_CREAT | O_NOFOLLOW;
>>         size_t name_len = strlen (debug_output);
>> @@ -2710,6 +2737,15 @@ process_envvars (struct dl_main_state *state)
>>       }
>>   }
>>   +static void
>> +process_envvars (struct dl_main_state *state)
>> +{
>> +  if (__glibc_unlikely (__libc_enable_secure))
>> +    process_envvars_secure (state);
>> +  else
>> +    process_envvars_default (state);
>> +}
>> +
>>   #if HP_TIMING_INLINE
>>   static void
>>   print_statistics_item (const char *title, hp_timing_t time,