[2/3] elf: Simplify handling of lists of audit strings

  All list elements are colon-separated strings, and there is a hard
upper limit for the number of audit modules, so it is possible to
pre-allocate a fixed-size array of strings to which the LD_AUDIT
environment variable and --audit arguments are added.

Also eliminate the global variables for the audit list because
the list is only needed briefly during startup.

There is a slight behavior change: All duplicate LD_AUDIT environment
variables are now processed, not just the last one as before.  However,
such environment vectors are invalid anyway.
---
 elf/rtld.c | 230 ++++++++++++++++++++++++++---------------------------
 1 file changed, 113 insertions(+), 117 deletions(-)
  

On 4/3/20 8:02 AM, Florian Weimer via Libc-alpha wrote:
> All list elements are colon-separated strings, and there is a hard
> upper limit for the number of audit modules, so it is possible to
> pre-allocate a fixed-size array of strings to which the LD_AUDIT
> environment variable and --audit arguments are added.
> 
> Also eliminate the global variables for the audit list because
> the list is only needed briefly during startup.
> 
> There is a slight behavior change: All duplicate LD_AUDIT environment
> variables are now processed, not just the last one as before.  However,
> such environment vectors are invalid anyway.

Agreed.

OK for master.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  elf/rtld.c | 230 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 113 insertions(+), 117 deletions(-)
> 
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 51dfaf966a..fe4dfbec67 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -45,6 +45,7 @@
>  #include <stap-probe.h>
>  #include <stackinfo.h>
>  #include <not-cancel.h>
> +#include <array_length.h>
>  
>  #include <assert.h>
>  
> @@ -109,8 +110,53 @@ static void print_missing_version (int errcode, const char *objname,
>  /* Print the various times we collected.  */
>  static void print_statistics (const hp_timing_t *total_timep);
>  
> -/* Add audit objects.  */
> -static void process_dl_audit (char *str);
> +/* Length limits for names and paths, to protect the dynamic linker,
> +   particularly when __libc_enable_secure is active.  */
> +#ifdef NAME_MAX
> +# define SECURE_NAME_LIMIT NAME_MAX
> +#else
> +# define SECURE_NAME_LIMIT 255
> +#endif
> +#ifdef PATH_MAX
> +# define SECURE_PATH_LIMIT PATH_MAX
> +#else
> +# define SECURE_PATH_LIMIT 1024
> +#endif

OK.

> +
> +/* Strings containing colon-separated lists of audit modules.  */
> +struct audit_list
> +{
> +  /* Array of strings containing colon-separated path lists.  Each
> +     audit module needs its own namespace, so pre-allocate the largest
> +     possible list.  */
> +  const char *audit_strings[DL_NNS];
> +
> +  /* Number of entries added to audit_strings.  */
> +  size_t length;
> +
> +  /* Index into the audit_strings array (for the iteration phase).  */
> +  size_t current_index;
> +
> +  /* Tail of audit_strings[current_index] which still needs
> +     processing.  */
> +  const char *current_tail;
> +
> +  /* Scratch buffer for returning a name which is part of the strings
> +     in audit_strings.  */
> +  char fname[SECURE_NAME_LIMIT];
> +};
> +
> +/* Creates an empty audit list.  */
> +static void audit_list_init (struct audit_list *);
> +
> +/* Add a string to the end of the audit list, for later parsing.  Must
> +   not be called after audit_list_next.  */
> +static void audit_list_add_string (struct audit_list *, const char *);
> +
> +/* Extract the next audit module from the audit list.  Only modules
> +   for which dso_name_valid_for_suid is true are returned.  Must be
> +   called after all the audit_list_add_string calls.  */
> +static const char *audit_list_next (struct audit_list *);
>  
>  /* This is a list of all the modes the dynamic loader can be in.  */
>  enum mode { normal, list, verify, trace };
> @@ -118,7 +164,7 @@ enum mode { normal, list, verify, trace };
>  /* Process all environments variables the dynamic linker must recognize.
>     Since all of them start with `LD_' we are a bit smarter while finding
>     all the entries.  */
> -static void process_envvars (enum mode *modep);
> +static void process_envvars (enum mode *modep, struct audit_list *);
>  
>  #ifdef DL_ARGV_NOT_RELRO
>  int _dl_argc attribute_hidden;
> @@ -146,19 +192,6 @@ uintptr_t __pointer_chk_guard_local
>  strong_alias (__pointer_chk_guard_local, __pointer_chk_guard)
>  #endif
>  
> -/* Length limits for names and paths, to protect the dynamic linker,
> -   particularly when __libc_enable_secure is active.  */
> -#ifdef NAME_MAX
> -# define SECURE_NAME_LIMIT NAME_MAX
> -#else
> -# define SECURE_NAME_LIMIT 255
> -#endif
> -#ifdef PATH_MAX
> -# define SECURE_PATH_LIMIT PATH_MAX
> -#else
> -# define SECURE_PATH_LIMIT 1024
> -#endif
> -
>  /* Check that AT_SECURE=0, or that the passed name does not contain
>     directories and is not overly long.  Reject empty names
>     unconditionally.  */
> @@ -176,89 +209,76 @@ dso_name_valid_for_suid (const char *p)
>    return *p != '\0';
>  }
>  
> -/* LD_AUDIT variable contents.  Must be processed before the
> -   audit_list below.  */
> -const char *audit_list_string;
> -
> -/* Cyclic list of auditing DSOs.  audit_list->next is the first
> -   element.  */
> -static struct audit_list
> +static void
> +audit_list_init (struct audit_list *list)
>  {
> -  const char *name;
> -  struct audit_list *next;
> -} *audit_list;
> +  list->length = 0;
> +  list->current_index = 0;
> +  list->current_tail = NULL;
> +}

