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

[Carlos O'Donell <carlos@redhat.com>]

On 9/14/23 06:13, Siddhesh Poyarekar wrote:
> 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.

LGTM. I agree that getcanonname can end up reusing memory after we call gethosts
twice and via convert_hostent_to_gaih_addrtuple have set the first res->at->name
to a value that has been freed, and then we later try to use it again.

Tested-by: Carlos O'Donell <carlos@redhat.com>
Reviewed-by: Carlos O'Donell <carlos@redhat.com>

This commit message could explain a *lot* more about why and how we get the UAF,
but that is not a sustained objection here.

FAIL: nss/tst-nss-gai-hv2-canonname
original exit status 1
Didn't expect signal from child: got `Aborted'
running post-clean rsync

Test fails without the fix with an abort because the sizes are wrong.

Linaro TCWG pre-commit CI fails because it can't run containerized tests and all
the NSS container tests fail there, so this adds to the baseline.

Passes the Red Hat Platform Tools pre-commit CI since we can run limited
NSS containerized tests and this works.

> 
> This resolves BZ #30843, which is assigned CVE-2023-4806.
> 
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
> ---
> Changes from v2:
> - Added su to container script to make it explicit that the test runs as
>   root.

OK. This matters because we change the UID/GID in the mapping accordingly, but it doesn't
materially alter the results of thes testing. It is the correct thing to do though.

> 
> 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          |  2 +
>  sysdeps/posix/getaddrinfo.c                   | 26 +++++---
>  7 files changed, 153 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
> 
> 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 \

OK. Add new container test.

>  # 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

OK. Build a new extra test object.

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

OK. Add nss test module.

>  $(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)

OK. Add link deps to DSO build.

>  $(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)

OK. Make the final libnss provider for the test.

WARNING: The test case is technically underlinked, no sustained objection from me since we do
this often in tests where we know the files module is loaded first and the symbols are avialable.

e.g.
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
...
     5: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND _nss_files_gethostbyname2_r@GLIBC_PRIVATE (3)

>  $(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)

OK. Test depends on these.

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

OK.

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

OK.

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

OK.

> +
> +/* 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 *);

OK. External NSS files function.

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

OK. Returns files-based result via _nss_*_gethostbyname2_r as implementation of gethostbynam2_r

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

OK. Implemenst getcannonname_r.

> +{
> +  /* 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 ();

OK. Abort if the buffer is smaller.

> +
> +  strncpy (buffer, name, buflen);

OK. Make a copy.

> +  *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"

OK.

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

OK. Setup /etc/hosts in the container.

> +}
> +
> +static int
> +do_test (void)
> +{
> +  __nss_configure_lookup ("hosts", "test_gai_hv2_canonname");

OK. Configure lookup to use the module.

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

OK. Call getaddrinfo.

> +
> +  if (ret != 0)
> +    FAIL_EXIT1 ("getaddrinfo failed: %s\n", gai_strerror (ret));
> +
> +  TEST_COMPARE_STRING (result->ai_canonname, QUERYNAME);

OK.

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

OK.

> 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

OK. Copy in the DSO to lib dir.

> +su

OK. Become root.

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

OK. Adds hname pointer to gaih_result.

No ABI impact, structure is internal to getaddrinfo, but all internal callers have to be adjusted.

Slight preference for 'h_name' to match duped string, no sustained objection.

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

OK. Handle in gaih_result_reset() because hname needs to be freed.

>    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.  */

OK. Drop the restriction that h_name is not copied, and drop the restriction that hostent must
not be deallocated prematurely (whateve that actually means for the lifetime of the function call).

>  static bool
>  convert_hostent_to_gaih_addrtuple (const struct addrinfo *req, int family,
>  				   struct hostent *h, struct gaih_result *res)

OK. Incoming struct hostent has h_name, but that's not the problem. The problem is that the incoming
'struct hostent *h' is coming from either a call to nss_gethostname3_r, or __gethostbyname2_r
which will both store results in a buffer that can be resized.

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

OK. If we haven't copyed into res yet...

> +    {
> +      res->hname = __strdup (h->h_name);

... dup the name... (no longer matters if buffer is resized)

> +      if (res->hname == NULL)
> +	return false;

... but if we ran out of memory then fail the conversion.

> +    }
> +
>    /* 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;

OK. Don't put h_name into at[0].name, instead save the copy in the structure as hname.

In gethostbyname4_r implementations we put the fallback canonical name in res->at->name e.g.

464           (*pat)->next = NULL;
465           (*pat)->name = got_canon ? NULL : result.h_name;
466           got_canon = true;

Florian's v3 question was if we needed 'array[0].name = h->h_name' and the answer
is that we don't because we use res->at->name right away after calling gethostbyname4_r.

 668               if ((req->ai_flags & AI_CANONNAME) != 0 && res->canon == NULL)
 669                 {
 670                   char *canonbuf = __strdup (res->at->name);
 671                   if (canonbuf == NULL)
 672                     {
 673                       __resolv_context_put (res_ctx);
 674                       result = -EAI_MEMORY;
 675                       goto out;
 676                     }
 677                   res->canon = canonbuf;
 678                 }

We use res->canon directly, and we no longer need res->at->name.

In generate_addrinfo we use res->canon too.

And buffer management is the reason we have an explicit canon in 'struct gaih_result' like
we do for hname now.

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

OK. Internal getcannonname. Use const char* not struct gaih_addrtuple.

>  {
>    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,

OK. Instead of at[0]->name, use hname pased in directly.

OK. At this point we *might* have called gethosts twice and run out of buffer, but on the first
gethosts we filled things up and set res->at->name to garbage that after resizing it might be
gone, hence the UAF.

> +			      &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;
> +

Spurious space? No sustained objection.

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

OK. Pass res->hname directly, instead of res->at[0]->name indirectly.

>  		      if (canonbuf == NULL)
>  			{
>  			  __resolv_context_put (res_ctx);

-- 
Cheers,
Carlos.