This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] rtld: Reject overly long LD_AUDIT path elements


On 06/26/2017 02:57 PM, Andreas Schwab wrote:
> On Jun 26 2017, Florian Weimer <fweimer@redhat.com> wrote:
> 
>> The goal is to prevent massaging the heap through LD_AUDIT variable
>> contents.  So it's purely hardening.
> 
> Why is that needed?

I'm not sure if it is needed.  I am not an experienced exploit writer.

I assume you want me to apply something like the attached patch, right?

Thanks,
Florian
ld.so: Simplify processing of LD_AUDIT

Return to the old scheme of calling process_dl_audit and allocating
the audit list on the heap, but process the last environment variable
only.

This reverts most of commit 81b82fb966ffbd94353f793ad17116c6088dedd9.

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

	* elf/rtld.c (process_dl_audit): Call dso_name_valid_for_suid.
	(audit_list_string): Remove variable.
	(struct audit_list_iter): Remove definition.
	(audit_list_iter_init, audit_list_iter_next): Remove functions.
	(dl_main): Remove iterator code and iterate over audit_list
	directly.
	(process_envvars): Process LD_AUDIT once using process_dl_audit.

diff --git a/elf/rtld.c b/elf/rtld.c
index 65647fb..3898257 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -129,10 +129,6 @@ 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
@@ -141,79 +137,6 @@ static struct audit_list
   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[SECURE_NAME_LIMIT];
-};
-
-/* 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 < sizeof (iter->fname))
-	    {
-	      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
@@ -1383,13 +1306,11 @@ 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.  */
-  bool need_security_init = true;
-  if (__glibc_unlikely (audit_list != NULL)
-      || __glibc_unlikely (audit_list_string != NULL))
+  if (__glibc_unlikely (audit_list != NULL))
     {
+      /* Iterate over all entries in the list.  The order is important.  */
       struct audit_ifaces *last_audit = NULL;
-      struct audit_list_iter al_iter;
-      audit_list_iter_init (&al_iter);
+      struct audit_list *al = audit_list->next;
 
       /* Since we start using the auditing DSOs right away we need to
 	 initialize the data structures now.  */
@@ -1400,14 +1321,9 @@ 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;
 
-      while (true)
+      do
 	{
-	  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
@@ -1416,7 +1332,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 = name;
+	  dlmargs.fname = al->name;
 	  dlmargs.map = NULL;
 
 	  const char *objname;
@@ -1429,7 +1345,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",
-				name, err_str);
+				al->name, err_str);
 	      if (malloced)
 		free ((char *) err_str);
 	    }
@@ -1533,7 +1449,10 @@ 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.  */
@@ -1797,7 +1716,7 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
   if (tcbp == NULL)
     tcbp = init_tls ();
 
-  if (__glibc_likely (need_security_init))
+  if (__glibc_likely (audit_list == NULL))
     /* Initialize security features.  But only if we have not done it
        earlier.  */
     security_init ();
@@ -2458,6 +2377,7 @@ process_envvars (enum mode *modep)
   char *envline;
   enum mode mode = normal;
   char *debug_output = NULL;
+  char *audit_list_string = NULL;
 
   /* This is the default place for profiling data file.  */
   GLRO(dl_profile_output)
@@ -2623,6 +2543,9 @@ process_envvars (enum mode *modep)
   /* The caller wants this information.  */
   *modep = mode;
 
+  if (audit_list_string != NULL)
+    process_dl_audit (audit_list_string);
+
   /* Extra security for SUID binaries.  Remove all dangerous environment
      variables.  */
   if (__builtin_expect (__libc_enable_secure, 0))

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]