From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1bLbrJ-00074l-KY for mharc-grub-devel@gnu.org; Fri, 08 Jul 2016 15:54:45 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLbrH-00074f-Ty for grub-devel@gnu.org; Fri, 08 Jul 2016 15:54:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bLbrE-0003Bo-P9 for grub-devel@gnu.org; Fri, 08 Jul 2016 15:54:43 -0400 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:33275) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bLbrE-0003Bd-Bg for grub-devel@gnu.org; Fri, 08 Jul 2016 15:54:40 -0400 Received: by mail-lf0-x241.google.com with SMTP id l188so8415250lfe.0 for ; Fri, 08 Jul 2016 12:54:40 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to; bh=boJEDiGXuTjEHn6u14j2jwS1836v7P3EhR1qLI3dMq0=; b=O840JMpYfFMpELh3C386K41oph9MRARQZnxYbePQrR7OCaeUXR2fMsbCxcgCn6buVx uS+vj+d0+4se9NsuuJ2dyemBWxIJbT3Vo4FyBDifLs5kTPP6NbwVos3QW+kRvaPYShi7 uex0x8fHpUysfTv6xOGB7Fb+oA/G7LyiUHmgcbq4oDRvPMiM0LcaR72trLv7wOWv/eRK htwZgLFzfsafMYCMX618a6rDbEXCKTHd3jnA9r41UGGYhCb86xulCB1dCRICnfMQdNSy EP6+eV1WJA1LYd5827K9P3WNmjCe2u1epkcyjHbgQZMI1rTwyD4CgyIN0nk4KHPCgp9a bzKw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to; bh=boJEDiGXuTjEHn6u14j2jwS1836v7P3EhR1qLI3dMq0=; b=dxV5yK20vKnOm5bkFuuCKqRyBqKOI0suj5ETZ1ldYqg8+afjhorKHLlMrxRHUqLaVa TEEsFnA8hFZmM52khXk0n0VC495LN2yiKcQV3EhBZRsoRtLJL06rgf60rPmHD8pytNdG 2eGIhLfexexTiBmR//C0NhUUxiOirN/UGzFIyAxnIyayUwGNWB9cp/XEaakFDLiViGW0 9szh2Df7vqQDc0oyF2gAxtqcw6I82Q2apa8MzD0/XOczoPTqO0JB/qaN4k4VLUGQYZX5 Ahbn7W0yVEBcXOaegyZ5pOFaT6Cj+0DQciexJSwRx75WzAVDXVY5KtWrN9wVHrpVfVG8 AOyg== X-Gm-Message-State: ALyK8tI9W62ELcHLuzaXMxWAcxucuwvAaJplHmvXxNqwvum9mFLcotq/OSvbJNU3QIBNwA== X-Received: by 10.46.1.93 with SMTP id 90mr2026143ljb.1.1468007679037; Fri, 08 Jul 2016 12:54:39 -0700 (PDT) Received: from [192.168.1.44] (ppp109-252-91-231.pppoe.spdop.ru. [109.252.91.231]) by smtp.gmail.com with ESMTPSA id 39sm3007072lja.37.2016.07.08.12.54.37 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 08 Jul 2016 12:54:37 -0700 (PDT) Subject: Re: [PATCH] dns: fix heap corruption To: grub-devel@gnu.org References: <20160707091817.GA16571@linux-9gqx.suse.de> From: Andrei Borzenkov Message-ID: <578004FD.5020601@gmail.com> Date: Fri, 8 Jul 2016 22:54:37 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160707091817.GA16571@linux-9gqx.suse.de> Content-Type: multipart/mixed; boundary="------------000109030809090503060107" X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c07::241 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 08 Jul 2016 19:54:45 -0000 This is a multi-part message in MIME format. --------------000109030809090503060107 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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++) > --------------000109030809090503060107 Content-Type: text/x-patch; name="dns-heap-corruption.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="dns-heap-corruption.patch" From: Andrei Borzenkov 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) --------------000109030809090503060107--