[COMMITTED] getaddrinfo: Fix use after free in getcanonname (CVE-2023-4806)

Message ID 20230915184111.1677205-1-siddhesh@sourceware.org
State Committed
Commit 973fe93a5675c42798b2161c6f29c01b0e243994
Headers
Series [COMMITTED] getaddrinfo: Fix use after free in getcanonname (CVE-2023-4806) |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 warning Patch is already merged
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 warning Patch is already merged
linaro-tcwg-bot/tcwg_glibc_build--master-arm warning Patch is already merged
linaro-tcwg-bot/tcwg_glibc_check--master-arm warning Patch is already merged

Commit Message

Siddhesh Poyarekar Sept. 15, 2023, 6:41 p.m. UTC
  When an NSS plugin only implements the _gethostbyname2_r and
_getcanonname_r callbacks, getaddrinfo could use memory that was freed
during tmpbuf resizing, through h_name in a previous query response.

The backing store for res->at->name when doing a query with
gethostbyname3_r or gethostbyname2_r is tmpbuf, which is reallocated in
gethosts during the query.  For AF_INET6 lookup with AI_ALL |
AI_V4MAPPED, gethosts gets called twice, once for a v6 lookup and second
for a v4 lookup.  In this case, if the first call reallocates tmpbuf
enough number of times, resulting in a malloc, th->h_name (that
res->at->name refers to) ends up on a heap allocated storage in tmpbuf.
Now if the second call to gethosts also causes the plugin callback to
return NSS_STATUS_TRYAGAIN, tmpbuf will get freed, resulting in a UAF
reference in res->at->name.  This then gets dereferenced in the
getcanonname_r plugin call, resulting in the use after free.

Fix this by copying h_name over and freeing it at the end.  This
resolves BZ #30843, which is assigned CVE-2023-4806.

Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
---

Fixed up nits pointed out by Carlos and pushed.

 nss/Makefile                                  | 15 ++++-
 nss/nss_test_gai_hv2_canonname.c              | 56 +++++++++++++++++
 nss/tst-nss-gai-hv2-canonname.c               | 63 +++++++++++++++++++
 nss/tst-nss-gai-hv2-canonname.h               |  1 +
 .../postclean.req                             |  0
 .../tst-nss-gai-hv2-canonname.script          |  2 +
 sysdeps/posix/getaddrinfo.c                   | 25 +++++---
 7 files changed, 152 insertions(+), 10 deletions(-)
 create mode 100644 nss/nss_test_gai_hv2_canonname.c
 create mode 100644 nss/tst-nss-gai-hv2-canonname.c
 create mode 100644 nss/tst-nss-gai-hv2-canonname.h
 create mode 100644 nss/tst-nss-gai-hv2-canonname.root/postclean.req
 create mode 100644 nss/tst-nss-gai-hv2-canonname.root/tst-nss-gai-hv2-canonname.script
  

Patch

diff --git a/nss/Makefile b/nss/Makefile
index 06fcdc450f..8a5126ecf3 100644
--- a/nss/Makefile
+++ b/nss/Makefile
@@ -82,6 +82,7 @@  tests-container := \
   tst-nss-test3 \
   tst-reload1 \
   tst-reload2 \
+  tst-nss-gai-hv2-canonname \
 # tests-container
 
 # Tests which need libdl
@@ -145,7 +146,8 @@  libnss_compat-inhibit-o	= $(filter-out .os,$(object-suffixes))
 ifeq ($(build-static-nss),yes)
 tests-static		+= tst-nss-static
 endif
-extra-test-objs		+= nss_test1.os nss_test2.os nss_test_errno.os
+extra-test-objs		+= nss_test1.os nss_test2.os nss_test_errno.os \
+			   nss_test_gai_hv2_canonname.os
 
 include ../Rules
 
@@ -180,12 +182,16 @@  rtld-tests-LDFLAGS += -Wl,--dynamic-list=nss_test.ver
 libof-nss_test1 = extramodules
 libof-nss_test2 = extramodules
 libof-nss_test_errno = extramodules
+libof-nss_test_gai_hv2_canonname = extramodules
 $(objpfx)/libnss_test1.so: $(objpfx)nss_test1.os $(link-libc-deps)
 	$(build-module)
 $(objpfx)/libnss_test2.so: $(objpfx)nss_test2.os $(link-libc-deps)
 	$(build-module)
 $(objpfx)/libnss_test_errno.so: $(objpfx)nss_test_errno.os $(link-libc-deps)
 	$(build-module)
+$(objpfx)/libnss_test_gai_hv2_canonname.so: \
+  $(objpfx)nss_test_gai_hv2_canonname.os $(link-libc-deps)
+	$(build-module)
 $(objpfx)nss_test2.os : nss_test1.c
 # Use the nss_files suffix for these objects as well.
 $(objpfx)/libnss_test1.so$(libnss_files.so-version): $(objpfx)/libnss_test1.so
@@ -195,10 +201,14 @@  $(objpfx)/libnss_test2.so$(libnss_files.so-version): $(objpfx)/libnss_test2.so
 $(objpfx)/libnss_test_errno.so$(libnss_files.so-version): \
   $(objpfx)/libnss_test_errno.so
 	$(make-link)
+$(objpfx)/libnss_test_gai_hv2_canonname.so$(libnss_files.so-version): \
+  $(objpfx)/libnss_test_gai_hv2_canonname.so
+	$(make-link)
 $(patsubst %,$(objpfx)%.out,$(tests) $(tests-container)) : \
 	$(objpfx)/libnss_test1.so$(libnss_files.so-version) \
 	$(objpfx)/libnss_test2.so$(libnss_files.so-version) \
-	$(objpfx)/libnss_test_errno.so$(libnss_files.so-version)
+	$(objpfx)/libnss_test_errno.so$(libnss_files.so-version) \
+	$(objpfx)/libnss_test_gai_hv2_canonname.so$(libnss_files.so-version)
 
 ifeq (yes,$(have-thread-library))
 $(objpfx)tst-cancel-getpwuid_r: $(shared-thread-library)
@@ -215,3 +225,4 @@  LDFLAGS-tst-nss-test3 = -Wl,--disable-new-dtags
 LDFLAGS-tst-nss-test4 = -Wl,--disable-new-dtags
 LDFLAGS-tst-nss-test5 = -Wl,--disable-new-dtags
 LDFLAGS-tst-nss-test_errno = -Wl,--disable-new-dtags
+LDFLAGS-tst-nss-test_gai_hv2_canonname = -Wl,--disable-new-dtags
diff --git a/nss/nss_test_gai_hv2_canonname.c b/nss/nss_test_gai_hv2_canonname.c
new file mode 100644
index 0000000000..4439c83c9f
--- /dev/null
+++ b/nss/nss_test_gai_hv2_canonname.c
@@ -0,0 +1,56 @@ 
+/* NSS service provider that only provides gethostbyname2_r.
+   Copyright The GNU Toolchain Authors.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <nss.h>
+#include <stdlib.h>
+#include <string.h>
+#include "nss/tst-nss-gai-hv2-canonname.h"
+
+/* Catch misnamed and functions.  */
+#pragma GCC diagnostic error "-Wmissing-prototypes"
+NSS_DECLARE_MODULE_FUNCTIONS (test_gai_hv2_canonname)
+
+extern enum nss_status _nss_files_gethostbyname2_r (const char *, int,
+						    struct hostent *, char *,
+						    size_t, int *, int *);
+
+enum nss_status
+_nss_test_gai_hv2_canonname_gethostbyname2_r (const char *name, int af,
+					      struct hostent *result,
+					      char *buffer, size_t buflen,
+					      int *errnop, int *herrnop)
+{
+  return _nss_files_gethostbyname2_r (name, af, result, buffer, buflen, errnop,
+				      herrnop);
+}
+
+enum nss_status
+_nss_test_gai_hv2_canonname_getcanonname_r (const char *name, char *buffer,
+					    size_t buflen, char **result,
+					    int *errnop, int *h_errnop)
+{
+  /* We expect QUERYNAME, which is a small enough string that it shouldn't fail
+     the test.  */
+  if (memcmp (QUERYNAME, name, sizeof (QUERYNAME))
+      || buflen < sizeof (QUERYNAME))
+    abort ();
+
+  strncpy (buffer, name, buflen);
+  *result = buffer;
+  return NSS_STATUS_SUCCESS;
+}
diff --git a/nss/tst-nss-gai-hv2-canonname.c b/nss/tst-nss-gai-hv2-canonname.c
new file mode 100644
index 0000000000..d5f10c07d6
--- /dev/null
+++ b/nss/tst-nss-gai-hv2-canonname.c
@@ -0,0 +1,63 @@ 
+/* Test NSS query path for plugins that only implement gethostbyname2
+   (#30843).
+   Copyright The GNU Toolchain Authors.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <nss.h>
+#include <netdb.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/xstdio.h>
+#include "nss/tst-nss-gai-hv2-canonname.h"
+
+#define PREPARE do_prepare
+
+static void do_prepare (int a, char **av)
+{
+  FILE *hosts = xfopen ("/etc/hosts", "w");
+  for (unsigned i = 2; i < 255; i++)
+    {
+      fprintf (hosts, "ff01::ff02:ff03:%u:2\ttest.example.com\n", i);
+      fprintf (hosts, "192.168.0.%u\ttest.example.com\n", i);
+    }
+  xfclose (hosts);
+}
+
+static int
+do_test (void)
+{
+  __nss_configure_lookup ("hosts", "test_gai_hv2_canonname");
+
+  struct addrinfo hints = {};
+  struct addrinfo *result = NULL;
+
+  hints.ai_family = AF_INET6;
+  hints.ai_flags = AI_ALL | AI_V4MAPPED | AI_CANONNAME;
+
+  int ret = getaddrinfo (QUERYNAME, NULL, &hints, &result);
+
+  if (ret != 0)
+    FAIL_EXIT1 ("getaddrinfo failed: %s\n", gai_strerror (ret));
+
+  TEST_COMPARE_STRING (result->ai_canonname, QUERYNAME);
+
+  freeaddrinfo(result);
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/nss/tst-nss-gai-hv2-canonname.h b/nss/tst-nss-gai-hv2-canonname.h
new file mode 100644
index 0000000000..14f2a9cb08
--- /dev/null
+++ b/nss/tst-nss-gai-hv2-canonname.h
@@ -0,0 +1 @@ 
+#define QUERYNAME "test.example.com"
diff --git a/nss/tst-nss-gai-hv2-canonname.root/postclean.req b/nss/tst-nss-gai-hv2-canonname.root/postclean.req
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/nss/tst-nss-gai-hv2-canonname.root/tst-nss-gai-hv2-canonname.script b/nss/tst-nss-gai-hv2-canonname.root/tst-nss-gai-hv2-canonname.script
new file mode 100644
index 0000000000..31848b4a28
--- /dev/null
+++ b/nss/tst-nss-gai-hv2-canonname.root/tst-nss-gai-hv2-canonname.script
@@ -0,0 +1,2 @@ 
+cp $B/nss/libnss_test_gai_hv2_canonname.so $L/libnss_test_gai_hv2_canonname.so.2
+su
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 6ae6744fe4..47f421fddf 100644
--- a/sysdeps/posix/getaddrinfo.c
+++ b/sysdeps/posix/getaddrinfo.c
@@ -120,6 +120,7 @@  struct gaih_result
 {
   struct gaih_addrtuple *at;
   char *canon;
+  char *h_name;
   bool free_at;
   bool got_ipv6;
 };
@@ -165,6 +166,7 @@  gaih_result_reset (struct gaih_result *res)
   if (res->free_at)
     free (res->at);
   free (res->canon);
+  free (res->h_name);
   memset (res, 0, sizeof (*res));
 }
 
@@ -203,9 +205,8 @@  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.  The new addresses are appended to the tuple array in RES.  */
+/* Convert struct hostent to a list of struct gaih_addrtuple objects.  The new
+   addresses are appended to the tuple array in RES.  */
 static bool
 convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, int family,
 				   struct hostent *h, struct gaih_result *res)
@@ -238,6 +239,15 @@  convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, int family,
   res->at = array;
   res->free_at = true;
 
+  /* Duplicate h_name because it may get reclaimed when the underlying storage
+     is freed.  */
+  if (res->h_name == NULL)
+    {
+      res->h_name = __strdup (h->h_name);
+      if (res->h_name == NULL)
+	return false;
+    }
+
   /* Update the next pointers on reallocation.  */
   for (size_t i = 0; i < old; i++)
     array[i].next = array + i + 1;
@@ -262,7 +272,6 @@  convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, int family,
 	}
       array[i].next = array + i + 1;
     }
-  array[0].name = h->h_name;
   array[count - 1].next = NULL;
 
   return true;
@@ -324,15 +333,15 @@  gethosts (nss_gethostbyname3_r fct, int family, const char *name,
    memory allocation failure.  The returned string is allocated on the
    heap; the caller has to free it.  */
 static char *
-getcanonname (nss_action_list nip, struct gaih_addrtuple *at, const char *name)
+getcanonname (nss_action_list nip, const char *hname, const char *name)
 {
   nss_getcanonname_r *cfct = __nss_lookup_function (nip, "getcanonname_r");
   char *s = (char *) name;
   if (cfct != NULL)
     {
       char buf[256];
-      if (DL_CALL_FCT (cfct, (at->name ?: name, buf, sizeof (buf),
-			      &s, &errno, &h_errno)) != NSS_STATUS_SUCCESS)
+      if (DL_CALL_FCT (cfct, (hname ?: name, buf, sizeof (buf), &s, &errno,
+			      &h_errno)) != NSS_STATUS_SUCCESS)
 	/* If the canonical name cannot be determined, use the passed
 	   string.  */
 	s = (char *) name;
@@ -771,7 +780,7 @@  get_nss_addresses (const char *name, const struct addrinfo *req,
 		  if ((req->ai_flags & AI_CANONNAME) != 0
 		      && res->canon == NULL)
 		    {
-		      char *canonbuf = getcanonname (nip, res->at, name);
+		      char *canonbuf = getcanonname (nip, res->h_name, name);
 		      if (canonbuf == NULL)
 			{
 			  __resolv_context_put (res_ctx);