[v3,19/19] elf: Refactor process_envvars

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

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");
>   }
>   
>   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.

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,
  

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");
>>   }
>>   
>>   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,