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

[Carlos O'Donell <carlos at redhat dot com>]

On 06/19/2017 12:13 PM, Florian Weimer wrote:
> 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.

The same rationale for PATH_MAX applies here as it did for the review
of the LD_LIBRARY_PATH patch.

For now I think the community should adopt a measured position for
SUID binaries, for all GNU operating systems including GNU/Hurd, and
place a small but useful limit for PATH_MAX. This value is used only
inside glibc and only for SUID binaries.

OK to checkin IMO as long as you define PATH_MAX for operating systems
that don't define it, and you would have already done that for the
earlier LD_LIBRARY_PATH patch, so this patch would be safe to use PATH_MAX.

Until we have a better understanding of how we are going to fix this with
gcc -fstack-check, we need these heuristics in upstream.

> 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;
> +

OK.

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

OK.

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

OK.

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

OK.

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

OK.

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

OK.

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

OK.

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

OK.

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

OK.

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

OK.

>  	  break;
>  
>  	case 7:
> 


-- 
Cheers,
Carlos.