OK.

>  
> -/* Iterator for audit_list_string followed by audit_list.  */
> -struct audit_list_iter
> +static void
> +audit_list_add_string (struct audit_list *list, const char *string)
>  {
> -  /* Tail of audit_list_string still needing processing, or NULL.  */
> -  const char *audit_list_tail;
> +  /* Empty strings do not load anything.  */
> +  if (*string == '\0')
> +    return;
>  
> -  /* The list element returned in the previous iteration.  NULL before
> -     the first element.  */
> -  struct audit_list *previous;
> +  /* Grow the array if necessary.  */
> +  if (list->length == array_length (list->audit_strings))
> +    _dl_fatal_printf ("Too many audit modules requested\n");
>  
> -  /* Scratch buffer for returning a name which is part of
> -     audit_list_string.  */
> -  char fname[SECURE_NAME_LIMIT];
> -};
> +  list->audit_strings[list->length++] = string;
>  
> -/* Initialize an audit list iterator.  */
> -static void
> -audit_list_iter_init (struct audit_list_iter *iter)
> -{
> -  iter->audit_list_tail = audit_list_string;
> -  iter->previous = NULL;
> +  /* Initialize processing of the first string for
> +     audit_list_next.  */
> +  if (list->length == 1)
> +    list->current_tail = string;
>  }
>  
> -/* Iterate through both audit_list_string and audit_list.  */
>  static const char *
> -audit_list_iter_next (struct audit_list_iter *iter)
> +audit_list_next (struct audit_list *list)
>  {
> -  if (iter->audit_list_tail != NULL)
> +  if (list->current_tail == NULL)
> +    return NULL;
> +
> +  while (true)
>      {
> -      /* First iterate over audit_list_string.  */
> -      while (*iter->audit_list_tail != '\0')
> +      /* Advance to the next string in audit_strings if the current
> +	 string has been exhausted.  */
> +      while (*list->current_tail == '\0')
>  	{
> -	  /* Split audit list at colon.  */
> -	  size_t len = strcspn (iter->audit_list_tail, ":");
> -	  if (len > 0 && len < sizeof (iter->fname))
> +	  ++list->current_index;
> +	  if (list->current_index == list->length)
>  	    {
> -	      memcpy (iter->fname, iter->audit_list_tail, len);
> -	      iter->fname[len] = '\0';
> +	      list->current_tail = NULL;
> +	      return NULL;
>  	    }
> -	  else
> -	    /* Do not return this name to the caller.  */
> -	    iter->fname[0] = '\0';
> -
> -	  /* Skip over the substring and the following delimiter.  */
> -	  iter->audit_list_tail += len;
> -	  if (*iter->audit_list_tail == ':')
> -	    ++iter->audit_list_tail;
> -
> -	  /* If the name is valid, return it.  */
> -	  if (dso_name_valid_for_suid (iter->fname))
> -	    return iter->fname;
> -	  /* Otherwise, wrap around and try the next name.  */
> +	  list->current_tail = list->audit_strings[list->current_index];
>  	}
> -      /* Fall through to the procesing of audit_list.  */
> -    }
>  
> -  if (iter->previous == NULL)
> -    {
> -      if (audit_list == NULL)
> -	/* No pre-parsed audit list.  */
> -	return NULL;
> -      /* Start of audit list.  The first list element is at
> -	 audit_list->next (cyclic list).  */
> -      iter->previous = audit_list->next;
> -      return iter->previous->name;
> +      /* Split the in-string audit list at the next colon colon.  */
> +      size_t len = strcspn (list->current_tail, ":");
> +      if (len > 0 && len < sizeof (list->fname))
> +	{
> +	  memcpy (list->fname, list->current_tail, len);
> +	  list->fname[len] = '\0';
> +	}
> +      else
> +	/* Mark the name as unusable for dso_name_valid_for_suid.  */
> +	list->fname[0] = '\0';
> +
> +      /* Skip over the substring and the following delimiter.  */
> +      list->current_tail += len;
> +      if (*list->current_tail == ':')
> +	++list->current_tail;
> +
> +      /* If the name is valid, return it.  */
> +      if (dso_name_valid_for_suid (list->fname))
> +	return list->fname;
> +
> +      /* Otherwise wrap around to find the next list element. .  */
>      }
> -  if (iter->previous == audit_list)
> -    /* Cyclic list wrap-around.  */
> -    return NULL;
> -  iter->previous = iter->previous->next;
> -  return iter->previous->name;
>  }
>  
>  #ifndef HAVE_INLINED_SYSCALLS
> @@ -1062,15 +1082,13 @@ notify_audit_modules_of_loaded_object (struct link_map *map)
>  
>  /* Load all audit modules.  */
>  static void
> -load_audit_modules (struct link_map *main_map)
> +load_audit_modules (struct link_map *main_map, struct audit_list *audit_list)
>  {
>    struct audit_ifaces *last_audit = NULL;
> -  struct audit_list_iter al_iter;
> -  audit_list_iter_init (&al_iter);
>  
>    while (true)
>      {
> -      const char *name = audit_list_iter_next (&al_iter);
> +      const char *name = audit_list_next (audit_list);
>        if (name == NULL)
>  	break;
>        load_audit_module (name, &last_audit);
> @@ -1102,6 +1120,9 @@ dl_main (const ElfW(Phdr) *phdr,
>    bool rtld_is_main = false;
>    void *tcbp = NULL;
>  
> +  struct audit_list audit_list;
> +  audit_list_init (&audit_list);
> +
>    GL(dl_init_static_tls) = &_dl_nothread_init_static_tls;
>  
>  #if defined SHARED && defined _LIBC_REENTRANT \
> @@ -1115,7 +1136,7 @@ dl_main (const ElfW(Phdr) *phdr,
>    GL(dl_make_stack_executable_hook) = &_dl_make_stack_executable;
>  
>    /* Process the environment variable which control the behaviour.  */
> -  process_envvars (&mode);
> +  process_envvars (&mode, &audit_list);
>  
>  #ifndef HAVE_INLINED_SYSCALLS
>    /* Set up a flag which tells we are just starting.  */
> @@ -1189,7 +1210,7 @@ dl_main (const ElfW(Phdr) *phdr,
>  	  }
>  	else if (! strcmp (_dl_argv[1], "--audit") && _dl_argc > 2)
>  	  {
> -	    process_dl_audit (_dl_argv[2]);
> +	    audit_list_add_string (&audit_list, _dl_argv[2]);
>  
>  	    _dl_skip_args += 2;
>  	    _dl_argc -= 2;
> @@ -1619,8 +1640,7 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
>  
>    /* If we have auditing DSOs to load, do it now.  */
>    bool need_security_init = true;
> -  if (__glibc_unlikely (audit_list != NULL)
> -      || __glibc_unlikely (audit_list_string != NULL))
> +  if (audit_list.length > 0)
>      {
>        /* Since we start using the auditing DSOs right away we need to
>  	 initialize the data structures now.  */
> @@ -1633,7 +1653,7 @@ ERROR: '%s': cannot process note segment.\n", _dl_argv[0]);
>        security_init ();
>        need_security_init = false;
>  
> -      load_audit_modules (main_map);
> +      load_audit_modules (main_map, &audit_list);
>      }
>  
>    /* Keep track of the currently loaded modules to count how many
> @@ -2507,30 +2527,6 @@ a filename can be specified using the LD_DEBUG_OUTPUT environment variable.\n");
>      }
>  }
>  
> -static void
> -process_dl_audit (char *str)
> -{
> -  /* The parameter is a colon separated list of DSO names.  */
> -  char *p;
> -
> -  while ((p = (strsep) (&str, ":")) != NULL)
> -    if (dso_name_valid_for_suid (p))
> -      {
> -	/* This is using the local malloc, not the system malloc.  The
> -	   memory can never be freed.  */
> -	struct audit_list *newp = malloc (sizeof (*newp));
> -	newp->name = p;
> -
> -	if (audit_list == NULL)
> -	  audit_list = newp->next = newp;
> -	else
> -	  {
> -	    newp->next = audit_list->next;
> -	    audit_list = audit_list->next = newp;
> -	  }
> -      }
> -}
> -
>  /* Process all environments variables the dynamic linker must recognize.
>     Since all of them start with `LD_' we are a bit smarter while finding
>     all the entries.  */
> @@ -2538,7 +2534,7 @@ extern char **_environ attribute_hidden;
>  
>  
>  static void
> -process_envvars (enum mode *modep)
> +process_envvars (enum mode *modep, struct audit_list *audit_list)
>  {
>    char **runp = _environ;
>    char *envline;
> @@ -2578,7 +2574,7 @@ process_envvars (enum mode *modep)
>  	      break;
>  	    }
>  	  if (memcmp (envline, "AUDIT", 5) == 0)
> -	    audit_list_string = &envline[6];
> +	    audit_list_add_string (audit_list, &envline[6]);

OK.

>  	  break;
>  
>  	case 7:
>
  

* Carlos O'Donell:

> On 4/3/20 8:02 AM, Florian Weimer via Libc-alpha wrote:
>> All list elements are colon-separated strings, and there is a hard
>> upper limit for the number of audit modules, so it is possible to
>> pre-allocate a fixed-size array of strings to which the LD_AUDIT
>> environment variable and --audit arguments are added.
>> 
>> Also eliminate the global variables for the audit list because
>> the list is only needed briefly during startup.
>> 
>> There is a slight behavior change: All duplicate LD_AUDIT environment
>> variables are now processed, not just the last one as before.  However,
>> such environment vectors are invalid anyway.
>
> Agreed.
>
> OK for master.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>

Thanks.

>> +  /* Grow the array if necessary.  */
>> +  if (list->length == array_length (list->audit_strings))
>> +    _dl_fatal_printf ("Too many audit modules requested\n");

I'm going to change this to:

  Fatal glibc error: Too many audit modules requested

Thanks,
Florian
  

On 4/3/20 8:58 AM, Florian Weimer wrote:
> * Carlos O'Donell:
> 
>> On 4/3/20 8:02 AM, Florian Weimer via Libc-alpha wrote:
>>> All list elements are colon-separated strings, and there is a hard
>>> upper limit for the number of audit modules, so it is possible to
>>> pre-allocate a fixed-size array of strings to which the LD_AUDIT
>>> environment variable and --audit arguments are added.
>>>
>>> Also eliminate the global variables for the audit list because
>>> the list is only needed briefly during startup.
>>>
>>> There is a slight behavior change: All duplicate LD_AUDIT environment
>>> variables are now processed, not just the last one as before.  However,
>>> such environment vectors are invalid anyway.
>>
>> Agreed.
>>
>> OK for master.
>>
>> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
> 
> Thanks.
> 
>>> +  /* Grow the array if necessary.  */
>>> +  if (list->length == array_length (list->audit_strings))
>>> +    _dl_fatal_printf ("Too many audit modules requested\n");
> 
> I'm going to change this to:
> 
>   Fatal glibc error: Too many audit modules requested

That seems fine to me.
  

On Apr 03 2020, Florian Weimer via Libc-alpha wrote:

> +  /* Grow the array if necessary.  */
> +  if (list->length == array_length (list->audit_strings))
> +    _dl_fatal_printf ("Too many audit modules requested\n");

That comment doesn't make sense.  You are definitely not growing
anything here.

Andreas.
  

* Andreas Schwab:

> On Apr 03 2020, Florian Weimer via Libc-alpha wrote:
>
>> +  /* Grow the array if necessary.  */
>> +  if (list->length == array_length (list->audit_strings))
>> +    _dl_fatal_printf ("Too many audit modules requested\n");
>
> That comment doesn't make sense.  You are definitely not growing
> anything here.

Right, it's a leftover from previous versions.  I will fix it.  Thanks.

Florian