[PATCH] rtld: Reject overly long LD_AUDIT path elements

[fweimer at redhat dot com (Florian Weimer)]

Also only process the last LD_AUDIT entry.

2017-06-19  Florian Weimer  <fweimer@redhat.com>

	* elf/rtld.c (audit_list_string): New variable.
	(audit_list): Update comment.
	(struct audit_list_iter): Define.
	(audit_list_iter_init, audit_list_iter_next): New function.
	(dl_main): Use struct audit_list_iter to process audit modules.
	(process_dl_audit): Call dso_name_valid_for_suid.
	(process_envvars): Set audit_list_string instead of calling
	process_dl_audit.

diff --git a/elf/rtld.c b/elf/rtld.c
index c801ee5..74147c3 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -116,13 +116,91 @@ dso_name_valid_for_suid (const char *p)
   return *p != '\0';
 }
 
-/* List of auditing DSOs.  */
+/* 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
 {
   const char *name;
   struct audit_list *next;
 } *audit_list;
 
+/* Iterator for audit_list_string followed by audit_list.  */
+struct audit_list_iter
+{
+  /* Tail of audit_list_string still needing processing, or NULL.  */
+  const char *audit_list_tail;
+
+  /* The list element returned in the previous iteration.  NULL before
+     the first element.  */
+  struct audit_list *previous;
+
+  /* Scratch buffer for returning a name which is part of
+     audit_list_string.  */
+  char fname[PATH_MAX];
+};
+
+/* 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;
+}
+
+/* Iterate through both audit_list_string and audit_list.  */
+static const char *
+audit_list_iter_next (struct audit_list_iter *iter)
+{
+  if (iter->audit_list_tail != NULL)
+    {
+      /* First iterate over audit_list_string.  */
+      while (*iter->audit_list_tail != '\0')
+	{
+	  /* Split audit list at colon.  */
+	  size_t len = strcspn (iter->audit_list_tail, ":");
+	  if (len > 0 && len < PATH_MAX)
+	    {
+	      memcpy (iter->fname, iter->audit_list_tail, len);
+	      iter->fname[len] = '\0';
+	    }
+	  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.  */
+	}
+      /* 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;
+    }
+  if (iter->previous == audit_list)
+    /* Cyclic list wrap-around.  */
+    return NULL;
+  iter->previous = iter->previous->next;
+  return iter->previous->name;
+}
+
 #ifndef HAVE_INLINED_SYSCALLS
 /* Set nonzero during loading and initialization of executable and
    libraries, cleared before the executable's entry point runs.  This
@@ -1292,11 +1370,13 @@ of this helper program; chances are you did not intend to run this program.\n\
     GL(dl_rtld_map).l_tls_modid = _dl_next_tls_modid ();
 
   /* If we have auditing DSOs to load, do it now.  */
-  if (__glibc_unlikely (audit_list != NULL))
+  bool need_security_init = true;
+  if (__glibc_unlikely (audit_list != NULL)
+      || __glibc_unlikely (audit_list_string != NULL))
     {
-      /* Iterate over all entries in the list.  The order is important.  */
       struct audit_ifaces *last_audit = NULL;
-      struct audit_list *al = audit_list->next;
+      struct audit_list_iter al_iter;
+      audit_list_iter_init (&al_iter);
 
       /* Since we start using the auditing DSOs right away we need to
 	 initialize the data structures now.  */
@@ -1307,9 +1387,14 @@ of this helper program; chances are you did not intend to run this program.\n\
 	 use different values (especially the pointer guard) and will
 	 fail later on.  */
       security_init ();
+      need_security_init = false;
 
-      do
+      while (true)
 	{
+	  const char *name = audit_list_iter_next (&al_iter);
+	  if (name == NULL)
+	    break;
+
 	  int tls_idx = GL(dl_tls_max_dtv_idx);
 
 	  /* Now it is time to determine the layout of the static TLS
@@ -1318,7 +1403,7 @@ of this helper program; chances are you did not intend to run this program.\n\
 	     no DF_STATIC_TLS bit is set.  The reason is that we know
 	     glibc will use the static model.  */
 	  struct dlmopen_args dlmargs;
-	  dlmargs.fname = al->name;
+	  dlmargs.fname = name;
 	  dlmargs.map = NULL;
 
 	  const char *objname;
@@ -1331,7 +1416,7 @@ of this helper program; chances are you did not intend to run this program.\n\
 	    not_loaded:
 	      _dl_error_printf ("\
 ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
-				al->name, err_str);
+				name, err_str);
 	      if (malloced)
 		free ((char *) err_str);
 	    }
@@ -1435,10 +1520,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
 		  goto not_loaded;
 		}
 	    }
-
-	  al = al->next;
 	}
-      while (al != audit_list->next);
 
       /* If we have any auditing modules, announce that we already
 	 have two objects loaded.  */
@@ -1702,7 +1784,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
   if (tcbp == NULL)
     tcbp = init_tls ();
 
-  if (__glibc_likely (audit_list == NULL))
+  if (__glibc_likely (need_security_init))
     /* Initialize security features.  But only if we have not done it
        earlier.  */
     security_init ();
@@ -2333,9 +2415,7 @@ process_dl_audit (char *str)
   char *p;
 
   while ((p = (strsep) (&str, ":")) != NULL)
-    if (p[0] != '\0'
-	&& (__builtin_expect (! __libc_enable_secure, 1)
-	    || strchr (p, '/') == NULL))
+    if (dso_name_valid_for_suid (p))
       {
 	/* This is using the local malloc, not the system malloc.  The
 	   memory can never be freed.  */
@@ -2399,7 +2479,7 @@ process_envvars (enum mode *modep)
 	      break;
 	    }
 	  if (memcmp (envline, "AUDIT", 5) == 0)
-	    process_dl_audit (&envline[6]);
+	    audit_list_string = &envline[6];
 	  break;
 
 	case 7: