From 239903d44c12f10b5fe7c1f8457ae5203e47d8cc Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sat, 27 Dec 2025 15:24:45 +0100 Subject: [PATCH 1/3] nss-systemd: set sg_adm/sg_mem in intrinsic groups The sg_adm and sg_mem fields are supposed to point to a NULL terminated string array. If these are NULL, some foreign tools like shadow's sg trigger NULL pointer dereferences (or fortunately their asset() calls). --- src/nss-systemd/nss-systemd.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/src/nss-systemd/nss-systemd.c b/src/nss-systemd/nss-systemd.c index ae0b5c803d..11384dbad8 100644 --- a/src/nss-systemd/nss-systemd.c +++ b/src/nss-systemd/nss-systemd.c @@ -75,6 +75,8 @@ static const struct group root_group = { static const struct sgrp root_sgrp = { .sg_namp = (char*) "root", .sg_passwd = (char*) PASSWORD_LOCKED_AND_INVALID, + .sg_adm = (char*[]) { NULL }, + .sg_mem = (char*[]) { NULL }, }; static const struct group nobody_group = { @@ -87,6 +89,8 @@ static const struct group nobody_group = { static const struct sgrp nobody_sgrp = { .sg_namp = (char*) NOBODY_GROUP_NAME, .sg_passwd = (char*) PASSWORD_LOCKED_AND_INVALID, + .sg_adm = (char*[]) { NULL }, + .sg_mem = (char*[]) { NULL }, }; typedef struct GetentData { @@ -257,12 +261,18 @@ static enum nss_status copy_synthesized_sgrp( assert(src); assert(src->sg_namp); assert(src->sg_passwd); + assert(src->sg_adm); + assert(!*src->sg_adm); /* Our synthesized records' sg_adm is always just NULL... */ + assert(src->sg_mem); + assert(!*src->sg_mem); /* Our synthesized records' sg_mem is always just NULL... */ size_t required = strlen(src->sg_namp) + 1 + - strlen(src->sg_passwd) + 1; + strlen(src->sg_passwd) + 1 + + sizeof(char*) + /* NULL terminator storage for src->sg_adm */ + sizeof(char*); /* NULL terminator storage for src->sg_mem */ - if (buflen < required) { + if (buflen < ALIGN(required)) { *errnop = ERANGE; return NSS_STATUS_TRYAGAIN; } @@ -274,7 +284,10 @@ static enum nss_status copy_synthesized_sgrp( /* String fields point into the user-provided buffer */ dest->sg_namp = buffer; dest->sg_passwd = stpcpy(dest->sg_namp, src->sg_namp) + 1; - strcpy(dest->sg_passwd, src->sg_passwd); + dest->sg_adm = ALIGN_PTR(stpcpy(dest->sg_passwd, src->sg_passwd) + 1); + *dest->sg_adm = NULL; + dest->sg_mem = dest->sg_adm + 1; + *dest->sg_mem = NULL; return NSS_STATUS_SUCCESS; } From 57682793dac269994d6e69f7a5a937f5ad459cc8 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sat, 27 Dec 2025 15:27:05 +0100 Subject: [PATCH 2/3] nss-systemd: set sg_adm/sg_mem for all groups Fill sg_adm and sg_mem in nss_pack_group_record_shadow to stay compatible with other NSS getsgnam implementations which set these members to NULL terminated string arrays. Tools like shadow's sg would trigger a NULL pointer dereference with groups only found through nss-systemd otherwise. --- src/nss-systemd/userdb-glue.c | 56 +++++++++++++++++++++++++++++++---- 1 file changed, 51 insertions(+), 5 deletions(-) diff --git a/src/nss-systemd/userdb-glue.c b/src/nss-systemd/userdb-glue.c index 70f0ecfbd5..1d5e311ce8 100644 --- a/src/nss-systemd/userdb-glue.c +++ b/src/nss-systemd/userdb-glue.c @@ -411,6 +411,40 @@ enum nss_status userdb_getgrgid( return NSS_STATUS_SUCCESS; } +/* Counts string pointers (including terminating NULL element) of given + * string vector strv and stores amount of pointers in n and total + * length of all contained strings including NUL bytes in len. */ +static void nss_count_strv(char * const *strv, size_t *n, size_t *len) { + STRV_FOREACH(str, strv) { + (*len) += sizeof(char*); /* space for array entry */ + (*len) += strlen(*str) + 1; + (*n)++; + } + (*len) += sizeof(char*); /* trailing NULL in array entry */ + (*n)++; +} + +/* Performs deep copy of given string vector src and stores content + * of contained strings into buf with references to these strings + * in dst. At dst location, a new NULL-terminated string vector is + * created. The dst and buf locations are updated to point just behind + * the last pointer or char respectively. Returns total amount of + * pointers in newly created string vector in dst, including the + * terminating NULL element. */ +static size_t nss_deep_copy_strv(char * const *src, char ***dst, char **buf) { + char *p = *buf; + size_t i = 0; + + STRV_FOREACH(str, src) { + (*dst)[i++] = p; + p = stpcpy(p, *str) + 1; + } + (*dst)[i++] = NULL; + *dst += i; + *buf = p; + return i; +} + int nss_pack_group_record_shadow( GroupRecord *hr, struct sgrp *sgrp, @@ -418,7 +452,8 @@ int nss_pack_group_record_shadow( size_t buflen) { const char *hashed; - size_t required; + char **array = NULL, *p; + size_t i = 0, n = 0, required; assert(hr); assert(sgrp); @@ -429,15 +464,26 @@ int nss_pack_group_record_shadow( assert_se(hashed = strv_isempty(hr->hashed_password) ? PASSWORD_LOCKED_AND_INVALID : hr->hashed_password[0]); required += strlen(hashed) + 1; + nss_count_strv(hr->administrators, &n, &required); + nss_count_strv(hr->members, &n, &required); + if (buflen < required) return -ERANGE; - *sgrp = (struct sgrp) { - .sg_namp = buffer, - }; - assert(buffer); + p = buffer + sizeof(void*) * (n + 1); /* place member strings right after the ptr array */ + array = (char**) buffer; /* place ptr array at beginning of buffer, under assumption buffer is aligned */ + + sgrp->sg_mem = array; + i += nss_deep_copy_strv(hr->members, &array, &p); + + sgrp->sg_adm = array; + i += nss_deep_copy_strv(hr->administrators, &array, &p); + + assert_se(i == n); + + sgrp->sg_namp = p; sgrp->sg_passwd = stpcpy(sgrp->sg_namp, hr->group_name) + 1; strcpy(sgrp->sg_passwd, hashed); From 2eaca3ea5f0de702382d388d3bbc753c000ada4b Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 4 Jan 2026 12:19:22 +0100 Subject: [PATCH 3/3] nss-systemd: add unit test for sg_adm/sg_mem Add a test for getsgnam_r to verify that sg_adm and sg_mem always point to a NULL-terminated string vector. Extend the gr_mem check of struct group for non-NULL values as well. --- src/shared/nss-util.h | 7 ++++++ src/test/test-nss-users.c | 45 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/src/shared/nss-util.h b/src/shared/nss-util.h index c182237368..8f9c4e1ff3 100644 --- a/src/shared/nss-util.h +++ b/src/shared/nss-util.h @@ -2,6 +2,7 @@ #pragma once #include +#include #include #include #include @@ -291,3 +292,9 @@ typedef enum nss_status (*_nss_getgrgid_r_t)( struct group *gr, char *buffer, size_t buflen, int *errnop); + +typedef enum nss_status (*_nss_getsgnam_r_t)( + const char *name, + struct sgrp *sg, + char *buffer, size_t buflen, + int *errnop); diff --git a/src/test/test-nss-users.c b/src/test/test-nss-users.c index f253b53c1f..b9078f745e 100644 --- a/src/test/test-nss-users.c +++ b/src/test/test-nss-users.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: LGPL-2.1-or-later */ +#include #include #include @@ -35,7 +36,22 @@ static void print_struct_group(const struct group *gr) { gr->gr_name, gr->gr_gid); log_info(" passwd=\"%s\"", gr->gr_passwd); - assert_se(members = strv_join(gr->gr_mem, ", ")); + assert_se(members = strv_join(ASSERT_PTR(gr->gr_mem), ", ")); + // FIXME: use shell_maybe_quote(SHELL_ESCAPE_EMPTY) when it becomes available + log_info(" members=%s", members); +} + +static void print_struct_sgrp(const struct sgrp *sg) { + _cleanup_free_ char *administrators = NULL, *members = NULL; + + log_info(" \"%s\"", sg->sg_namp); + log_info(" passwd=\"%s\"", sg->sg_passwd); + + assert_se(administrators = strv_join(ASSERT_PTR(sg->sg_adm), ", ")); + // FIXME: use shell_maybe_quote(SHELL_ESCAPE_EMPTY) when it becomes available + log_info(" administrators=%s", administrators); + + assert_se(members = strv_join(ASSERT_PTR(sg->sg_mem), ", ")); // FIXME: use shell_maybe_quote(SHELL_ESCAPE_EMPTY) when it becomes available log_info(" members=%s", members); } @@ -92,6 +108,32 @@ static void test_getgrnam_r(void *handle, const char *module, const char *name) print_struct_group(&gr); } +static void test_getsgnam_r(void *handle, const char *module, const char *name) { + const char *fname; + _nss_getsgnam_r_t f; + char buffer[arg_bufsize]; + int errno1 = 999; /* nss-dns doesn't set those */ + enum nss_status status; + char pretty_status[DECIMAL_STR_MAX(enum nss_status)]; + struct sgrp sg; + + fname = strjoina("_nss_", module, "_getsgnam_r"); + f = dlsym(handle, fname); + log_debug("dlsym(0x%p, %s) → 0x%p", handle, fname, f); + if (!f) { + log_info("%s not defined", fname); + return; + } + + status = f(name, &sg, buffer, sizeof buffer, &errno1); + log_info("%s(\"%s\") → status=%s%-20serrno=%d/%s", + fname, name, + nss_status_to_string(status, pretty_status, sizeof pretty_status), "\n", + errno1, errno1 > 0 ? ERRNO_NAME(errno1) : "---"); + if (status == NSS_STATUS_SUCCESS) + print_struct_sgrp(&sg); +} + static void test_getpwuid_r(void *handle, const char *module, uid_t uid) { const char *fname; _nss_getpwuid_r_t f; @@ -147,6 +189,7 @@ static void test_getgrgid_r(void *handle, const char *module, gid_t gid) { static void test_byname(void *handle, const char *module, const char *name) { test_getpwnam_r(handle, module, name); test_getgrnam_r(handle, module, name); + test_getsgnam_r(handle, module, name); puts(""); }