commit 4c6e0415ef206a595c62d5d37e3b9a821782c533

Author: Florian Weimer <fweimer@redhat.com>

Date:   Fri Apr 3 13:17:48 2020 +0200

    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.

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

diff --git a/elf/rtld.c b/elf/rtld.c

index f755dc30331f799f..c39cb8f2cd4bb1cc 100644

--- a/elf/rtld.c

+++ b/elf/rtld.c

@@ -43,6 +43,7 @@

 #include <stap-probe.h>

 #include <stackinfo.h>

 #include <not-cancel.h>

+#include <array_length.h>

 #include <assert.h>

@@ -107,8 +108,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

+

+/* 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 };

@@ -116,7 +162,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;

@@ -144,19 +190,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.  */

@@ -174,89 +207,75 @@ 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;

+}

-/* 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;

+  if (list->length == array_length (list->audit_strings))

+    _dl_fatal_printf ("Fatal glibc error: 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;

 }

 /* Set nonzero during loading and initialization of executable and

@@ -1060,15 +1079,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);

@@ -1100,6 +1117,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 \

@@ -1113,7 +1133,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);

   /* Set up a flag which tells we are just starting.  */

   _dl_starting_up = 1;

@@ -1185,7 +1205,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;

@@ -1612,8 +1632,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.  */

@@ -1626,7 +1645,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

@@ -2500,30 +2519,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.  */

@@ -2531,7 +2526,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;

@@ -2571,7 +2566,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]);

 	  break;

 	case 7: