[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] dns: fix heap corruption
From: |
Michael Chang |
Subject: |
Re: [PATCH] dns: fix heap corruption |
Date: |
Mon, 11 Jul 2016 14:02:59 +0800 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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