Re: [PATCH][BZ #16072] Fix stack overflow due to large AF_INET6 requests

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/22/2013 08:51 AM, Siddhesh Poyarekar wrote:
> On Tue, Oct 22, 2013 at 12:45:51PM +0530, Siddhesh Poyarekar wrote:
>> Hi,
>>
>> This patch fixes another stack overflow in getaddrinfo when it is
>> called with AF_INET6.  The AF_UNSPEC case was fixed as CVE-2013-1914,
>> but the AF_INET case went undetected back then.
>>
>> Tested on x86_64 - same reproducer as 16071, with AF_INET6 instead of
>> AF_INET.  OK to commit?  Also, should I add another NEWS item for this
>> with this CVE number or should I post a request for another CVE?
>>
>> Siddhesh
>>
>> 	[BZ #16072]
>> 	* sysdeps/posix/getaddrinfo.c (gethosts): Allocate tmpbuf on
>> 	heap for large requests.
>>
> 
> A (very) slightly improved patch - fixed alloca accounting in
> gethosts.

I just debugged this for myself recently so I can comment and review.
 
> diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
> index 0f4b885..69bc97b 100644
> --- a/sysdeps/posix/getaddrinfo.c
> +++ b/sysdeps/posix/getaddrinfo.c
> @@ -197,7 +197,22 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>  				&rc, &herrno, NULL, &localcanon));	      \
>      if (rc != ERANGE || herrno != NETDB_INTERNAL)			      \
>        break;								      \
> -    tmpbuf = extend_alloca (tmpbuf, tmpbuflen, 2 * tmpbuflen);		      \
> +    if (!malloc_tmpbuf && __libc_use_alloca (alloca_used + 2 * tmpbuflen))    \

__glibc_likely? Who has huge /etc/hosts files?

> +      tmpbuf = extend_alloca_account (tmpbuf, tmpbuflen, 2 * tmpbuflen,	      \
> +				      alloca_used);			      \
> +    else								      \
> +      {									      \
> +	char *newp = realloc (malloc_tmpbuf ? tmpbuf : NULL,		      \
> +			      2 * tmpbuflen);				      \

realloc has no limit making it possible to DoS the system.

Should we consider a fixed size constant as the upper limit for this malloc?

See:
https://sourceware.org/glibc/wiki/Style_and_Conventions#Alloca_vs._Malloc
"If the size of the buffer is directly or indirectly under user control, 
consider imposing a maximum to help make denial-of-service attacks more 
difficult. "

> +	if (newp == NULL)						      \
> +	  {								      \
> +	    result = -EAI_MEMORY;					      \
> +	    goto free_and_return;					      \
> +	  }								      \
> +	tmpbuf = newp;							      \
> +	malloc_tmpbuf = true;						      \
> +	tmpbuflen = 2 * tmpbuflen;					      \
> +      }									      \
>    }									      \
>    if (status == NSS_STATUS_SUCCESS && rc == 0)				      \
>      h = &th;								      \
> @@ -209,7 +224,8 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
>  	{								      \
>  	  __set_h_errno (herrno);					      \
>  	  _res.options |= old_res_options & RES_USE_INET6;		      \
> -	  return -EAI_SYSTEM;						      \
> +	  result = -EAI_SYSTEM;						      \
> +	  goto free_and_return;						      \
>  	}								      \
>        if (herrno == TRY_AGAIN)						      \
>  	no_data = EAI_AGAIN;						      \
> 

Cheers,
Carlos.