CVE-2016-3706: getaddrinfo: stack overflow in hostent conversion [BZ #20010]

When converting a struct hostent response to struct gaih_addrtuple, the
gethosts macro (which is called from gaih_inet) used alloca, without
malloc fallback for large responses.  This commit changes this code to
use calloc unconditionally.

This commit also consolidated a second hostent-to-gaih_addrtuple
conversion loop (in gaih_inet) to use the new conversion function.

diff --git a/ChangeLog b/ChangeLog
index 1a6ad84..768c0a0 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,15 @@
 2016-04-29  Florian Weimer  <fweimer@redhat.com>
 
+       [BZ #20010]
+       CVE-2016-3706
+       * sysdeps/posix/getaddrinfo.c
+       (convert_hostent_to_gaih_addrtuple): New function.
+       (gethosts): Call convert_hostent_to_gaih_addrtuple.
+       (gaih_inet): Use convert_hostent_to_gaih_addrtuple to convert
+       AF_INET data.
+
+2016-04-29  Florian Weimer  <fweimer@redhat.com>
+
        glob: Simplify and document the interface for the GLOB_ALTDIRFUNC
        callback function gl_readdir.
        * posix/glob.c (NAMELEN, CONVERT_D_NAMLEN): Remove.

diff --git a/NEWS b/NEWS
index 54ffb02..aa6209e 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -27,7 +27,10 @@ Version 2.24
 
 Security related changes:
 
-  [Add security related changes here]
+* Previously, getaddrinfo copied large amounts of address data to the stack,
+  even after the fix for CVE-2013-4458 has been applied, potentially
+  resulting in a stack overflow.  getaddrinfo now uses a heap allocation
+  instead.  Reported by Michael Petlan.  (CVE-2016-3706)
 
 The following bugs are resolved with this release:
 

diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 1ef3f20..fed2d3b 100644 (file)
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -168,9 +168,58 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
   return 0;
 }
 
+/* Convert struct hostent to a list of struct gaih_addrtuple objects.
+   h_name is not copied, and the struct hostent object must not be
+   deallocated prematurely.  *RESULT must be NULL or a pointer to an
+   object allocated using malloc, which is freed.  */
+static bool
+convert_hostent_to_gaih_addrtuple (const struct addrinfo *req,
+                                  int family,
+                                  struct hostent *h,
+                                  struct gaih_addrtuple **result)
+{
+  free (*result);
+  *result = NULL;
+
+  /* Count the number of addresses in h->h_addr_list.  */
+  size_t count = 0;
+  for (char **p = h->h_addr_list; *p != NULL; ++p)
+    ++count;
+
+  /* Report no data if no addresses are available, or if the incoming
+     address size is larger than what we can store.  */
+  if (count == 0 || h->h_length > sizeof (((struct gaih_addrtuple) {}).addr))
+    return true;
+
+  struct gaih_addrtuple *array = calloc (count, sizeof (*array));
+  if (array == NULL)
+    return false;
+
+  for (size_t i = 0; i < count; ++i)
+    {
+      if (family == AF_INET && req->ai_family == AF_INET6)
+       {
+         /* Perform address mapping. */
+         array[i].family = AF_INET6;
+         memcpy(array[i].addr + 3, h->h_addr_list[i], sizeof (uint32_t));
+         array[i].addr[2] = htonl (0xffff);
+       }
+      else
+       {
+         array[i].family = family;
+         memcpy (array[i].addr, h->h_addr_list[i], h->h_length);
+       }
+      array[i].next = array + i + 1;
+    }
+  array[0].name = h->h_name;
+  array[count - 1].next = NULL;
+
+  *result = array;
+  return true;
+}
+
 #define gethosts(_family, _type) \
  {                                                                           \
-  int i;                                                                     \
   int herrno;                                                                \
   struct hostent th;                                                         \
   struct hostent *h;                                                         \
@@ -219,36 +268,23 @@ gaih_inet_serv (const char *servicename, const struct gaih_typeproto *tp,
     }                                                                        \
   else if (h != NULL)                                                        \
     {                                                                        \
-      for (i = 0; h->h_addr_list[i]; i++)                                    \
+      /* Make sure that addrmem can be freed.  */                            \
+      if (!malloc_addrmem)                                                   \
+       addrmem = NULL;                                                       \
+      if (!convert_hostent_to_gaih_addrtuple (req, _family,h, &addrmem))      \
        {                                                                     \
-         if (*pat == NULL)                                                   \
-           {                                                                 \
-             *pat = __alloca (sizeof (struct gaih_addrtuple));               \
-             (*pat)->scopeid = 0;                                            \
-           }                                                                 \
-         uint32_t *addr = (*pat)->addr;                                      \
-         (*pat)->next = NULL;                                                \
-         (*pat)->name = i == 0 ? strdupa (h->h_name) : NULL;                 \
-         if (_family == AF_INET && req->ai_family == AF_INET6)               \
-           {                                                                 \
-             (*pat)->family = AF_INET6;                                      \
-             addr[3] = *(uint32_t *) h->h_addr_list[i];                      \
-             addr[2] = htonl (0xffff);                                       \
-             addr[1] = 0;                                                    \
-             addr[0] = 0;                                                    \
-           }                                                                 \
-         else                                                                \
-           {                                                                 \
-             (*pat)->family = _family;                                       \
-             memcpy (addr, h->h_addr_list[i], sizeof(_type));                \
-           }                                                                 \
-         pat = &((*pat)->next);                                              \
+         _res.options |= old_res_options & RES_USE_INET6;                    \
+         result = -EAI_SYSTEM;                                               \
+         goto free_and_return;                                               \
        }                                                                     \
+      *pat = addrmem;                                                        \
+      /* The conversion uses malloc unconditionally.  */                     \
+      malloc_addrmem = true;                                                 \
                                                                              \
       if (localcanon !=        NULL && canon == NULL)                                \
        canon = strdupa (localcanon);                                         \
                                                                              \
-      if (_family == AF_INET6 && i > 0)                                              \
+      if (_family == AF_INET6 && *pat != NULL)                               \
        got_ipv6 = true;                                                      \
     }                                                                        \
  }
@@ -612,44 +648,16 @@ gaih_inet (const char *name, const struct gaih_service *service,
                {
                  if (h != NULL)
                    {
-                     int i;
-                     /* We found data, count the number of addresses.  */
-                     for (i = 0; h->h_addr_list[i]; ++i)
-                       ;
-                     if (i > 0 && *pat != NULL)
-                       --i;
-
-                     if (__libc_use_alloca (alloca_used
-                                            + i * sizeof (struct gaih_addrtuple)))
-                       addrmem = alloca_account (i * sizeof (struct gaih_addrtuple),
-                                                 alloca_used);
-                     else
-                       {
-                         addrmem = malloc (i
-                                           * sizeof (struct gaih_addrtuple));
-                         if (addrmem == NULL)
-                           {
-                             result = -EAI_MEMORY;
-                             goto free_and_return;
-                           }
-                         malloc_addrmem = true;
-                       }
-
-                     /* Now convert it into the list.  */
-                     struct gaih_addrtuple *addrfree = addrmem;
-                     for (i = 0; h->h_addr_list[i]; ++i)
+                     /* We found data, convert it.  */
+                     if (!convert_hostent_to_gaih_addrtuple
+                         (req, AF_INET, h, &addrmem))
                        {
-                         if (*pat == NULL)
-                           {
-                             *pat = addrfree++;
-                             (*pat)->scopeid = 0;
-                           }
-                         (*pat)->next = NULL;
-                         (*pat)->family = AF_INET;
-                         memcpy ((*pat)->addr, h->h_addr_list[i],
-                                 h->h_length);
-                         pat = &((*pat)->next);
+                         result = -EAI_MEMORY;
+                         goto free_and_return;
                        }
+                     *pat = addrmem;
+                     /* The conversion uses malloc unconditionally.  */
+                     malloc_addrmem = true;
                    }
                }
              else