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

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

[Florian Weimer]

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..540f91f9d6 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,16 @@ _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;
+      {
+	const char *cp = llp_tmp;
+	while (*cp)
+	  {
+	    if (*cp == ':' || *cp == ';')
+	      ++nllp;
+	    ++cp;
+	  }
+      }
 
       env_path_list.dirs = (struct r_search_path_elem **)
 	malloc ((nllp + 1) * sizeof (struct r_search_path_elem *));

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

[Andreas Schwab]

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.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Florian Weimer]

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.

Thanks,
Florian

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

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2052 bytes --]

On Thu, Dec 14, 2017 at 02:31:25PM +0100, Florian Weimer wrote:
> 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..540f91f9d6 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,16 @@ _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;
> +      {
> +	const char *cp = llp_tmp;
> +	while (*cp)
> +	  {
> +	    if (*cp == ':' || *cp == ';')
> +	      ++nllp;
> +	    ++cp;
> +	  }
> +      }

I'd really prefer to see a "for" statement here, e.g.

+      size_t nllp = 1;
+      {
+	const char *cp;
+	for (cp = llp_tmp; *cp; ++cp)
+	  {
+	    if (*cp == ':' || *cp == ';')
+	      ++nllp;
+	  }
+      }
	

-- 
ldv

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Andreas Schwab]

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.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Florian Weimer]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

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

[-- Attachment #2: llp.patch --]
[-- Type: text/x-patch, Size: 1782 bytes --]

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

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

[Andreas Schwab]

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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

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

[Dmitry V. Levin]

[-- Attachment #1: Type: text/plain, Size: 2920 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]