From 07ba00fcfda5147ce41aaf415a9ab033ed7fe92c Mon Sep 17 00:00:00 2001 From: Yosuke Shimizu Date: Fri, 5 Jun 2026 10:12:35 +0900 Subject: [PATCH] wolfsshd: honor Match-block auth restrictions in DoCheckUser and RequestAuthentication --- apps/wolfsshd/auth.c | 194 ++++++++++++++++++------ apps/wolfsshd/auth.h | 6 +- apps/wolfsshd/configuration.c | 2 - apps/wolfsshd/test/test_configuration.c | 154 +++++++++++++++++++ 4 files changed, 308 insertions(+), 48 deletions(-) diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index fe1dad4a5..062455d4d 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -463,18 +463,14 @@ static int CheckPasswordUnix(const char* usr, const byte* pw, word32 pwSz, WOLFS static const char authKeysDefault[] = ".ssh/authorized_keys"; -static char authKeysPattern[MAX_PATH_SZ] = { 0 }; -void SetAuthKeysPattern(const char* pattern) -{ - if (pattern != NULL) { - WMEMSET(authKeysPattern, 0, sizeof(authKeysPattern)); - WSTRNCPY(authKeysPattern, pattern, sizeof(authKeysPattern) - 1); - } -} - - -static int ResolveAuthKeysPath(const char* homeDir, char* resolved) +/* Resolve the authorized keys file path for a user. The pattern is the user's + * configured AuthorizedKeysFile (resolved per request from the per-user config) + * and is passed in explicitly rather than read from shared state so concurrent + * authentications (e.g. Windows threaded mode) cannot race on it. A NULL or + * empty pattern falls back to the default authorized_keys location. */ +static int ResolveAuthKeysPath(const char* homeDir, const char* pattern, + char* resolved) { int ret = WS_SUCCESS; char* idx; @@ -486,14 +482,25 @@ static int ResolveAuthKeysPath(const char* homeDir, char* resolved) } if (ret == WS_SUCCESS) { - if (*authKeysPattern != 0) { + if (pattern != NULL && *pattern != 0) { /* TODO: token substitutions (e.g. %h) */ - if (*authKeysPattern == '/') { - WSTRNCPY(resolved, authKeysPattern, MAX_PATH_SZ); - return WS_SUCCESS; + if (*pattern == '/') { + /* Absolute path is used as-is. Error out rather than + * silently truncate when it does not fit, mirroring the + * relative-path branch below. */ + if (WSTRLEN(pattern) >= MAX_PATH_SZ) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Path for key file larger than max allowed"); + ret = WS_FATAL_ERROR; + } + else { + WSTRNCPY(resolved, pattern, MAX_PATH_SZ - 1); + resolved[MAX_PATH_SZ - 1] = '\0'; + } + return ret; } else { - suffix = authKeysPattern; + suffix = pattern; } } } @@ -519,7 +526,8 @@ static int ResolveAuthKeysPath(const char* homeDir, char* resolved) return ret; } -static int SearchForPubKey(const char* path, const WS_UserAuthData_PublicKey* pubKeyCtx) +static int SearchForPubKey(const char* path, const char* authKeysFile, + const WS_UserAuthData_PublicKey* pubKeyCtx) { int ret = WSSHD_AUTH_SUCCESS; char authKeysPath[MAX_PATH_SZ]; @@ -531,7 +539,7 @@ static int SearchForPubKey(const char* path, const WS_UserAuthData_PublicKey* pu int rc = 0; WMEMSET(authKeysPath, 0, sizeof(authKeysPath)); - rc = ResolveAuthKeysPath(path, authKeysPath); + rc = ResolveAuthKeysPath(path, authKeysFile, authKeysPath); if (rc != WS_SUCCESS) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Failed to resolve authorized keys" " file path."); @@ -616,7 +624,9 @@ static int CheckUserUnix(const char* name) { static int CheckPublicKeyUnix(const char* name, const WS_UserAuthData_PublicKey* pubKeyCtx, - const char* usrCaKeysFile, WOLFSSHD_AUTH* authCtx) + const char* usrCaKeysFile, + const char* authorizedKeysFile, + WOLFSSHD_AUTH* authCtx) { int ret = WSSHD_AUTH_SUCCESS; struct passwd* pwInfo; @@ -695,7 +705,7 @@ static int CheckPublicKeyUnix(const char* name, } if (ret == WSSHD_AUTH_SUCCESS) { - ret = SearchForPubKey(pwInfo->pw_dir, pubKeyCtx); + ret = SearchForPubKey(pwInfo->pw_dir, authorizedKeysFile, pubKeyCtx); } } @@ -1015,7 +1025,8 @@ static int SetupUserTokenWin(const char* usr, /* Uses Windows LSA for getting an impersination token */ static int CheckPublicKeyWIN(const char* usr, const WS_UserAuthData_PublicKey* pubKeyCtx, - const char* usrCaKeysFile, WOLFSSHD_AUTH* authCtx) + const char* usrCaKeysFile, const char* authorizedKeysFile, + WOLFSSHD_AUTH* authCtx) { int ret; @@ -1038,7 +1049,7 @@ static int CheckPublicKeyWIN(const char* usr, if (ret == WSSHD_AUTH_SUCCESS) { r[rSz-1] = L'\0'; - ret = SearchForPubKey(r, pubKeyCtx); + ret = SearchForPubKey(r, authorizedKeysFile, pubKeyCtx); if (ret != WSSHD_AUTH_SUCCESS) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Failed to find public key for user %s", usr); @@ -1062,11 +1073,24 @@ static int DoCheckUser(const char* usr, WOLFSSHD_AUTH* auth) { int ret = WOLFSSH_USERAUTH_SUCCESS; int rc; + WOLFSSHD_CONFIG* usrConf; wolfSSH_Log(WS_LOG_INFO, "[SSHD] Checking user name %s", usr); - if (wolfSSHD_ConfigGetPermitRoot(auth->conf) == 0) { - if (XSTRCMP(usr, "root") == 0) { + if (XSTRCMP(usr, "root") == 0) { + /* Resolve the per-user configuration so that a Match block override of + * PermitRootLogin is honored, rather than only consulting the global + * config node. The lookup is only needed for the root user, so it is + * scoped to this branch. + * + * A NULL return means the root user's configuration could not be + * resolved (e.g. getgrgid() failure inside wolfSSHD_AuthGetUserConf). + * Fail closed and reject the login in that case rather than falling + * back to the global node, since denying root on an unresolvable + * configuration is the safe choice. */ + usrConf = wolfSSHD_AuthGetUserConf(auth, usr, NULL, NULL, NULL, NULL, + NULL); + if (usrConf == NULL || wolfSSHD_ConfigGetPermitRoot(usrConf) == 0) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Login as root not permitted"); ret = WOLFSSH_USERAUTH_REJECTED; } @@ -1092,10 +1116,49 @@ static int DoCheckUser(const char* usr, WOLFSSHD_AUTH* auth) } +/* NULL-safe comparison of two TrustedUserCAKeys file settings. Returns 1 when + * they differ (including the case where exactly one is NULL), 0 when they are + * equal. */ +#ifdef WOLFSSHD_UNIT_TEST +int CAKeysFileDiffers(const char* a, const char* b) +#else +static int CAKeysFileDiffers(const char* a, const char* b) +#endif +{ + int ret; + + if (a == NULL || b == NULL) { + ret = (a != b) ? 1 : 0; + } + else { + ret = (WSTRCMP(a, b) != 0) ? 1 : 0; + } + + return ret; +} + + /* * @TODO this will take a pipe or equivalent to talk to a privileged thread * rather than having WOLFSSHD_AUTH directly with privilege separation. * Note: authData->type of WOLFSSH_USERAUTH_NONE is not valid for wolfsshd. + * + * Certificate auth limitation: the X.509 CA store is loaded once at startup + * from the global TrustedUserCAKeys (see SetupCTX in wolfsshd.c) and wolfSSH + * verifies the client cert chain against it before this callback runs. A + * per-user "Match ... TrustedUserCAKeys" override is never loaded into that + * store, so it cannot be enforced for certificate verification. Rather than + * silently accept a cert validated against the wrong (global) CA, the + * CA-only branch below fails closed when a Match block sets a CA file that + * differs from the global one. + * + * Note: the comparison is against the *resolved* per-user value. Match nodes + * are built by copying the preceding config node (see HandleMatch in + * configuration.c), so with multiple Match blocks a user can inherit a + * TrustedUserCAKeys set by an earlier block even though that user's own Match + * never set it. Such a user is also rejected for certificate auth, which is + * consistent with the fail-closed intent: the resolved CA still differs from + * the global store the chain was verified against. */ static int RequestAuthentication(WS_UserAuthData* authData, WOLFSSHD_AUTH* authCtx) @@ -1103,6 +1166,7 @@ static int RequestAuthentication(WS_UserAuthData* authData, int ret; int rc; const char* usr; + WOLFSSHD_CONFIG* usrConf = NULL; if (authData == NULL || authCtx == NULL) { return WOLFSSH_USERAUTH_FAILURE; @@ -1124,10 +1188,34 @@ static int RequestAuthentication(WS_UserAuthData* authData, ret = WOLFSSH_USERAUTH_FAILURE; } + /* Resolve the per-user configuration so that Match block overrides are + * honored. wolfSSHD_AuthGetUserConf defaults to the global config when no + * user-specific node applies, so this matches the existing behavior for + * non-Match users while enforcing Match restrictions. + * + * A NULL return here means the user's configuration could not be resolved + * (e.g. the primary group is unresolvable and getgrgid() failed inside + * wolfSSHD_AuthGetUserConf). DoCheckUser has already confirmed the user + * exists, so this is a rare edge. Fail closed rather than fall back to the + * permissive global node: such a user cannot complete a session anyway + * (session setup in wolfsshd.c rejects an unresolvable user config with + * WS_FATAL_ERROR), so denying auth here is safe and avoids evaluating + * password/public-key authorization against the wrong config node. */ + if (ret == WOLFSSH_USERAUTH_SUCCESS) { + usrConf = wolfSSHD_AuthGetUserConf(authCtx, usr, NULL, NULL, NULL, NULL, + NULL); + if (usrConf == NULL) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Failure to get user configuration for auth (user=%s)", + usr); + ret = WOLFSSH_USERAUTH_FAILURE; + } + } + if (ret == WOLFSSH_USERAUTH_SUCCESS && authData->type == WOLFSSH_USERAUTH_PASSWORD) { - if (wolfSSHD_ConfigGetPwAuth(authCtx->conf) != 1) { + if (wolfSSHD_ConfigGetPwAuth(usrConf) != 1) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Password authentication not " "allowed by configuration!"); ret = WOLFSSH_USERAUTH_REJECTED; @@ -1135,7 +1223,7 @@ static int RequestAuthentication(WS_UserAuthData* authData, /* Check if password is valid for this user. */ /* first handle empty password cases */ else if (authData->sf.password.passwordSz == 0 && - wolfSSHD_ConfigGetPermitEmptyPw(authCtx->conf) != 1) { + wolfSSHD_ConfigGetPermitEmptyPw(usrConf) != 1) { wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Empty passwords not allowed by " "configuration!"); ret = WOLFSSH_USERAUTH_FAILURE; @@ -1238,30 +1326,48 @@ static int RequestAuthentication(WS_UserAuthData* authData, /* if this is a certificate and no specific authorized keys file has * been set then rely on CA to have verified the cert */ if (authData->sf.publicKey.isCert && - !wolfSSHD_ConfigGetAuthKeysFileSet(authCtx->conf)) { - wolfSSH_Log(WS_LOG_INFO, - "[SSHD] Relying on CA for public key check"); - #ifdef WIN32 - /* Still need to get users token on Windows */ - rc = SetupUserTokenWin(usr, &authData->sf.publicKey, - wolfSSHD_ConfigGetUserCAKeysFile(authCtx->conf), authCtx); - if (rc == WSSHD_AUTH_SUCCESS) { - wolfSSH_Log(WS_LOG_INFO, "[SSHD] Got users token ok."); - ret = WOLFSSH_USERAUTH_SUCCESS; + !wolfSSHD_ConfigGetAuthKeysFileSet(usrConf)) { + /* The cert chain was already verified by wolfSSH against the CA + * store loaded once from the global TrustedUserCAKeys. A per-user + * Match override of TrustedUserCAKeys is not part of that store and + * cannot be enforced here, so fail closed instead of accepting a + * cert validated against the wrong CA. See the function comment. */ + if (CAKeysFileDiffers( + wolfSSHD_ConfigGetUserCAKeysFile(authCtx->conf), + wolfSSHD_ConfigGetUserCAKeysFile(usrConf))) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Per-user TrustedUserCAKeys override is not enforced " + "for certificate authentication; rejecting (user=%s)", usr); + ret = WOLFSSH_USERAUTH_REJECTED; } else { - wolfSSH_Log(WS_LOG_ERROR, - "[SSHD] Error getting users token."); - ret = WOLFSSH_USERAUTH_FAILURE; + wolfSSH_Log(WS_LOG_INFO, + "[SSHD] Relying on CA for public key check"); + #ifdef WIN32 + /* Still need to get users token on Windows */ + rc = SetupUserTokenWin(usr, &authData->sf.publicKey, + wolfSSHD_ConfigGetUserCAKeysFile(usrConf), authCtx); + if (rc == WSSHD_AUTH_SUCCESS) { + wolfSSH_Log(WS_LOG_INFO, "[SSHD] Got users token ok."); + ret = WOLFSSH_USERAUTH_SUCCESS; + } + else { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Error getting users token."); + ret = WOLFSSH_USERAUTH_FAILURE; + } + #else + ret = WOLFSSH_USERAUTH_SUCCESS; + #endif } - #else - ret = WOLFSSH_USERAUTH_SUCCESS; - #endif } else { - /* if not a certificate then parse through authorized key file */ + /* if not a certificate then parse through authorized key file. + * Pass this user's resolved AuthorizedKeysFile so a Match-block + * override is honored without relying on shared mutable state. */ rc = authCtx->checkPublicKeyCb(usr, &authData->sf.publicKey, - wolfSSHD_ConfigGetUserCAKeysFile(authCtx->conf), + wolfSSHD_ConfigGetUserCAKeysFile(usrConf), + wolfSSHD_ConfigGetAuthKeysFile(usrConf), authCtx); if (rc == WSSHD_AUTH_SUCCESS) { wolfSSH_Log(WS_LOG_INFO, "[SSHD] Public key ok."); diff --git a/apps/wolfsshd/auth.h b/apps/wolfsshd/auth.h index 808d4446a..6d3d706c0 100644 --- a/apps/wolfsshd/auth.h +++ b/apps/wolfsshd/auth.h @@ -28,7 +28,6 @@ USER_NODE* AddNewUser(USER_NODE* list, byte type, const byte* username, word32 usernameSz, const byte* value, word32 valueSz); #endif -void SetAuthKeysPattern(const char* pattern); int DefaultUserAuth(byte authType, WS_UserAuthData* authData, void* ctx); int DefaultUserAuthTypes(WOLFSSH* ssh, void* ctx); @@ -59,7 +58,9 @@ typedef int (*CallbackCheckPassword)(const char* usr, const byte* psw, */ typedef int (*CallbackCheckPublicKey)(const char* usr, const WS_UserAuthData_PublicKey* pubKey, - const char* usrCaKeysFile, WOLFSSHD_AUTH* authCtx); + const char* usrCaKeysFile, + const char* authorizedKeysFile, + WOLFSSHD_AUTH* authCtx); WOLFSSHD_AUTH* wolfSSHD_AuthCreateUser(void* heap, const WOLFSSHD_CONFIG* conf); int wolfSSHD_AuthFreeUser(WOLFSSHD_AUTH* auth); @@ -89,5 +90,6 @@ int CheckPasswordHashUnix(const char* input, char* stored); #endif int CheckAuthKeysLine(char* line, word32 lineSz, const byte* key, word32 keySz); +int CAKeysFileDiffers(const char* a, const char* b); #endif #endif /* WOLFAUTH_H */ diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 8e84f95e2..91dd6480c 100644 --- a/apps/wolfsshd/configuration.c +++ b/apps/wolfsshd/configuration.c @@ -1197,8 +1197,6 @@ int wolfSSHD_ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename) } WFCLOSE(NULL, f); - SetAuthKeysPattern(conf->authKeysFile); - return ret; } diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index 72cacf195..e1a2b2f6d 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -406,6 +406,101 @@ static int test_ConfigCopy(void) return ret; } +/* Verifies that a Match block override of the auth-relevant settings is the + * value returned by wolfSSHD_GetUserConf, and that it differs from the global + * node. RequestAuthentication and DoCheckUser resolve the per-user config via + * wolfSSHD_AuthGetUserConf (a wrapper around wolfSSHD_GetUserConf) before + * consulting PwAuth, PermitEmptyPw, PermitRootLogin and AuthKeysFileSet, so + * this locks in that resolution: a regression that reverts to the global node + * would be caught here. + * + * Coverage note: the new fail-closed branches in DoCheckUser and + * RequestAuthentication (rejecting auth when wolfSSHD_AuthGetUserConf returns + * NULL, and the Match-aware PermitRootLogin check) are not exercised directly. + * Those paths require a populated WOLFSSHD_AUTH context (opaque to this test) + * plus real system users, group lookups, callbacks, and privilege raising, so + * they are validated here only at the config-resolution layer they depend on. + * The auth-boundary enforcement itself (a tightened Match node is honored, and + * a NULL per-user config rejects rather than falls through to the global node) + * is covered by manual/integration testing of wolfsshd against an sshd_config + * containing a Match block that disables password auth and PermitRootLogin. */ +static int test_GetUserConfMatchOverride(void) +{ + int ret = WS_SUCCESS; + WOLFSSHD_CONFIG* head; + WOLFSSHD_CONFIG* conf; + WOLFSSHD_CONFIG* match; + WOLFSSHD_CONFIG* other; + + head = wolfSSHD_ConfigNew(NULL); + if (head == NULL) + ret = WS_MEMORY_E; + conf = head; + +#define PCL(s) ParseConfigLine(&conf, s, (int)WSTRLEN(s)) + /* permissive global settings */ + if (ret == WS_SUCCESS) ret = PCL("PasswordAuthentication yes"); + if (ret == WS_SUCCESS) ret = PCL("PermitEmptyPasswords yes"); + if (ret == WS_SUCCESS) ret = PCL("PermitRootLogin yes"); + + /* Match block tightens the auth settings for testuser. Lines after the + * Match keyword apply to the newly created per-user node, leaving the + * global head node unchanged. */ + if (ret == WS_SUCCESS) ret = PCL("Match User testuser"); + if (ret == WS_SUCCESS) ret = PCL("PasswordAuthentication no"); + if (ret == WS_SUCCESS) ret = PCL("PermitEmptyPasswords no"); + if (ret == WS_SUCCESS) ret = PCL("PermitRootLogin no"); + if (ret == WS_SUCCESS) ret = PCL("AuthorizedKeysFile .ssh/match_keys"); +#undef PCL + + /* the global head node must keep the permissive values */ + if (ret == WS_SUCCESS) { + if (wolfSSHD_ConfigGetPwAuth(head) != 1 || + wolfSSHD_ConfigGetPermitEmptyPw(head) != 1 || + wolfSSHD_ConfigGetPermitRoot(head) != 1) + ret = WS_FATAL_ERROR; + } + + /* resolving testuser must return the per-user node, not the global head */ + if (ret == WS_SUCCESS) { + match = wolfSSHD_GetUserConf(head, "testuser", NULL, NULL, NULL, + NULL, NULL, NULL); + if (match == NULL || match == head) + ret = WS_FATAL_ERROR; + } + + /* the resolved node must carry the tightened (overridden) values, i.e. the + * ones RequestAuthentication and DoCheckUser will now enforce */ + if (ret == WS_SUCCESS) { + if (wolfSSHD_ConfigGetPwAuth(match) != 0 || + wolfSSHD_ConfigGetPermitEmptyPw(match) != 0 || + wolfSSHD_ConfigGetPermitRoot(match) != 0) + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + if (wolfSSHD_ConfigGetAuthKeysFileSet(match) == 0) + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + if (wolfSSHD_ConfigGetAuthKeysFile(match) == NULL || + XSTRCMP(wolfSSHD_ConfigGetAuthKeysFile(match), + ".ssh/match_keys") != 0) + ret = WS_FATAL_ERROR; + } + + /* a user with no Match block must fall back to the permissive global head, + * confirming the default behavior is unchanged for non-Match users */ + if (ret == WS_SUCCESS) { + other = wolfSSHD_GetUserConf(head, "otheruser", NULL, NULL, NULL, + NULL, NULL, NULL); + if (other != head) + ret = WS_FATAL_ERROR; + } + + wolfSSHD_ConfigFree(head); + return ret; +} + /* Verifies ConfigFree releases all string fields - most useful under ASan. */ static int test_ConfigFree(void) { @@ -703,10 +798,69 @@ static int test_AuthReducePermissionsUser_uid_fail(void) } #endif /* !_WIN32 */ +/* Locks in the NULL-safe comparison used by RequestAuthentication to fail + * closed when a Match block's TrustedUserCAKeys differs from the global one. + * Covers all four permutations: both NULL (equal), exactly one NULL (differ), + * equal strings (equal), and differing strings (differ). */ +static int test_CAKeysFileDiffers(void) +{ + int ret = WS_SUCCESS; + static const char caA[] = "/etc/ssh/ca_a.pem"; + static const char caB[] = "/etc/ssh/ca_b.pem"; + static const char caADup[] = "/etc/ssh/ca_a.pem"; + + Log(" Testing scenario: both NULL compares equal."); + if (CAKeysFileDiffers(NULL, NULL) != 0) { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + else { + Log(" PASSED.\n"); + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: NULL vs non-NULL compares different."); + if (CAKeysFileDiffers(NULL, caA) != 1 || + CAKeysFileDiffers(caA, NULL) != 1) { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + else { + Log(" PASSED.\n"); + } + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: equal strings compare equal."); + if (CAKeysFileDiffers(caA, caADup) != 0) { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + else { + Log(" PASSED.\n"); + } + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: differing strings compare different."); + if (CAKeysFileDiffers(caA, caB) != 1) { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + } + else { + Log(" PASSED.\n"); + } + } + + return ret; +} + const TEST_CASE testCases[] = { TEST_DECL(test_ConfigDefaults), TEST_DECL(test_ParseConfigLine), TEST_DECL(test_ConfigCopy), + TEST_DECL(test_GetUserConfMatchOverride), + TEST_DECL(test_CAKeysFileDiffers), TEST_DECL(test_ConfigFree), #ifdef WOLFSSL_BASE64_ENCODE TEST_DECL(test_CheckAuthKeysLine),