On 04/15/2013 12:51 PM, Tommie Gannert wrote:
> 2013/4/15 Tommie Gannert <tommie_at_spotify.com <mailto:tommie_at_spotify.com>>
>
> 2013/4/15 Patrick Valsecchi <pvalsecc_at_cisco.com
> <mailto:pvalsecc_at_cisco.com>>
>
> The first sub-string is fine, but with the second sub-string,
> the code in ares_parse_txt.c (c-ares version 1.9.1) will have
> a bad behavior. The loop line 146 will just compute a total
> length of 4+255=259 and the loop line 164 can have two
> possible outcomes: crash or possible information leak.
>
>
> Sounds reasonable. Adding a check that the last label ends on the
> rr_len boundary makes a lot of sense.
>
>
> Something like this:
>
> Subject: [PATCH] Check that TXT labels end on RR boundary in
> ares_parse_txt_reply().
>
> ---
> ares_parse_txt_reply.c | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/ares_parse_txt_reply.c b/ares_parse_txt_reply.c
> index dcc6473..106c3f8 100644
> --- a/ares_parse_txt_reply.c
> +++ b/ares_parse_txt_reply.c
> @@ -144,6 +144,12 @@ ares_parse_txt_reply (const unsigned char *abuf,
> int alen,
> strptr += substr_len + 1;
> }
> + if (strptr != (aptr + rr_len))
> + {
> + status = ARES_EBADRESP;
> + break;
> + }
> +
> /* Including null byte */
> txt_curr->txt = malloc (txt_curr->length + 1);
> if (txt_curr->txt == NULL)
>
Yes, your patch looks good.
I did a quick pass over the rest of the parsers to see if I spot other
problems like that. Here are my findings:
- ares_parse_a_reply: I have a feeling that rr_name is leaked if the
code reaches lines 157 or 168
- ares_parse_aaaa_reply: same suspicions
- ares_parse_*: when looping over each records, there is no check that
aptr+rr_len<abuf+alen before parsing the record (should be done just
after computing rr_len).
- in ares_expand_name.c: line 150 should use the >= operator instead of ==
Do you want a patch for all that? Who has the authority to accept patches?
Received on 2013-04-15