diff mbox series

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

Message ID 20230913205628.3113433-1-siddhesh@sourceware.org
State New
Headers show
Series [v2] getaddrinfo: Fix use after free in getcanonname (CVE-2023-4806) | expand

Commit Message

Siddhesh Poyarekar Sept. 13, 2023, 8:56 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.
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>
---

Verified that reverting the fix still causes the test case to crash.

Changes from v1:
- Auto-generated hosts in PREPARE of the test.
- Added postclean.req

 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          |  1 +
 sysdeps/posix/getaddrinfo.c                   | 26 +++++---
 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

Comments

Andreas Schwab Sept. 14, 2023, 8:37 a.m. UTC | #1 
On Sep 13 2023, Siddhesh Poyarekar wrote:

> 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");

Doesn't that mean the test needs to be run as root?
Siddhesh Poyarekar Sept. 14, 2023, 9:48 a.m. UTC | #2 
On 2023-09-14 04:37, Andreas Schwab wrote:
>> +static void do_prepare (int a, char **av)
>> +{
>> +  FILE *hosts = xfopen ("/etc/hosts", "w");
> 
> Doesn't that mean the test needs to be run as root?
> 

Yes, that's why it's a container test.

Thanks,
Sid
Andreas Schwab Sept. 14, 2023, 9:55 a.m. UTC | #3 
On Sep 14 2023, Siddhesh Poyarekar wrote:

> On 2023-09-14 04:37, Andreas Schwab wrote:
>>> +static void do_prepare (int a, char **av)
>>> +{
>>> +  FILE *hosts = xfopen ("/etc/hosts", "w");
>> Doesn't that mean the test needs to be run as root?
>> 
>
> Yes, that's why it's a container test.

I don't see where it's enabled.
Siddhesh Poyarekar Sept. 14, 2023, 9:57 a.m. UTC | #4 
On 2023-09-14 05:55, Andreas Schwab wrote:
> On Sep 14 2023, Siddhesh Poyarekar wrote:
> 
>> On 2023-09-14 04:37, Andreas Schwab wrote:
>>>> +static void do_prepare (int a, char **av)
>>>> +{
>>>> +  FILE *hosts = xfopen ("/etc/hosts", "w");
>>> Doesn't that mean the test needs to be run as root?
>>>
>>
>> Yes, that's why it's a container test.
> 
> I don't see where it's enabled.
> 

Right here in nss/Makefile:

> --- 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
>
Andreas Schwab Sept. 14, 2023, 10 a.m. UTC | #5 
On Sep 14 2023, Siddhesh Poyarekar wrote:

> On 2023-09-14 05:55, Andreas Schwab wrote:
>> On Sep 14 2023, Siddhesh Poyarekar wrote:
>> 
>>> On 2023-09-14 04:37, Andreas Schwab wrote:
>>>>> +static void do_prepare (int a, char **av)
>>>>> +{
>>>>> +  FILE *hosts = xfopen ("/etc/hosts", "w");
>>>> Doesn't that mean the test needs to be run as root?
>>>>
>>>
>>> Yes, that's why it's a container test.
>> I don't see where it's enabled.
>> 
>
> Right here in nss/Makefile:

No, there is no root.
Siddhesh Poyarekar Sept. 14, 2023, 10:06 a.m. UTC | #6 
On 2023-09-14 06:00, Andreas Schwab wrote:
> On Sep 14 2023, Siddhesh Poyarekar wrote:
> 
>> On 2023-09-14 05:55, Andreas Schwab wrote:
>>> On Sep 14 2023, Siddhesh Poyarekar wrote:
>>>
>>>> On 2023-09-14 04:37, Andreas Schwab wrote:
>>>>>> +static void do_prepare (int a, char **av)
>>>>>> +{
>>>>>> +  FILE *hosts = xfopen ("/etc/hosts", "w");
>>>>> Doesn't that mean the test needs to be run as root?
>>>>>
>>>>
>>>> Yes, that's why it's a container test.
>>> I don't see where it's enabled.
>>>
>>
>> Right here in nss/Makefile:
> 
> No, there is no root.
> 

AFAICT, all container tests run as root within the container.  I can add 
su in the script file to make it explicit, but it's working without it 
at the moment.  Would you prefer that?

Thanks,
Sid
Andreas Schwab Sept. 14, 2023, 10:12 a.m. UTC | #7 
On Sep 14 2023, Siddhesh Poyarekar wrote:

> AFAICT, all container tests run as root within the container.

That's not what test-container implies, AFAICS.

> I can add su in the script file to make it explicit,

Why would that option exist if it is a no-op?
Siddhesh Poyarekar Sept. 14, 2023, 10:24 a.m. UTC | #8 
On 2023-09-14 06:12, Andreas Schwab wrote:
> On Sep 14 2023, Siddhesh Poyarekar wrote:
> 
>> AFAICT, all container tests run as root within the container.
> 
> That's not what test-container implies, AFAICS.
> 
>> I can add su in the script file to make it explicit,
> 
> Why would that option exist if it is a no-op?
> 

OK, so what seems to be happening here is that files in the container 
(at least the few I've tested right now) are owned by the executing 
user, so there's actually no need to run as root to modify them, which 
explains why the test works.

I've sent v3 anyway to make it kosher in case we end up fixing this in 
future.

Thanks,
Sid
diff mbox series

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..02ac31e52b
--- /dev/null
+++ b/nss/tst-nss-gai-hv2-canonname.root/tst-nss-gai-hv2-canonname.script
@@ -0,0 +1 @@ 
+cp $B/nss/libnss_test_gai_hv2_canonname.so $L/libnss_test_gai_hv2_canonname.so.2
diff --git a/sysdeps/posix/getaddrinfo.c b/sysdeps/posix/getaddrinfo.c
index 6ae6744fe4..eb5ba59dac 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 *hname;
   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->hname);
   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->hname == NULL)
+    {
+      res->hname = __strdup (h->h_name);
+      if (res->hname == 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;
@@ -740,6 +749,7 @@  get_nss_addresses (const char *name, const struct addrinfo *req,
 		    }
 		  no_inet6_data = no_data;
 		  inet6_status = status;
+
 		}
 	      if (req->ai_family == AF_INET
 		  || req->ai_family == AF_UNSPEC
@@ -771,7 +781,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->hname, name);
 		      if (canonbuf == NULL)
 			{
 			  __resolv_context_put (res_ctx);