rtld: Reject overly long LD_AUDIT path elements
diff mbox

Message ID 34b57a2e-356b-3a92-1403-e6d16529b0eb@redhat.com
State New
Headers show

Commit Message

Florian Weimer June 29, 2017, 7:05 p.m. UTC
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

Comments

Zack Weinberg June 29, 2017, 7:56 p.m. UTC | #1
On Thu, Jun 29, 2017 at 3:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
> 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?

I am not an experienced exploit writer either, and I don't know this
code at all, but as a matter of principle, I do not think you should
make any changes until Andreas actually explains his concerns in
_detail_.  One-sentence cryptic questions, at a rate of one per email,
are not proper code review.

zw
Florian Weimer June 29, 2017, 8:36 p.m. UTC | #2
On 06/29/2017 09:56 PM, Zack Weinberg wrote:
> On Thu, Jun 29, 2017 at 3:05 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> 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?
> 
> I am not an experienced exploit writer either, and I don't know this
> code at all, but as a matter of principle, I do not think you should
> make any changes until Andreas actually explains his concerns in
> _detail_.  One-sentence cryptic questions, at a rate of one per email,
> are not proper code review.

To be fair, the original patch went in without much review on
libc-alpha, too.

Florian
Zack Weinberg June 29, 2017, 9:09 p.m. UTC | #3
On Thu, Jun 29, 2017 at 4:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
> On 06/29/2017 09:56 PM, Zack Weinberg wrote:
>>
>> I am not an experienced exploit writer either, and I don't know this
>> code at all, but as a matter of principle, I do not think you should
>> make any changes until Andreas actually explains his concerns in
>> _detail_.  One-sentence cryptic questions, at a rate of one per email,
>> are not proper code review.
>
> To be fair, the original patch went in without much review on
> libc-alpha, too.

Carlos made clear that he had, in fact, read the entire thing, and I
trust that if he had had concerns he would have said so.

zw
Carlos O'Donell June 30, 2017, 1:46 p.m. UTC | #4
On 06/29/2017 05:09 PM, Zack Weinberg wrote:
> On Thu, Jun 29, 2017 at 4:36 PM, Florian Weimer <fweimer@redhat.com> wrote:
>> On 06/29/2017 09:56 PM, Zack Weinberg wrote:
>>>
>>> I am not an experienced exploit writer either, and I don't know this
>>> code at all, but as a matter of principle, I do not think you should
>>> make any changes until Andreas actually explains his concerns in
>>> _detail_.  One-sentence cryptic questions, at a rate of one per email,
>>> are not proper code review.
>>
>> To be fair, the original patch went in without much review on
>> libc-alpha, too.
> 
> Carlos made clear that he had, in fact, read the entire thing, and I
> trust that if he had had concerns he would have said so.

I read the whole thing, and multiple times. I had no concerns with the 
patch. Is it exactly how I would have solved it? No. However, my 
responsibility as a reviewer is not to make everyone conform to the 
way *I* would have done it. In fact I appreciate immensely that Florian
comes up with answers that I didn't think about, because it adds
resilience to our community.

If there is a defect in the original patch I didn't detect it in my
review, and I reviewed over a dozen backports of the same patch. I think
I could rewrite it by hand without looking at the original.

The only overarching concern for *all* the patches is that we previously
had no limits, which is what the CVE is about, and now we *do* have limits
and that's because we need them to protect against this attack vector.

Eventually I forsee with -fstack-check we might lift the limits again,
but until then we need to limit exactly what secure applications can do.

Patch
diff mbox

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))