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

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 16-02-2016 16:24, Carlos O'Donell wrote:
> 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.

Although the date is set for next 18th, I will wait until you give me 
an ok.

> 
> 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.
>