Re: [PATCH] elf: Count components of the expanded path in _dl_init_path [BZ #22607]

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

On Thu, Dec 14, 2017 at 03:08:31PM +0100, Florian Weimer wrote:
> On 12/14/2017 02:58 PM, Andreas Schwab wrote:
> > On Dez 14 2017, Florian Weimer <fweimer@redhat.com> wrote:
> >> On 12/14/2017 02:45 PM, Andreas Schwab wrote:
> >>> On Dez 14 2017, fweimer@redhat.com (Florian Weimer) wrote:
> >>>
> >>>> +      {
> >>>> +	const char *cp = llp_tmp;
> >>>> +	while (*cp)
> >>>> +	  {
> >>>> +	    if (*cp == ':' || *cp == ';')
> >>>> +	      ++nllp;
> >>>> +	    ++cp;
> >>>> +	  }
> >>>> +      }
> >>>
> >>> No need for the outermost braces.
> >>
> >> I included them to limit the scope of cp, to make sure that there aren't
> >> any uses afterwards because of the changed value of cp compared to the
> >> original code.
> > 
> > Since it is obviously unused afterwards I don't see any value for that.
> 
> What about this?  It follows Dmitry's suggestion to use a for loop.
> 
> Thanks,
> Florian

> Subject: [PATCH] elf: Count components of the expanded path in _dl_init_path [BZ #22607]
> To: libc-alpha@sourceware.org
> 
> 2017-12-14  Florian Weimer  <fweimer@redhat.com>
> 
> 	[BZ #22607]
> 	CVE-2017-1000409
> 	* elf/dl-load.c (_dl_init_paths): Compute number of components in
> 	the expanded path string.
> 
> diff --git a/NEWS b/NEWS
> index eef51b65a6..c5607c855f 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -130,6 +130,12 @@ Security related changes:
>    it is mentioned here only because of the CVE assignment.)  Reported by
>    Qualys.
>  
> +  CVE-2017-1000409: Buffer overflow in _dl_init_paths due to miscomputation
> +  of the number of search path components.  (This is not a security
> +  vulnerability per se because no trust boundary is crossed if the fix for
> +  CVE-2017-1000366 has been applied, but 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 82c9f46050..f5a9c0cc8e 100644
> --- a/elf/dl-load.c
> +++ b/elf/dl-load.c
> @@ -773,8 +773,6 @@ _dl_init_paths (const char *llp)
>  
>    if (llp != NULL && *llp != '\0')
>      {
> -      size_t nllp;
> -      const char *cp = llp;
>        char *llp_tmp;
>  
>  #ifdef SHARED
> @@ -797,13 +795,10 @@ _dl_init_paths (const char *llp)
>  
>        /* Decompose the LD_LIBRARY_PATH contents.  First determine how many
>  	 elements it has.  */
> -      nllp = 1;
> -      while (*cp)
> -	{
> -	  if (*cp == ':' || *cp == ';')
> -	    ++nllp;
> -	  ++cp;
> -	}
> +      size_t nllp = 1;
> +      for (const char *cp = llp_tmp; *cp != '\0'; ++cp)
> +	if (*cp == ':' || *cp == ';')
> +	  ++nllp;
>  
>        env_path_list.dirs = (struct r_search_path_elem **)
>  	malloc ((nllp + 1) * sizeof (struct r_search_path_elem *));

Yes, this is fine.


-- 
ldv

Attachment: signature.asc
Description: PGP signature