Re: [PATCH] dns: fix heap corruption

[Michael Chang]

On Fri, Jul 08, 2016 at 10:54:37PM +0300, Andrei Borzenkov wrote:
> 07.07.2016 12:18, Michael Chang пишет:
> > Since commit f9d1b4422efb2c06e5472fb2c304712e2029796b I occasionally bumped
> > into heap corruption problem during dns lookup.
> > 
> > After tracing the issue, it looks the *data->addresses array is not 
> > correctly
> > allocated. It need to hold accumulated dns look up result but not only the 
> > new
> > result in new message. The heap corruption occured when appending new 
> > result to
> > it.
> > 
> > This patch fixed the issue for me by reallocating the array if it found too
> > small to hold all the result. 
> > 
> 
> I'm not sure. I think we discussed this with Josef back then. The code
> apparently was assuming single response; and if we are going to collect
> multiple answers, we need to filter out duplicates at least and also not
> depend on packet order to select between A and AAAA.

OK.

> 
> Does attached patch fix corruption for you? I think that is the least
> intrusive as bug fix, and we need to revisit code to properly handle
> multiple responses later.

Yes, it does. I have tested several times to make sure it doesn't happen.

Thanks for review.

Michael

> 
> > Thanks,
> > 
> > ---
> >  grub-core/net/dns.c | 23 +++++++++++++++++------
> >  1 file changed, 17 insertions(+), 6 deletions(-)
> > 
> > diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
> > index 89741dd..b8d8873 100644
> > --- a/grub-core/net/dns.c
> > +++ b/grub-core/net/dns.c
> > @@ -276,14 +276,25 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ 
> > ((unused)),
> >        ptr++;
> >        ptr += 4;
> >      }
> > -  *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
> > -                            * grub_be_to_cpu16 (head->ancount));
> > -  if (!*data->addresses)
> > +
> > +  if (ALIGN_UP (grub_be_to_cpu16 (head->ancount) + *data->naddresses, 4) > 
> > ALIGN_UP (*data->naddresses, 4))
> >      {
> > -      grub_errno = GRUB_ERR_NONE;
> > -      grub_netbuff_free (nb);
> > -      return GRUB_ERR_NONE;
> > +      grub_net_network_level_address_t *old_addresses = *data->addresses;
> > +      *data->addresses = grub_malloc (sizeof ((*data->addresses)[0])
> > +                            * ALIGN_UP (grub_be_to_cpu16 (head->ancount) + 
> > *data->naddresses, 4));
> > +      if (!*data->addresses)
> > +   {
> > +     grub_errno = GRUB_ERR_NONE;
> > +     grub_netbuff_free (nb);
> > +     return GRUB_ERR_NONE;
> > +   }
> > +      if (*data->naddresses)
> > +   {
> > +     grub_memcpy (*data->addresses, old_addresses, sizeof 
> > ((*data->addresses)[0]) * (*data->naddresses));
> > +     grub_free (old_addresses);
> > +   }
> >      }
> > +
> >    reparse_ptr = ptr;
> >   reparse:
> >    for (i = 0, ptr = reparse_ptr; i < grub_be_to_cpu16 (head->ancount); i++)
> > 
> 

> From: Andrei Borzenkov <address@hidden>
> Subject: [PATCH] dns: out of bounds for data->addresses in recv_hook
> 
> We may get more than one response before exiting out of loop in
> grub_net_dns_lookup. We never really use more than the very first
> addresses during lookup so there is little point in collecting all
> of them. Just quit early if we already have some reply.
> 
> Code needs serious redesign to actually collect multiple answers
> and select the best fit according to requested type (IPv4 nr IPv6).
> 
> ---
>  grub-core/net/dns.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/grub-core/net/dns.c b/grub-core/net/dns.c
> index 89741dd..5d9afe0 100644
> --- a/grub-core/net/dns.c
> +++ b/grub-core/net/dns.c
> @@ -238,6 +238,15 @@ recv_hook (grub_net_udp_socket_t sock __attribute__ 
> ((unused)),
>    char *redirect_save = NULL;
>    grub_uint32_t ttl_all = ~0U;
>  
> +  /* Code apparently assumed that only one packet is received as response.
> +     We may get multiple responses due to network condition, so check here
> +     and quit early. */
> +  if (*data->addresses)
> +    {
> +      grub_netbuff_free (nb);
> +      return GRUB_ERR_NONE;
> +    }
> +
>    head = (struct dns_header *) nb->data;
>    ptr = (grub_uint8_t *) (head + 1);
>    if (ptr >= nb->tail)
> -- 
> tg: (b524fa2..) u/dns-fix-naddresses-out-of-bounds (depends on: master)

> _______________________________________________
> Grub-devel mailing list
> address@hidden
> https://lists.gnu.org/mailman/listinfo/grub-devel