Re: [PATCH] ld.so: Reject overly long LD_PRELOAD path elements

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

On 06/19/2017 12:13 PM, Florian Weimer wrote:
> 2017-06-19  Florian Weimer  <fweimer@redhat.com>
> 
> 	* elf/rtld.c (dso_name_valid_for_suid): New function.
> 	(handle_ld_preload): Likewise.
> 	(dl_main): Call it.  Remove alloca.

It is my opinion that this can be applied almost as-is directly today
with a small tweak to the GNU/Hurd.

This patch is an understanding in glibc that for SUID binaries there
are some limits we need to impose because of the semantics of the way
our security measures (guard pages) interact user input (environment
variables).

For these cases I think you should be doing:

/* For SUID binaries, all glibc ports have limits, even though we want
   to avoid limits in the GNU operating system.
   For those operating systems that do not define such limits, we
   define them to an arbitrary but small value.  The GNU/Hurd includes 
   no such limits, but we define them for now as a security heuristic for
   SUID binaries.  */
#ifndef NAME_MAX
#define NAME_MAX 4096
#endif

This is what debian is doing to fix the hurd builds there and it's what
we should adopt immediately upstream to put in place these mitigations.

OK to checkin with a fix for missing NAME_MAX and PATH_MAX which defines
them as a limit.

> diff --git a/elf/rtld.c b/elf/rtld.c
> index 2269dbe..c801ee5 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -99,6 +99,22 @@ uintptr_t __pointer_chk_guard_local
>  strong_alias (__pointer_chk_guard_local, __pointer_chk_guard)
>  #endif
>  
> +/* Check that AT_SECURE=0, or that the passed name does not contain
> +   directories and is not overly long.  Reject empty names
> +   unconditionally.  */
> +static bool
> +dso_name_valid_for_suid (const char *p)
> +{
> +  if (__glibc_unlikely (__libc_enable_secure))
> +    {
> +      /* Ignore pathnames with directories for AT_SECURE=1
> +	 programs, and also skip overlong names.  */
> +      size_t len = strlen (p);
> +      if (len >= NAME_MAX || memchr (p, '/', len) != NULL)
> +	return false;
> +    }
> +  return *p != '\0';
> +}
>  
>  /* List of auditing DSOs.  */
>  static struct audit_list
> @@ -718,6 +734,42 @@ static const char *preloadlist attribute_relro;
>  /* Nonzero if information about versions has to be printed.  */
>  static int version_info attribute_relro;
>  
> +/* The LD_PRELOAD environment variable gives list of libraries
> +   separated by white space or colons that are loaded before the
> +   executable's dependencies and prepended to the global scope list.
> +   (If the binary is running setuid all elements containing a '/' are
> +   ignored since it is insecure.)  Return the number of preloads
> +   performed.  */
> +unsigned int
> +handle_ld_preload (const char *preloadlist, struct link_map *main_map)
> +{
> +  unsigned int npreloads = 0;
> +  const char *p = preloadlist;
> +  char fname[PATH_MAX];
> +
> +  while (*p != '\0')
> +    {
> +      /* Split preload list at space/colon.  */
> +      size_t len = strcspn (p, " :");
> +      if (len > 0 && len < PATH_MAX)
> +	{
> +	  memcpy (fname, p, len);
> +	  fname[len] = '\0';
> +	}
> +      else
> +	fname[0] = '\0';
> +
> +      /* Skip over the substring and the following delimiter.  */
> +      p += len;
> +      if (*p == ' ' || *p == ':')
> +	++p;
> +
> +      if (dso_name_valid_for_suid (fname))
> +	npreloads += do_preload (fname, main_map, "LD_PRELOAD");
> +    }
> +  return npreloads;
> +}
> +
>  static void
>  dl_main (const ElfW(Phdr) *phdr,
>  	 ElfW(Word) phnum,
> @@ -1464,23 +1516,8 @@ ERROR: ld.so: object '%s' cannot be loaded as audit interface: %s; ignored.\n",
>  
>    if (__glibc_unlikely (preloadlist != NULL))
>      {
> -      /* The LD_PRELOAD environment variable gives list of libraries
> -	 separated by white space or colons that are loaded before the
> -	 executable's dependencies and prepended to the global scope
> -	 list.  If the binary is running setuid all elements
> -	 containing a '/' are ignored since it is insecure.  */
> -      char *list = strdupa (preloadlist);
> -      char *p;
> -
>        HP_TIMING_NOW (start);
> -
> -      /* Prevent optimizing strsep.  Speed is not important here.  */
> -      while ((p = (strsep) (&list, " :")) != NULL)
> -	if (p[0] != '\0'
> -	    && (__builtin_expect (! __libc_enable_secure, 1)
> -		|| strchr (p, '/') == NULL))
> -	  npreloads += do_preload (p, main_map, "LD_PRELOAD");
> -
> +      npreloads += handle_ld_preload (preloadlist, main_map);
>        HP_TIMING_NOW (stop);
>        HP_TIMING_DIFF (diff, start, stop);
>        HP_TIMING_ACCUM_NT (load_time, diff);
> 


-- 
Cheers,
Carlos.