ld.so: Reject overly long LD_PRELOAD path elements

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.

On Jun 19 2017, fweimer@redhat.com (Florian Weimer) wrote:

> +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 == ':')

If you use *p != '\0' you don't have to repeat the list of delimiters.

Andreas.

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

On 06/19/2017 06:27 PM, Andreas Schwab wrote:
>> +      /* Skip over the substring and the following delimiter.  */
>> +      p += len;
>> +      if (*p == ' ' || *p == ':')
> If you use *p != '\0' you don't have to repeat the list of delimiters.

Thanks, applied.

Florian

On 06/19/2017 10:00 PM, Carlos O'Donell wrote:
> /* 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

We use 1024 for PATH_MAX in various places inside glibc already, so I'm
going to stick with that.  NAME_MAX should be 255, I think.  This is
what I'm going to check in:

/* Length limits for names and paths, to protect the dynamic linker,
   particularly when __libc_enable_secure is active.  */
#ifdef NAME_MAX
# define SECURE_NAME_LIMIT NAME_MAX
#else
# define SECURE_NAME_LIMIT 255
#endif
#ifdef PATH_MAX
# define SECURE_PATH_LIMIT PATH_MAX
#else
# define SECURE_PATH_LIMIT 1024
#endif

Thanks,
Florian