[PATCH] dns: fix heap corruption

[PATCH] dns: fix heap corruption

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

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++)
-- 
2.6.2

Re: [PATCH] dns: fix heap corruption

[Andrei Borzenkov]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

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.

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.

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


[-- Attachment #2: dns-heap-corruption.patch --]
[-- Type: text/x-patch, Size: 1321 bytes --]

From: Andrei Borzenkov <arvidjaar@gmail.com>
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)

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 <arvidjaar@gmail.com>
> 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
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH] dns: fix heap corruption

[Andrei Borzenkov]

11.07.2016 09:02, 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.
> 

Applied. Thanks!