[v2] getaddrinfo: Fix use after free in getcanonname (CVE-2023-4806)
Checks
Context |
Check |
Description |
redhat-pt-bot/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
redhat-pt-bot/TryBot-32bit |
success
|
Build for i686
|
linaro-tcwg-bot/tcwg_glibc_check--master-arm |
fail
|
Testing failed
|
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 |
success
|
Testing passed
|
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 |
fail
|
Patch failed to apply
|
linaro-tcwg-bot/tcwg_glibc_build--master-arm |
fail
|
Patch failed to apply
|
Commit Message
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
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?
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
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.
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
>
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.
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
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?
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
@@ -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
new file mode 100644
@@ -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;
+}
new file mode 100644
@@ -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>
new file mode 100644
@@ -0,0 +1 @@
+#define QUERYNAME "test.example.com"
new file mode 100644
new file mode 100644
@@ -0,0 +1 @@
+cp $B/nss/libnss_test_gai_hv2_canonname.so $L/libnss_test_gai_hv2_canonname.so.2
@@ -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);