Re: [PATCH] CVE-2015-7547 --- glibc getaddrinfo() stack-based buffer overflow

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

On 02/16/2016 01:13 PM, Mike Frysinger wrote:
> On 16 Feb 2016 09:09, Carlos O'Donell wrote:
> 
> many of us have seen this already, so were you going to wait for much
> public review before pushing it ?  assuming you'll be applying it to
> the older branches too.

Thanks for the review.

Committing the patch to master is not time sensitive except for the
upcoming release. I will coordinate with Adhemerval so the branch is
not cut until we commit this fix.

I will be committing shortly. I will also be following up quickly if
anyone finds any other problems (which I hope don't exist).

I'll be looking at backports based on Fedora's requirements and those
requirements from others. Any help from Debian, Ubuntu, or Gentoo to
prepare and test those backports is much appreciated. Basically you
want all of the tests in the regression test framework to pass cleanly
both with and without valgrind. Obviously committing to master is a
pre-requisite to backporting.

> just nits below as i'm not familiar with resolver internals.
> 
>> --- a/resolv/nss_dns/dns-host.c
>> +++ b/
>>  resolv/nss_dns/dns-host.c
>> +     answer 2, but that's not true (see the implemetnation of send_dg
> 
> "implementation"

Fixed.

> 
>> +     and send_vc to see response can arrive in any order, particlarly
> 
> "particularly"

Fixed.

> 
>> +     [1] If the first response is a success we return success.
>> +         This ignores the state of the second answer and in fact
>> +         incorrectly sets errno and h_errno to that of the second
>> +	 answer.  However because the response is a success we ignore
>> +	 *errnop and *h_errnop (though that means you touched errno on
>> +         success).  We are being conservative here and returning the
>> +         likely IPv4 response in the first answer as a success.
> 
> inconsistency with indentation

Fixed.

>> +      /* Do not return a truncated second response (unless it was
>> +         unavoidable e.g. unrecoverable TRYAGAIN).  */
> 
> 2nd line should use a tab

Fixed.

>> --- a/resolv/res_send.c
>> +++ b/resolv/res_send.c
>>  
>> +   Please also note that for TCP we send both queries over the same
>> +   socket one after another.  This technically violates best practice
>> +   since the server is allowed to read the first query, respond, and
>> +   then close the socket (to service another client).  If the server
>> +   does this, then the remaining second query in the socket data buffer
>> +   will cause the server to send the client an RST which will arrive
>> +   asynchronously and the client's OS will likely tear down the socket
>> +   receive buffer resulting in a potentially short read and lost
>> +   response data.  This will force the client to retry the query again,
>> +   and this process may repeat until all servers and connection resets
>> +   are exhausted and then the query will fail.  It's not known if this
>> +   happens with any frequency in real DNS server implementations.  This
>> +   implementation should be corrected to use two sockets by default for
>> +   parallel queries.
> 
> should we open a bug now for this ?

Yes. Would you mind helping with that?

>> +   packets header feild TC will bet set to 1, indicating a truncated
> 
> "field"

Fixed (twice).

>> +	/* Skip the second response if there is no second query.
>> +           To do that we mark the second response as received.  */
> 
> 2nd line should use tabs.  this comes up twice in the file.

Fixed (twice).

Cheers,
Carlos.