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

[Mike Frysinger <vapier at gentoo dot org>]

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.

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"

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

"particularly"

> +     [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

> +      /* Do not return a truncated second response (unless it was
> +         unavoidable e.g. unrecoverable TRYAGAIN).  */

2nd line should use a tab

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

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

"field"

> +	/* 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.
-mike

Attachment: signature.asc
Description: Digital signature