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

  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
  

On Dez 14 2017, Florian Weimer <fweimer@redhat.com> 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.

Ok.

Andreas.
  

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.