From da908a02d4e26818c65efb77997288b6637f2b74 Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Wed, 8 Jul 2026 11:31:47 +0900 Subject: [PATCH] wolfsshd: make the supplementary-group drop reliable and tested --- apps/wolfsshd/auth.c | 165 ++++++++++-------- apps/wolfsshd/auth.h | 2 + apps/wolfsshd/test/test_configuration.c | 214 ++++++++++++++++++++++++ 3 files changed, 315 insertions(+), 66 deletions(-) diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index 793ee2dcc..a4df53d66 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -93,8 +93,16 @@ #endif #if defined(WOLFSSHD_UNIT_TEST) && !defined(_WIN32) +/* Adapts the platform setgroups(2) to a fixed prototype so unit tests can + * swap in a stub regardless of the size argument's native type (int on + * macOS, size_t on Linux). */ +static int wsshd_setgroups_default(int size, const WGID_T* list) +{ + return setgroups(size, list); +} int (*wsshd_setregid_cb)(WGID_T, WGID_T) = setregid; int (*wsshd_setreuid_cb)(WUID_T, WUID_T) = setreuid; +int (*wsshd_setgroups_cb)(int, const WGID_T*) = wsshd_setgroups_default; #endif struct WOLFSSHD_AUTH { @@ -1626,7 +1634,7 @@ static int RequestAuthentication(WS_UserAuthData* authData, ret = WOLFSSH_USERAUTH_REJECTED; } else { - #ifdef WIN32 + #ifdef _WIN32 /* Still need to get users token on Windows */ wolfSSH_Log(WS_LOG_INFO, "[SSHD] Relying on CA for public key check"); @@ -1945,7 +1953,7 @@ int wolfSSHD_AuthRaisePermissions(WOLFSSHD_AUTH* auth) int ret = 0; wolfSSH_Log(WS_LOG_INFO, "[SSHD] Attempting to raise permissions level"); -#ifndef WIN32 +#ifndef _WIN32 if (auth) { if (setegid(auth->sGid) != 0) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error raising gid"); @@ -1970,7 +1978,7 @@ int wolfSSHD_AuthRaisePermissions(WOLFSSHD_AUTH* auth) int wolfSSHD_AuthReducePermissionsUser(WOLFSSHD_AUTH* auth, WUID_T uid, WGID_T gid) { -#ifndef WIN32 +#ifndef _WIN32 #ifdef WOLFSSHD_UNIT_TEST if (wsshd_setregid_cb(gid, gid) != 0) { #else @@ -2005,7 +2013,7 @@ int wolfSSHD_AuthReducePermissions(WOLFSSHD_AUTH* auth) } flag = wolfSSHD_ConfigGetPrivilegeSeparation(auth->conf); -#ifndef WIN32 +#ifndef _WIN32 if (flag == WOLFSSHD_PRIV_SEPARAT || flag == WOLFSSHD_PRIV_SANDBOX) { wolfSSH_Log(WS_LOG_INFO, "[SSHD] Lowering permissions level"); @@ -2023,13 +2031,25 @@ int wolfSSHD_AuthReducePermissions(WOLFSSHD_AUTH* auth) return ret; } -#ifndef WIN32 +#ifndef _WIN32 #if defined(__OSX__) || defined( __APPLE__) #define WGETGROUPLIST(x,y,z,w) getgrouplist((x),(y),(int*)(z),(w)) #else #define WGETGROUPLIST(x,y,z,w) getgrouplist((x),(y),(z),(w)) #endif +#if defined(WOLFSSHD_UNIT_TEST) && !defined(_WIN32) +/* Adapts getgrouplist(3) to a fixed prototype so unit tests can swap in a + * stub, hiding the macOS int/gid_t argument difference behind WGETGROUPLIST. */ +static int wsshd_getgrouplist_default(const char* usr, WGID_T grp, + WGID_T* groups, int* ngroups) +{ + return WGETGROUPLIST(usr, grp, groups, ngroups); +} +int (*wsshd_getgrouplist_cb)(const char*, WGID_T, WGID_T*, int*) + = wsshd_getgrouplist_default; +#endif + /* Initial guess and upper bound for the number of groups a user can be in. * getgrouplist cannot be reliably sized with a NULL probe (macOS returns * success with size 0 and, when the buffer is too small, echoes the input size @@ -2042,39 +2062,19 @@ int wolfSSHD_AuthReducePermissions(WOLFSSHD_AUTH* auth) #define WOLFSSHD_GROUP_LIST_MAX 65536 #endif -/* frees a group name array previously built by wolfSSHD_GetUserGroupNames */ -WOLFSSHD_STATIC void wolfSSHD_FreeUserGroupNames(void* heap, char** names, - word32 count) -{ - word32 i; - - if (names != NULL) { - for (i = 0; i < count; i++) { - WFREE(names[i], heap, DYNTYPE_SSHD); - } - WFREE(names, heap, DYNTYPE_SSHD); - } -} - -/* Builds the list of group names the user belongs to, primary plus - * supplementary, so Match Group can be evaluated against the full set. On - * success sets *outNames to an owned array of owned names and *outCount to the - * entry count; the caller frees them with wolfSSHD_FreeUserGroupNames. Returns - * WS_SUCCESS on success, leaving *outNames NULL on failure. */ -WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, - WGID_T primaryGid, char*** outNames, word32* outCount) +/* Resolves the user's full gid list into an owned buffer via grow-and-retry + * sizing, since a NULL-size probe is unreliable (macOS reports size 0). Sets + * *outList (caller frees) and *outCount; returns WS_SUCCESS. */ +static int wolfSSHD_GetUserGroupList(void* heap, const char* usr, + WGID_T primaryGid, gid_t** outList, int* outCount) { int ret = WS_SUCCESS; int grpListSz = 0; int allocSz; int res; - int i; gid_t* grpList = NULL; - char** names = NULL; - struct group* g; - word32 count = 0; - *outNames = NULL; + *outList = NULL; *outCount = 0; #if defined(__QNX__) || defined(__QNXNTO__) @@ -2092,7 +2092,11 @@ WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, } if (ret == WS_SUCCESS) { grpListSz = allocSz; + #ifdef WOLFSSHD_UNIT_TEST + res = wsshd_getgrouplist_cb(usr, primaryGid, grpList, &grpListSz); + #else res = WGETGROUPLIST(usr, primaryGid, grpList, &grpListSz); + #endif if (res != 0) { ret = WS_FATAL_ERROR; } @@ -2111,7 +2115,11 @@ WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, } grpListSz = allocSz; + #ifdef WOLFSSHD_UNIT_TEST + res = wsshd_getgrouplist_cb(usr, primaryGid, grpList, &grpListSz); + #else res = WGETGROUPLIST(usr, primaryGid, grpList, &grpListSz); + #endif if (res < 0) { /* buffer too small: discard, grow, and retry up to the cap */ WFREE(grpList, heap, DYNTYPE_SSHD); @@ -2128,6 +2136,51 @@ WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, } #endif + if (ret == WS_SUCCESS) { + *outList = grpList; + *outCount = grpListSz; + } + else if (grpList != NULL) { + WFREE(grpList, heap, DYNTYPE_SSHD); + } + + return ret; +} + +/* frees a group name array previously built by wolfSSHD_GetUserGroupNames */ +WOLFSSHD_STATIC void wolfSSHD_FreeUserGroupNames(void* heap, char** names, + word32 count) +{ + word32 i; + + if (names != NULL) { + for (i = 0; i < count; i++) { + WFREE(names[i], heap, DYNTYPE_SSHD); + } + WFREE(names, heap, DYNTYPE_SSHD); + } +} + +/* Builds the owned list of group names the user belongs to (primary plus + * supplementary) for Match Group evaluation; freed with + * wolfSSHD_FreeUserGroupNames. Returns WS_SUCCESS, *outNames NULL on failure. */ +WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, + WGID_T primaryGid, char*** outNames, word32* outCount) +{ + int ret; + int grpListSz = 0; + int i; + gid_t* grpList = NULL; + char** names = NULL; + struct group* g; + word32 count = 0; + + *outNames = NULL; + *outCount = 0; + + ret = wolfSSHD_GetUserGroupList(heap, usr, primaryGid, &grpList, + &grpListSz); + if (ret == WS_SUCCESS) { names = (char**)WMALLOC(sizeof(char*) * grpListSz, heap, DYNTYPE_SSHD); if (names == NULL) { @@ -2167,52 +2220,32 @@ WOLFSSHD_STATIC int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, return ret; } -#endif /* WIN32 */ +#endif /* _WIN32 */ /* sets the extended groups the user is in, returns WS_SUCCESS on success */ int wolfSSHD_AuthSetGroups(const WOLFSSHD_AUTH* auth, const char* usr, WGID_T gid) { int ret = WS_SUCCESS; -#ifndef WIN32 +#ifndef _WIN32 int grpListSz = 0; gid_t* grpList = NULL; -#if defined(__QNX__) || defined(__QNXNTO__) - /* QNX does not support getting the exact group list size ahead of time, - only the max group list size */ - grpListSz = sysconf( _SC_NGROUPS_MAX ); + /* resolve the full group list with portable grow-and-retry sizing, then + * apply it; a NULL-size probe is unreliable and would skip the drop. */ + ret = wolfSSHD_GetUserGroupList(auth->heap, usr, gid, &grpList, &grpListSz); + if (ret == WS_SUCCESS) { +#ifdef WOLFSSHD_UNIT_TEST + if (wsshd_setgroups_cb(grpListSz, grpList) == -1) { #else - /* should return -1 if grpListSz is smaller than actual groups */ - if (WGETGROUPLIST(usr, gid, NULL, &grpListSz) == -1) + if (setgroups(grpListSz, grpList) == -1) { #endif - { - grpList = (gid_t*)WMALLOC(sizeof(gid_t) * grpListSz, auth->heap, - DYNTYPE_SSHD); - if (grpList == NULL) { - ret = WS_MEMORY_E; - } - else { - int res; - - res = WGETGROUPLIST(usr, gid, grpList, &grpListSz); - #if defined(__QNX__) || defined(__QNXNTO__) - if (res != 0) { - ret = WS_FATAL_ERROR; - } - #else - if (res != grpListSz) { - ret = WS_FATAL_ERROR; - } - #endif - - if (ret == WS_SUCCESS && - setgroups(grpListSz, grpList) == -1) { - ret = WS_FATAL_ERROR; - } - WFREE(grpList, auth->heap, DYNTYPE_SSHD); + ret = WS_FATAL_ERROR; } } + if (grpList != NULL) { + WFREE(grpList, auth->heap, DYNTYPE_SSHD); + } #else WOLFSSH_UNUSED(auth); WOLFSSH_UNUSED(usr); @@ -2247,7 +2280,7 @@ WOLFSSHD_CONFIG* wolfSSHD_AuthGetUserConf(const WOLFSSHD_AUTH* auth, if (auth != NULL) { if (usr != NULL) { -#ifdef WIN32 +#ifdef _WIN32 /* LogonUserEx(): group lookup is not implemented on Windows, so * Match Group directives do not apply here */ #else @@ -2271,7 +2304,7 @@ WOLFSSHD_CONFIG* wolfSSHD_AuthGetUserConf(const WOLFSSHD_AUTH* auth, ret = wolfSSHD_GetUserConf(auth->conf, usr, (const char**)grpNames, grpCount, host, localAdr, localPort, RDomain, adr); -#ifndef WIN32 +#ifndef _WIN32 wolfSSHD_FreeUserGroupNames(auth->heap, grpNames, grpCount); #endif } diff --git a/apps/wolfsshd/auth.h b/apps/wolfsshd/auth.h index c487a68ca..6e2b28afb 100644 --- a/apps/wolfsshd/auth.h +++ b/apps/wolfsshd/auth.h @@ -90,6 +90,8 @@ int wolfSSHD_OpenSecureFile(const char* path, WUID_T ownerUid, #ifndef _WIN32 extern int (*wsshd_setregid_cb)(WGID_T, WGID_T); extern int (*wsshd_setreuid_cb)(WUID_T, WUID_T); +extern int (*wsshd_setgroups_cb)(int, const WGID_T*); +extern int (*wsshd_getgrouplist_cb)(const char*, WGID_T, WGID_T*, int*); int wolfSSHD_GetUserGroupNames(void* heap, const char* usr, WGID_T primaryGid, char*** outNames, word32* outCount); void wolfSSHD_FreeUserGroupNames(void* heap, char** names, word32 count); diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index aa5c410de..c6e203f72 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -32,6 +32,7 @@ #include #include #include + #include #endif #ifndef WOLFSSH_DEFAULT_LOG_WIDTH @@ -1450,6 +1451,88 @@ static void InstallPrivDropStubs(int regidRet, int reuidRet, s_setreuid_arg0 = s_setreuid_arg1 = 0; } +#define GROUP_STUB_MAX 16 +static WGID_T s_grouplist_groups[GROUP_STUB_MAX]; +static int s_grouplist_count; +static int s_grouplist_always_fail; +static int s_grouplist_calls; +static WGID_T s_setgroups_seen[GROUP_STUB_MAX]; +static int s_setgroups_size; +static int s_setgroups_list_nonnull; +static int s_setgroups_ret; +static int s_setgroups_called; + +static int stub_getgrouplist(const char* usr, WGID_T grp, WGID_T* groups, + int* ngroups) +{ + int i; + + WOLFSSH_UNUSED(usr); + WOLFSSH_UNUSED(grp); + s_grouplist_calls++; + /* simulate a lookup that never fits, exercising the resolve failure path */ + if (s_grouplist_always_fail) { + *ngroups = s_grouplist_count; + return -1; + } + /* too-small buffer: echo the needed size and return -1 so the caller grows + * and retries, matching the getgrouplist(3) contract. */ + if (groups == NULL || *ngroups < s_grouplist_count) { + *ngroups = s_grouplist_count; + return -1; + } + for (i = 0; i < s_grouplist_count; i++) { + groups[i] = s_grouplist_groups[i]; + } + *ngroups = s_grouplist_count; +#if defined(__QNX__) || defined(__QNXNTO__) + /* QNX getgrouplist returns 0 on success rather than the count */ + return 0; +#else + return s_grouplist_count; +#endif +} + +static int stub_setgroups(int size, const WGID_T* list) +{ + int i; + + s_setgroups_called = 1; + s_setgroups_size = size; + /* record what we can while the buffer is live; the caller frees it on + * return, so the pointer itself must not be inspected afterwards. */ + s_setgroups_list_nonnull = (list != NULL); + for (i = 0; list != NULL && i < size && i < GROUP_STUB_MAX; i++) { + s_setgroups_seen[i] = list[i]; + } + return s_setgroups_ret; +} + +static void InstallGroupStubs(int setgroupsRet, + int (**savedGrouplist)(const char*, WGID_T, WGID_T*, int*), + int (**savedSetgroups)(int, const WGID_T*)) +{ + int i; + + *savedGrouplist = wsshd_getgrouplist_cb; + *savedSetgroups = wsshd_setgroups_cb; + wsshd_getgrouplist_cb = stub_getgrouplist; + wsshd_setgroups_cb = stub_setgroups; + s_grouplist_count = 3; + s_grouplist_groups[0] = 1001; + s_grouplist_groups[1] = 1002; + s_grouplist_groups[2] = 1003; + s_grouplist_always_fail = 0; + s_grouplist_calls = 0; + s_setgroups_ret = setgroupsRet; + s_setgroups_called = 0; + s_setgroups_size = 0; + s_setgroups_list_nonnull = 0; + for (i = 0; i < GROUP_STUB_MAX; i++) { + s_setgroups_seen[i] = 0; + } +} + /* Exercises the group-name enumeration helper against the account running the * test. Covers the allocation and ownership contract end to end (the names * array, each duplicated name, and the gid scratch buffer) so a sanitizer run @@ -1559,6 +1642,134 @@ static int test_AuthReducePermissionsUser_uid_fail(void) wsshd_setreuid_cb = savedReuid; return ret; } + +/* Drives the supplementary-group drop with getgrouplist and setgroups mocked + * so it is deterministic across platforms; asserts setgroups is invoked with + * the resolved group count. */ +static int test_AuthSetGroups_ok(void) +{ + int ret = WS_SUCCESS; + WOLFSSHD_CONFIG* conf = NULL; + WOLFSSHD_AUTH* auth = NULL; + int (*savedGrouplist)(const char*, WGID_T, WGID_T*, int*); + int (*savedSetgroups)(int, const WGID_T*); + + conf = wolfSSHD_ConfigNew(NULL); + if (conf == NULL) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { + auth = wolfSSHD_AuthCreateUser(NULL, conf); + if (auth == NULL) + ret = WS_FATAL_ERROR; + } + + InstallGroupStubs(0, &savedGrouplist, &savedSetgroups); + + if (ret == WS_SUCCESS + && wolfSSHD_AuthSetGroups(auth, "testuser", 1000) != WS_SUCCESS) + ret = WS_FATAL_ERROR; + + /* the drop must actually call setgroups with the resolved list */ + if (ret == WS_SUCCESS && !s_setgroups_called) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS && s_setgroups_size != s_grouplist_count) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS && !s_setgroups_list_nonnull) + ret = WS_FATAL_ERROR; + /* the exact resolved gids must reach setgroups, not just the count */ + if (ret == WS_SUCCESS + && (s_setgroups_seen[0] != 1001 || s_setgroups_seen[1] != 1002 + || s_setgroups_seen[2] != 1003)) + ret = WS_FATAL_ERROR; +#if !defined(__QNX__) && !defined(__QNXNTO__) + /* the small init size (-DWOLFSSHD_GROUP_LIST_INIT=1) forces at least one + * grow-and-retry before the lookup fits, covering that branch */ + if (ret == WS_SUCCESS && s_grouplist_calls < 2) + ret = WS_FATAL_ERROR; +#endif + + wsshd_getgrouplist_cb = savedGrouplist; + wsshd_setgroups_cb = savedSetgroups; + if (auth != NULL) + (void)wolfSSHD_AuthFreeUser(auth); + if (conf != NULL) + wolfSSHD_ConfigFree(conf); + return ret; +} + +/* A setgroups(2) failure must abort the privilege setup with WS_FATAL_ERROR + * rather than being silently ignored. */ +static int test_AuthSetGroups_setgroups_fail(void) +{ + int ret = WS_SUCCESS; + WOLFSSHD_CONFIG* conf = NULL; + WOLFSSHD_AUTH* auth = NULL; + int (*savedGrouplist)(const char*, WGID_T, WGID_T*, int*); + int (*savedSetgroups)(int, const WGID_T*); + + conf = wolfSSHD_ConfigNew(NULL); + if (conf == NULL) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { + auth = wolfSSHD_AuthCreateUser(NULL, conf); + if (auth == NULL) + ret = WS_FATAL_ERROR; + } + + InstallGroupStubs(-1, &savedGrouplist, &savedSetgroups); + + if (ret == WS_SUCCESS + && wolfSSHD_AuthSetGroups(auth, "testuser", 1000) != WS_FATAL_ERROR) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS && !s_setgroups_called) + ret = WS_FATAL_ERROR; + + wsshd_getgrouplist_cb = savedGrouplist; + wsshd_setgroups_cb = savedSetgroups; + if (auth != NULL) + (void)wolfSSHD_AuthFreeUser(auth); + if (conf != NULL) + wolfSSHD_ConfigFree(conf); + return ret; +} + +/* A getgrouplist lookup that never succeeds must fail closed with + * WS_FATAL_ERROR and never reach the setgroups drop. */ +static int test_AuthSetGroups_getgrouplist_fail(void) +{ + int ret = WS_SUCCESS; + WOLFSSHD_CONFIG* conf = NULL; + WOLFSSHD_AUTH* auth = NULL; + int (*savedGrouplist)(const char*, WGID_T, WGID_T*, int*); + int (*savedSetgroups)(int, const WGID_T*); + + conf = wolfSSHD_ConfigNew(NULL); + if (conf == NULL) + ret = WS_FATAL_ERROR; + if (ret == WS_SUCCESS) { + auth = wolfSSHD_AuthCreateUser(NULL, conf); + if (auth == NULL) + ret = WS_FATAL_ERROR; + } + + InstallGroupStubs(0, &savedGrouplist, &savedSetgroups); + s_grouplist_always_fail = 1; + + if (ret == WS_SUCCESS + && wolfSSHD_AuthSetGroups(auth, "testuser", 1000) != WS_FATAL_ERROR) + ret = WS_FATAL_ERROR; + /* group resolution failed, so the drop must not have run */ + if (ret == WS_SUCCESS && s_setgroups_called) + ret = WS_FATAL_ERROR; + + wsshd_getgrouplist_cb = savedGrouplist; + wsshd_setgroups_cb = savedSetgroups; + if (auth != NULL) + (void)wolfSSHD_AuthFreeUser(auth); + if (conf != NULL) + wolfSSHD_ConfigFree(conf); + return ret; +} #endif /* !_WIN32 */ /* Locks in the NULL-safe comparison used by RequestAuthentication to fail @@ -1929,6 +2140,9 @@ const TEST_CASE testCases[] = { TEST_DECL(test_AuthReducePermissionsUser_ok), TEST_DECL(test_AuthReducePermissionsUser_gid_fail), TEST_DECL(test_AuthReducePermissionsUser_uid_fail), + TEST_DECL(test_AuthSetGroups_ok), + TEST_DECL(test_AuthSetGroups_setgroups_fail), + TEST_DECL(test_AuthSetGroups_getgrouplist_fail), #endif #if defined(WOLFSSH_HAVE_LIBCRYPT) || defined(WOLFSSH_HAVE_LIBLOGIN) TEST_DECL(test_CheckPasswordHashUnix),