Re: [PATCH] elf: Compute correct array size in _dl_init_paths [BZ #22606]

["Dmitry V. Levin" <ldv at altlinux dot org>]

On Thu, Dec 14, 2017 at 01:55:50PM +0100, Florian Weimer wrote:
> 2017-12-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22606]
> 	CVE-2017-1000408
> 	* elf/dl-load.c (system_dirs): Update comment.
> 	(nsystem_dirs_len): Use array_length.
> 	(_dl_init_paths): Use nsystem_dirs_len to compute the array size.
> 
> diff --git a/NEWS b/NEWS
> index abbe561dcb..eef51b65a6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -125,6 +125,11 @@ Security related changes:
>    instead of NULL.  This was a regression introduced with the new malloc
>    thread cache in glibc 2.26.  Reported by Iain Buclaw.
>  
> +  CVE-2017-1000408: Incorrect array size computation in _dl_init_paths leads
> +  to the allocation of too much memory.  (This is not a security bug per se,
> +  it is mentioned here only because of the CVE assignment.)  Reported by
> +  Qualys.
> +
>  The following bugs are resolved with this release:
>  
>    [The release manager will add the list generated by
> diff --git a/elf/dl-load.c b/elf/dl-load.c
> index 10d859bd35..82c9f46050 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -37,6 +37,7 @@
>  #include <sysdep.h>
>  #include <stap-probe.h>
>  #include <libc-pointer-arith.h>
> +#include <array_length.h>
>  
>  #include <dl-dst.h>
>  #include <dl-load.h>
> @@ -103,7 +104,9 @@ static size_t ncapstr attribute_relro;
>  static size_t max_capstrlen attribute_relro;
>  
>  
> -/* Get the generated information about the trusted directories.  */
> +/* Get the generated information about the trusted directories.  Use
> +   an array of concatenated strings to avoid relocations.  See
> +   gen-trusted-dirs.awk.  */
>  #include "trusted-dirs.h"
>  
>  static const char system_dirs[] = SYSTEM_DIRS;
> @@ -111,9 +114,7 @@ static const size_t system_dirs_len[] =
>  {
>    SYSTEM_DIRS_LEN
>  };
> -#define nsystem_dirs_len \
> -  (sizeof (system_dirs_len) / sizeof (system_dirs_len[0]))
> -
> +#define nsystem_dirs_len array_length (system_dirs_len)
>  
>  static bool
>  is_trusted_path (const char *path, size_t len)
> @@ -685,9 +686,8 @@ _dl_init_paths (const char *llp)
>  		 + ncapstr * sizeof (enum r_dir_status))
>  		/ sizeof (struct r_search_path_elem));
>  
> -  rtld_search_dirs.dirs[0] = (struct r_search_path_elem *)
> -    malloc ((sizeof (system_dirs) / sizeof (system_dirs[0]))
> -	    * round_size * sizeof (struct r_search_path_elem));
> +  rtld_search_dirs.dirs[0] = malloc (nsystem_dirs_len * round_size
> +				     * sizeof (struct r_search_path_elem));
>    if (rtld_search_dirs.dirs[0] == NULL)
>      {
>        errstring = N_("cannot create cache for search path");

This is OK.  My personal preference for that kind of code is

> +  rtld_search_dirs.dirs[0] = malloc (nsystem_dirs_len * round_size
> +				     * sizeof (**rtld_search_dirs.dirs));

so I certainly wouldn't object if it was used instead.


-- 
ldv

Attachment: signature.asc
Description: PGP signature