[v3] elf: dl-load: Get rid of alloca usage.

  Replace alloca usage with a scratch_buffer.  Change the semantics of
is_trusted_path_normalize to return 1, 0, or -1 on error.  Change
_dl_dst_substitute to return NULL on error and update callers to handle
the NULL return value.
---
Changes to v2:
* Remove 3 other alloca removals as separate patches.
* Use "Memory allocation failure" message rather than a new one.
* Simplify multiple returns into one.
* Only call is_trusted_path_normalize if check_for_trusted is set.
* In expand_dynamic_string_token handle a NULL return from
  _dl_dst_substitute and free malloc'ed memory.
* Print an error message in fillin_rpath when
  expand_dynamic_string_token returns NULL.

 elf/dl-deps.c |  5 +++--
 elf/dl-load.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 41 insertions(+), 14 deletions(-)
  

Ping.

On Thu, Oct 19, 2023 at 01:24:48PM -0400, Joe Simmons-Talbott wrote:
> Replace alloca usage with a scratch_buffer.  Change the semantics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.  Change
> _dl_dst_substitute to return NULL on error and update callers to handle
> the NULL return value.
> ---
> Changes to v2:
> * Remove 3 other alloca removals as separate patches.
> * Use "Memory allocation failure" message rather than a new one.
> * Simplify multiple returns into one.
> * Only call is_trusted_path_normalize if check_for_trusted is set.
> * In expand_dynamic_string_token handle a NULL return from
>   _dl_dst_substitute and free malloc'ed memory.
> * Print an error message in fillin_rpath when
>   expand_dynamic_string_token returns NULL.
> 
>  elf/dl-deps.c |  5 +++--
>  elf/dl-load.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 0549b4a4ff..16077384b6 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -100,8 +100,9 @@ DST not allowed in SUID/SGID programs"));				      \
>  						   __dst_cnt));		      \
>  									      \
>  	__result = _dl_dst_substitute (l, __str, __newp);		      \
> -									      \
> -	if (*__result == '\0')						      \
> +	if (__result == NULL)						      \
> +	  _dl_signal_error (0, __str, NULL, N_("Memory allocation failure")); \
> +	else if (*__result == '\0')					      \
>  	  {								      \
>  	    /* The replacement for the DST is not known.  We can't	      \
>  	       processed.  */						      \
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..1133421fd8 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
>  #include <errno.h>
>  #include <fcntl.h>
>  #include <libintl.h>
> +#include <scratch_buffer.h>
>  #include <stdbool.h>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
>  };
>  #define nsystem_dirs_len array_length (system_dirs_len)
>  
> -static bool
> +static int
>  is_trusted_path_normalize (const char *path, size_t len)
>  {
>    if (len == 0)
> -    return false;
> +    return 0;
> +
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>  
> -  char *npath = (char *) alloca (len + 2);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> +    return -1;
> +
> +  char *npath = sbuf.data;
>    char *wnp = npath;
> +
>    while (*path != '\0')
>      {
>        if (path[0] == '/')
> @@ -167,17 +175,22 @@ is_trusted_path_normalize (const char *path, size_t len)
>  
>    const char *trun = system_dirs;
>  
> +  int r = 0;
>    for (size_t idx = 0; idx < nsystem_dirs_len; ++idx)
>      {
>        if (wnp - npath >= system_dirs_len[idx]
>  	  && memcmp (trun, npath, system_dirs_len[idx]) == 0)
>  	/* Found it.  */
> -	return true;
> +	{
> +          r = 1;
> +	  break;
> +	}
>  
>        trun += system_dirs_len[idx] + 1;
>      }
>  
> -  return false;
> +  scratch_buffer_free (&sbuf);
> +  return r;
>  }
>  
>  /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -270,7 +283,7 @@ _dl_dst_count (const char *input)
>     least equal to the value returned by DL_DST_REQUIRED.  Note that it
>     is possible for a DT_NEEDED, DT_AUXILIARY, and DT_FILTER entries to
>     have colons, but we treat those as literal colons here, not as path
> -   list delimiters.  */
> +   list delimiters.  Returns NULL on failure.  */
>  char *
>  _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>  {
> @@ -362,11 +375,16 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>       trusted to have designed this correctly.  Only $ORIGIN is tested in
>       this way because it may be manipulated in some ways with hard
>       links.  */
> -  if (__glibc_unlikely (check_for_trusted)
> -      && !is_trusted_path_normalize (result, wp - result))
> +  if (__glibc_unlikely (check_for_trusted))
>      {
> -      *result = '\0';
> -      return result;
> +      int r = is_trusted_path_normalize (result, wp - result);
> +      if (r == -1)
> +	return NULL;
> +      else if (r == 0)
> +	{
> +	  *result = '\0';
> +	  return result;
> +	}
>      }
>  
>    *wp = '\0';
> @@ -406,7 +424,11 @@ expand_dynamic_string_token (struct link_map *l, const char *input)
>    if (result == NULL)
>      return NULL;
>  
> -  return _dl_dst_substitute (l, input, result);
> +  char *r = _dl_dst_substitute (l, input, result);
> +  if (r == NULL)
> +    free (result);
> +
> +  return r;
>  }
>  
>  
> @@ -485,7 +507,11 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
>  	  /* expand_dynamic_string_token can return NULL in case of empty
>  	     path or memory allocation failure.  */
>  	  if (cp == NULL)
> -	    continue;
> +	    {
> +	      _dl_signal_error (errno, NULL, NULL,
> +			        N_("empty path or memory allocation failure"));
> +	      continue;
> +	    }
>  
>  	  /* Compute the length after dynamic string token expansion and
>  	     ignore empty paths.  */
> -- 
> 2.39.2
>
  

On 2023-10-19 13:24, Joe Simmons-Talbott wrote:
> Replace alloca usage with a scratch_buffer.  Change the semantics of
> is_trusted_path_normalize to return 1, 0, or -1 on error.  Change
> _dl_dst_substitute to return NULL on error and update callers to handle
> the NULL return value.
> ---
> Changes to v2:
> * Remove 3 other alloca removals as separate patches.
> * Use "Memory allocation failure" message rather than a new one.
> * Simplify multiple returns into one.
> * Only call is_trusted_path_normalize if check_for_trusted is set.
> * In expand_dynamic_string_token handle a NULL return from
>    _dl_dst_substitute and free malloc'ed memory.
> * Print an error message in fillin_rpath when
>    expand_dynamic_string_token returns NULL.
> 
>   elf/dl-deps.c |  5 +++--
>   elf/dl-load.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>   2 files changed, 41 insertions(+), 14 deletions(-)
> 
> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
> index 0549b4a4ff..16077384b6 100644
> --- a/elf/dl-deps.c
> +++ b/elf/dl-deps.c
> @@ -100,8 +100,9 @@ DST not allowed in SUID/SGID programs"));				      \
>   						   __dst_cnt));		      \
>   									      \
>   	__result = _dl_dst_substitute (l, __str, __newp);		      \
> -									      \
> -	if (*__result == '\0')						      \
> +	if (__result == NULL)						      \
> +	  _dl_signal_error (0, __str, NULL, N_("Memory allocation failure")); \
> +	else if (*__result == '\0')					      \
>   	  {								      \
>   	    /* The replacement for the DST is not known.  We can't	      \
>   	       processed.  */						      \
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 2923b1141d..1133421fd8 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -21,6 +21,7 @@
>   #include <errno.h>
>   #include <fcntl.h>
>   #include <libintl.h>
> +#include <scratch_buffer.h>
>   #include <stdbool.h>
>   #include <stdlib.h>
>   #include <string.h>
> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
>   };
>   #define nsystem_dirs_len array_length (system_dirs_len)
>   
> -static bool
> +static int
>   is_trusted_path_normalize (const char *path, size_t len)
>   {
>     if (len == 0)
> -    return false;
> +    return 0;
> +
> +  struct scratch_buffer sbuf;
> +  scratch_buffer_init (&sbuf);
>   
> -  char *npath = (char *) alloca (len + 2);
> +  if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
> +    return -1;

I wonder if we want to use the scratch_buffer abstraction in the dynamic 
loader; we tend to have greater control on allocations in this layer. 
How about, instead, failing if len > PATH_MAX and retaining the alloca? 
the PATH_MAX check should make sure that the passed len is safe for alloca.

Thanks,
Sid

> +
> +  char *npath = sbuf.data;
>     char *wnp = npath;
> +
>     while (*path != '\0')
>       {
>         if (path[0] == '/')
> @@ -167,17 +175,22 @@ is_trusted_path_normalize (const char *path, size_t len)
>   
>     const char *trun = system_dirs;
>   
> +  int r = 0;
>     for (size_t idx = 0; idx < nsystem_dirs_len; ++idx)
>       {
>         if (wnp - npath >= system_dirs_len[idx]
>   	  && memcmp (trun, npath, system_dirs_len[idx]) == 0)
>   	/* Found it.  */
> -	return true;
> +	{
> +          r = 1;
> +	  break;
> +	}
>   
>         trun += system_dirs_len[idx] + 1;
>       }
>   
> -  return false;
> +  scratch_buffer_free (&sbuf);
> +  return r;
>   }
>   
>   /* Given a substring starting at INPUT, just after the DST '$' start
> @@ -270,7 +283,7 @@ _dl_dst_count (const char *input)
>      least equal to the value returned by DL_DST_REQUIRED.  Note that it
>      is possible for a DT_NEEDED, DT_AUXILIARY, and DT_FILTER entries to
>      have colons, but we treat those as literal colons here, not as path
> -   list delimiters.  */
> +   list delimiters.  Returns NULL on failure.  */
>   char *
>   _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>   {
> @@ -362,11 +375,16 @@ _dl_dst_substitute (struct link_map *l, const char *input, char *result)
>        trusted to have designed this correctly.  Only $ORIGIN is tested in
>        this way because it may be manipulated in some ways with hard
>        links.  */
> -  if (__glibc_unlikely (check_for_trusted)
> -      && !is_trusted_path_normalize (result, wp - result))
> +  if (__glibc_unlikely (check_for_trusted))
>       {
> -      *result = '\0';
> -      return result;
> +      int r = is_trusted_path_normalize (result, wp - result);
> +      if (r == -1)
> +	return NULL;
> +      else if (r == 0)
> +	{
> +	  *result = '\0';
> +	  return result;
> +	}
>       }
>   
>     *wp = '\0';
> @@ -406,7 +424,11 @@ expand_dynamic_string_token (struct link_map *l, const char *input)
>     if (result == NULL)
>       return NULL;
>   
> -  return _dl_dst_substitute (l, input, result);
> +  char *r = _dl_dst_substitute (l, input, result);
> +  if (r == NULL)
> +    free (result);
> +
> +  return r;
>   }
>   
>   
> @@ -485,7 +507,11 @@ fillin_rpath (char *rpath, struct r_search_path_elem **result, const char *sep,
>   	  /* expand_dynamic_string_token can return NULL in case of empty
>   	     path or memory allocation failure.  */
>   	  if (cp == NULL)
> -	    continue;
> +	    {
> +	      _dl_signal_error (errno, NULL, NULL,
> +			        N_("empty path or memory allocation failure"));
> +	      continue;
> +	    }
>   
>   	  /* Compute the length after dynamic string token expansion and
>   	     ignore empty paths.  */
  

On 30/10/23 09:56, Siddhesh Poyarekar wrote:
> On 2023-10-19 13:24, Joe Simmons-Talbott wrote:
>> Replace alloca usage with a scratch_buffer.  Change the semantics of
>> is_trusted_path_normalize to return 1, 0, or -1 on error.  Change
>> _dl_dst_substitute to return NULL on error and update callers to handle
>> the NULL return value.
>> ---
>> Changes to v2:
>> * Remove 3 other alloca removals as separate patches.
>> * Use "Memory allocation failure" message rather than a new one.
>> * Simplify multiple returns into one.
>> * Only call is_trusted_path_normalize if check_for_trusted is set.
>> * In expand_dynamic_string_token handle a NULL return from
>>    _dl_dst_substitute and free malloc'ed memory.
>> * Print an error message in fillin_rpath when
>>    expand_dynamic_string_token returns NULL.
>>
>>   elf/dl-deps.c |  5 +++--
>>   elf/dl-load.c | 50 ++++++++++++++++++++++++++++++++++++++------------
>>   2 files changed, 41 insertions(+), 14 deletions(-)
>>
>> diff --git a/elf/dl-deps.c b/elf/dl-deps.c
>> index 0549b4a4ff..16077384b6 100644
>> --- a/elf/dl-deps.c
>> +++ b/elf/dl-deps.c
>> @@ -100,8 +100,9 @@ DST not allowed in SUID/SGID programs"));                      \
>>                              __dst_cnt));              \
>>                                             \
>>       __result = _dl_dst_substitute (l, __str, __newp);              \
>> -                                          \
>> -    if (*__result == '\0')                              \
>> +    if (__result == NULL)                              \
>> +      _dl_signal_error (0, __str, NULL, N_("Memory allocation failure")); \
>> +    else if (*__result == '\0')                          \
>>         {                                      \
>>           /* The replacement for the DST is not known.  We can't          \
>>              processed.  */                              \
>> diff --git a/elf/dl-load.c b/elf/dl-load.c
>> index 2923b1141d..1133421fd8 100644
>> --- a/elf/dl-load.c
>> +++ b/elf/dl-load.c
>> @@ -21,6 +21,7 @@
>>   #include <errno.h>
>>   #include <fcntl.h>
>>   #include <libintl.h>
>> +#include <scratch_buffer.h>
>>   #include <stdbool.h>
>>   #include <stdlib.h>
>>   #include <string.h>
>> @@ -124,14 +125,21 @@ static const size_t system_dirs_len[] =
>>   };
>>   #define nsystem_dirs_len array_length (system_dirs_len)
>>   -static bool
>> +static int
>>   is_trusted_path_normalize (const char *path, size_t len)
>>   {
>>     if (len == 0)
>> -    return false;
>> +    return 0;
>> +
>> +  struct scratch_buffer sbuf;
>> +  scratch_buffer_init (&sbuf);
>>   -  char *npath = (char *) alloca (len + 2);
>> +  if (!scratch_buffer_set_array_size (&sbuf, 1, len + 2))
>> +    return -1;
> 
> I wonder if we want to use the scratch_buffer abstraction in the dynamic loader; we tend to have greater control on allocations in this layer. How about, instead, failing if len > PATH_MAX and retaining the alloca? the PATH_MAX check should make sure that the passed len is safe for alloca.

Another possibility would to add a system specific implementation, where on
linux we always use a PATH_MAX buffer for the normalization and on Hurd
it continues to use alloca.
  

On 2023-10-31 09:14, Adhemerval Zanella Netto wrote:
>> I wonder if we want to use the scratch_buffer abstraction in the dynamic loader; we tend to have greater control on allocations in this layer. How about, instead, failing if len > PATH_MAX and retaining the alloca? the PATH_MAX check should make sure that the passed len is safe for alloca.
> 
> Another possibility would to add a system specific implementation, where on
> linux we always use a PATH_MAX buffer for the normalization and on Hurd
> it continues to use alloca.
> 

Yeah I think that ought to work too.

Thanks,
Sid
  

On Tue, Oct 31, 2023 at 12:47:31PM -0400, Siddhesh Poyarekar wrote:
> On 2023-10-31 09:14, Adhemerval Zanella Netto wrote:
> > > I wonder if we want to use the scratch_buffer abstraction in the dynamic loader; we tend to have greater control on allocations in this layer. How about, instead, failing if len > PATH_MAX and retaining the alloca? the PATH_MAX check should make sure that the passed len is safe for alloca.
> > 
> > Another possibility would to add a system specific implementation, where on
> > linux we always use a PATH_MAX buffer for the normalization and on Hurd
> > it continues to use alloca.
> > 
> 
> Yeah I think that ought to work too.
> 

I'm working on implementing this.  Does anyone have suggestions for how
I can ensure that I'm testing is_trusted_path_normalize?  I ran a 'make
check' but I'm not certain that this code is covered by any existing
tests.

Thanks,
Joe
  

On 02/11/23 12:09, Joe Simmons-Talbott wrote:
> On Tue, Oct 31, 2023 at 12:47:31PM -0400, Siddhesh Poyarekar wrote:
>> On 2023-10-31 09:14, Adhemerval Zanella Netto wrote:
>>>> I wonder if we want to use the scratch_buffer abstraction in the dynamic loader; we tend to have greater control on allocations in this layer. How about, instead, failing if len > PATH_MAX and retaining the alloca? the PATH_MAX check should make sure that the passed len is safe for alloca.
>>>
>>> Another possibility would to add a system specific implementation, where on
>>> linux we always use a PATH_MAX buffer for the normalization and on Hurd
>>> it continues to use alloca.
>>>
>>
>> Yeah I think that ought to work too.
>>
> 
> I'm working on implementing this.  Does anyone have suggestions for how
> I can ensure that I'm testing is_trusted_path_normalize?  I ran a 'make
> check' but I'm not certain that this code is covered by any existing
> tests.

The is_trusted_path_normalize is used on DT_RUNPATH, DT_RPATH, and
LD_LIBRARY_PATH expansion (for the case where the path contains $ORIGIN, 
$LIB, and/or $PLATFORM). It is also used on library loading, including
preloading, to expand the path in case of substitution tokens.

To test fully test the path expansion and check whether loader filters out 
correctly wrong paths, it would require to add some libraries on the default
system path (/lib64 and /usr/lib64) so thus it would require to be a container
test.

The setuid test framework (support_capture_subprogram_self_sgid) also have
some limitations: although it works correctly for statically linked binaries
you will need to use --enable-hardcoded-path-in-tests to correctly instruct
the kernel to setup the AT_SECURE bit for the binary itself.  

With the default test configuration, where loader is issued with the program
as argument, the support_capture_subprogram_self_sgid ends up setting the
setuid bit on the loader itself.  Check a recent patch I did for the tunable 
work on how to handle it [1], unfortunately I ended returning FAIL_UNSUPPORTED
in such case.

Maybe with container tests we can come up with a better way to test setuid, 
by adding the support_capture_subprogram_self_sgid logic to test-container 
itself and instructing it to now issue the loader in such cases.

So I would add some tests to check for is_trusted_path_normalize expansion,
which would also add more coverage for setuid binaries:

  1. Binary with DT_RUNPATH with substitution token.
  2. Binary with DT_RPATH with substitution token.
  3. LD_LIBRARY_PATH with substitution token.
  4. Library with DT_RUNPATH with substitution token pointing, installed on
     a systemdir and resulting path pointing an allowed system path.
  5. Same as 4, but with the library outside the system paths.
  6. Library with DT_RPATH with substitution token pointing, installed on
     a systemdir and resulting path pointing an allowed system path.
  7. Same as 6, but with the library outside the system paths.
  8. LD_PRELOAD with substitution token.

I think these scenarios would give you a nice coverage for setuid binaries.

[1] https://patchwork.sourceware.org/project/glibc/patch/20231017130526.2216827-5-adhemerval.zanella@linaro.org/
  

On Fri, Nov 03, 2023 at 11:30:11AM -0300, Adhemerval Zanella Netto wrote:
> 
> 
> On 02/11/23 12:09, Joe Simmons-Talbott wrote:
> > On Tue, Oct 31, 2023 at 12:47:31PM -0400, Siddhesh Poyarekar wrote:
> >> On 2023-10-31 09:14, Adhemerval Zanella Netto wrote:
> >>>> I wonder if we want to use the scratch_buffer abstraction in the dynamic loader; we tend to have greater control on allocations in this layer. How about, instead, failing if len > PATH_MAX and retaining the alloca? the PATH_MAX check should make sure that the passed len is safe for alloca.
> >>>
> >>> Another possibility would to add a system specific implementation, where on
> >>> linux we always use a PATH_MAX buffer for the normalization and on Hurd
> >>> it continues to use alloca.
> >>>
> >>
> >> Yeah I think that ought to work too.
> >>
> > 
> > I'm working on implementing this.  Does anyone have suggestions for how
> > I can ensure that I'm testing is_trusted_path_normalize?  I ran a 'make
> > check' but I'm not certain that this code is covered by any existing
> > tests.
> 
> The is_trusted_path_normalize is used on DT_RUNPATH, DT_RPATH, and
> LD_LIBRARY_PATH expansion (for the case where the path contains $ORIGIN, 
> $LIB, and/or $PLATFORM). It is also used on library loading, including
> preloading, to expand the path in case of substitution tokens.

Thanks for this information.  I was able to test my changes manually.

> 
> To test fully test the path expansion and check whether loader filters out 
> correctly wrong paths, it would require to add some libraries on the default
> system path (/lib64 and /usr/lib64) so thus it would require to be a container
> test.
> 
> The setuid test framework (support_capture_subprogram_self_sgid) also have
> some limitations: although it works correctly for statically linked binaries
> you will need to use --enable-hardcoded-path-in-tests to correctly instruct
> the kernel to setup the AT_SECURE bit for the binary itself.  

I configured with --enable-hardcoded-path-in-tests and am working to
incorporate the support_capture_subprogram_self_sgid logic into
test-container.c.  I'm seeing fchown fail with EINVAL when trying to
change the test binary's group to 65534 which is the only supplementary
group returned by getgroups.  Do I need to do something special to set
up supplementary groups for the test user within the container?

Thanks,
Joe