diff --git a/apps/wolfsshd/auth.c b/apps/wolfsshd/auth.c index c2a933337..cc61747a3 100644 --- a/apps/wolfsshd/auth.c +++ b/apps/wolfsshd/auth.c @@ -2377,13 +2377,9 @@ static void DoFakePasswordCheck(WS_UserAuthData* authData) * 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. + * Note: the comparison is against the *resolved* per-user value. A Match node + * starts from the global config, so a user whose own Match never set + * TrustedUserCAKeys keeps the global value and is not rejected here. */ static int RequestAuthentication(WS_UserAuthData* authData, WOLFSSHD_AUTH* authCtx) diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 61ce9b1c1..2203b4bf9 100644 --- a/apps/wolfsshd/configuration.c +++ b/apps/wolfsshd/configuration.c @@ -95,6 +95,7 @@ struct WOLFSSHD_CONFIG { char* pidFile; char* authorizedUPNDomains; /* allowlist of UPN realms for cert auth */ WOLFSSHD_CONFIG* next; /* next config in list */ + WOLFSSHD_CONFIG* head; /* global config the Match nodes branch from */ long loginTimer; word16 port; byte usePrivilegeSeparation:2; @@ -112,7 +113,7 @@ struct WOLFSSHD_CONFIG { #ifndef WOLFSSHD_MAX_INCLUDE_DEPTH #define WOLFSSHD_MAX_INCLUDE_DEPTH 16 #endif -static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth); +static int ConfigLoad(WOLFSSHD_CONFIG** conf, const char* filename, int depth); static int CountWhitespace(const char* in, int inSz, byte inv); static int SetFileString(char** dst, const char* src, void* heap); @@ -238,6 +239,8 @@ WOLFSSHD_CONFIG* wolfSSHD_ConfigNew(void* heap) WMEMSET(ret, 0, sizeof(WOLFSSHD_CONFIG)); /* default values */ + ret->heap = heap; + ret->head = ret; ret->port = 22; ret->passwordAuth = 1; ret->pubKeyAuth = 1; @@ -348,6 +351,7 @@ static WOLFSSHD_CONFIG* wolfSSHD_ConfigCopy(WOLFSSHD_CONFIG* conf) newConf->permitEmptyPasswords = conf->permitEmptyPasswords; newConf->authKeysFileSet = conf->authKeysFileSet; newConf->strictModes = conf->strictModes; + newConf->head = conf->head; } else { wolfSSHD_ConfigFree(newConf); @@ -718,22 +722,25 @@ static int HandlePort(WOLFSSHD_CONFIG* conf, const char* value) } /* NOLINTNEXTLINE(misc-no-recursion): bounded by WOLFSSHD_MAX_INCLUDE_DEPTH */ -static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) +static int HandleInclude(WOLFSSHD_CONFIG **conf, const char *value, int depth) { const char *ptr; const char *ptr2; const char *postfix = NULL; const char *prefix = NULL; + void *heap = NULL; int prefixLen = 0; int found = 0; int ret = WS_SUCCESS; /* No value, nothing to do */ - if (!value || value[0] == '\0') { + if (conf == NULL || *conf == NULL || value == NULL || value[0] == '\0') { ret = WS_BAD_ARGUMENT; } if (ret == WS_SUCCESS) { + heap = (*conf)->heap; + /* Ignore trailing whitespace */ ptr = value + WSTRLEN(value) - 1; while (ptr != value) { @@ -772,7 +779,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) struct dirent *dir; WDIR d; char *path = NULL; - char *filepath = (char*)WMALLOC(PATH_MAX, conf->heap, DYNTYPE_PATH); + char *filepath = (char*)WMALLOC(PATH_MAX, heap, DYNTYPE_PATH); if (filepath == NULL) { ret = WS_MEMORY_E; @@ -788,8 +795,8 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) } if (ptr2 != value) { - path = (char*)WMALLOC(ptr2 - value + 1, - conf->heap, DYNTYPE_PATH); + path = (char*)WMALLOC(ptr2 - value + 1, heap, + DYNTYPE_PATH); if (path == NULL) { ret = WS_MEMORY_E; } @@ -801,7 +808,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) } } else { - path = (char*)WMALLOC(2, conf->heap, DYNTYPE_PATH); + path = (char*)WMALLOC(2, heap, DYNTYPE_PATH); if (path == NULL) { ret = WS_MEMORY_E; } @@ -815,7 +822,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) } if (ret == WS_SUCCESS) { - if (!WOPENDIR(NULL, conf->heap, &d, path)) { + if (!WOPENDIR(NULL, heap, &d, path)) { word32 fileCount = 0, fileFilled = 0, i, j; char** fileNames = NULL; @@ -842,7 +849,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) if (fileCount > 0) { fileNames = (char**)WMALLOC(fileCount * sizeof(char*), - conf->heap, DYNTYPE_PATH); + heap, DYNTYPE_PATH); if (fileNames == NULL) { ret = WS_MEMORY_E; } @@ -874,7 +881,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) /* Duplicate the name; readdir() may reuse its * dirent storage on the next call, so the * pointer cannot be retained across the loop. */ - char* nameCopy = WSTRDUP(dir->d_name, conf->heap, + char* nameCopy = WSTRDUP(dir->d_name, heap, DYNTYPE_PATH); if (nameCopy == NULL) { ret = WS_MEMORY_E; @@ -940,11 +947,11 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) * holds a valid pointer. */ for (i = 0; i < fileFilled; i++) { if (fileNames[i] != NULL) { - WFREE(fileNames[i], conf->heap, DYNTYPE_PATH); + WFREE(fileNames[i], heap, DYNTYPE_PATH); } } if (fileNames != NULL) { - WFREE(fileNames, conf->heap, DYNTYPE_PATH); + WFREE(fileNames, heap, DYNTYPE_PATH); } } WCLOSEDIR(NULL, &d); @@ -955,10 +962,10 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth) } } if (path != NULL) { - WFREE(path, conf->heap, DYNTYPE_PATH); + WFREE(path, heap, DYNTYPE_PATH); } if (filepath != NULL) { - WFREE(filepath, conf->heap, DYNTYPE_PATH); + WFREE(filepath, heap, DYNTYPE_PATH); } #else (void)postfix; @@ -1161,9 +1168,8 @@ static int HandleMatch(WOLFSSHD_CONFIG** conf, const char* value, int valueSz) } } - /* create new configure for altered options specific to the match */ if (ret == WS_SUCCESS) { - newConf = wolfSSHD_ConfigCopy(*conf); + newConf = wolfSSHD_ConfigCopy((*conf)->head); if (newConf == NULL) { ret = WS_MEMORY_E; } @@ -1294,7 +1300,7 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt, ret = WS_SUCCESS; break; case OPT_INCLUDE: - ret = HandleInclude(*conf, value, depth); + ret = HandleInclude(conf, value, depth); break; case OPT_CHROOT_DIR: ret = HandleChrootDir(*conf, value); @@ -1415,20 +1421,21 @@ WOLFSSHD_STATIC int ParseConfigLine(WOLFSSHD_CONFIG** conf, const char* l, */ int wolfSSHD_ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename) { - return ConfigLoad(conf, filename, 0); + WOLFSSHD_CONFIG* current = conf; + + return ConfigLoad(¤t, filename, 0); } /* NOLINTNEXTLINE(misc-no-recursion): bounded by WOLFSSHD_MAX_INCLUDE_DEPTH */ -static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth) +static int ConfigLoad(WOLFSSHD_CONFIG** conf, const char* filename, int depth) { WFILE *f; - WOLFSSHD_CONFIG* currentConfig; int ret = WS_SUCCESS; char buf[MAX_LINE_SIZE]; const char* current; - if (conf == NULL || filename == NULL) + if (conf == NULL || *conf == NULL || filename == NULL) return BAD_FUNC_ARG; if (depth >= WOLFSSHD_MAX_INCLUDE_DEPTH) { @@ -1446,7 +1453,6 @@ static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth) wolfSSH_Log(WS_LOG_INFO, "[SSHD] parsing config file %s", filename); depth++; - currentConfig = conf; while ((current = XFGETS(buf, MAX_LINE_SIZE, f)) != NULL) { int currentSz = (int)XSTRLEN(current); @@ -1465,7 +1471,7 @@ static int ConfigLoad(WOLFSSHD_CONFIG* conf, const char* filename, int depth) continue; /* commented out line */ } - ret = ParseConfigLine(¤tConfig, current, currentSz, depth); + ret = ParseConfigLine(conf, current, currentSz, depth); if (ret != WS_SUCCESS) { fprintf(stderr, "Unable to parse config line : %s\n", current); break; diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index b5b43848a..340718e9e 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -1152,61 +1152,59 @@ static int test_GetUserConfMatchRepeatedKeyword(void) return ret; } -/* Bounded recursion through Include directives: a self-including config - * must fail with WS_BAD_ARGUMENT once the depth limit is hit, and the - * config object must remain usable so a subsequent load of a normal - * config on the same WOLFSSHD_CONFIG still succeeds. */ -static int test_IncludeRecursionBound(void) +/* writes 'contents' to the file 'path', creating or truncating it. + * Returns WS_SUCCESS on success. */ +static int WriteConfigFile(const char* path, const char* contents) { - int ret = WS_SUCCESS; - WOLFSSHD_CONFIG* conf = NULL; WFILE* f = WBADFILE; - const char* loopPath = "./include_loop.conf"; - const char* normalPath = "./include_normal.conf"; - const char* loopContents = "Include ./include_loop.conf\n"; - const char* normalContents = "Port 22\n"; word32 sz, wr; + int ret = WS_SUCCESS; int cl; - if (WFOPEN(NULL, &f, loopPath, "w") != 0 || f == WBADFILE) { - Log(" Could not create %s.\n", loopPath); + if (WFOPEN(NULL, &f, path, "w") != 0 || f == WBADFILE) { + Log(" Could not create %s.\n", path); return WS_FATAL_ERROR; } - sz = (word32)WSTRLEN(loopContents); - wr = (word32)WFWRITE(NULL, loopContents, sizeof(char), sz, f); + + sz = (word32)WSTRLEN(contents); + wr = (word32)WFWRITE(NULL, contents, sizeof(char), sz, f); cl = WFCLOSE(NULL, f); - f = WBADFILE; + + /* both can fail from one I/O error, report the write first */ if (sz != wr) { - Log(" Could not write %s.\n", loopPath); - (void)WREMOVE(NULL, loopPath); - return WS_FATAL_ERROR; + Log(" Could not write %s.\n", path); + ret = WS_FATAL_ERROR; } - if (cl != 0) { - Log(" Could not close %s.\n", loopPath); - (void)WREMOVE(NULL, loopPath); - return WS_FATAL_ERROR; + else if (cl != 0) { + Log(" Could not close %s.\n", path); + ret = WS_FATAL_ERROR; } - if (WFOPEN(NULL, &f, normalPath, "w") != 0 || f == WBADFILE) { - (void)WREMOVE(NULL, loopPath); - Log(" Could not create %s.\n", normalPath); - return WS_FATAL_ERROR; - } - sz = (word32)WSTRLEN(normalContents); - wr = (word32)WFWRITE(NULL, normalContents, sizeof(char), sz, f); - cl = WFCLOSE(NULL, f); - f = WBADFILE; - if (sz != wr) { - Log(" Could not write %s.\n", normalPath); - (void)WREMOVE(NULL, loopPath); - (void)WREMOVE(NULL, normalPath); - return WS_FATAL_ERROR; + return ret; +} + + +/* Bounded recursion through Include directives: a self-including config + * must fail with WS_BAD_ARGUMENT once the depth limit is hit, and the + * config object must remain usable so a subsequent load of a normal + * config on the same WOLFSSHD_CONFIG still succeeds. */ +static int test_IncludeRecursionBound(void) +{ + int ret; + WOLFSSHD_CONFIG* conf = NULL; + const char* loopPath = "./include_loop.conf"; + const char* normalPath = "./include_normal.conf"; + const char* loopContents = "Include ./include_loop.conf\n"; + const char* normalContents = "Port 22\n"; + + ret = WriteConfigFile(loopPath, loopContents); + if (ret == WS_SUCCESS) { + ret = WriteConfigFile(normalPath, normalContents); } - if (cl != 0) { - Log(" Could not close %s.\n", normalPath); + if (ret != WS_SUCCESS) { (void)WREMOVE(NULL, loopPath); (void)WREMOVE(NULL, normalPath); - return WS_FATAL_ERROR; + return ret; } conf = wolfSSHD_ConfigNew(NULL); @@ -1242,6 +1240,212 @@ static int test_IncludeRecursionBound(void) return ret; } +/* Each Match block is built from the global config, not from the Match block + * before it. A user selected by a later block must not pick up settings that + * an earlier, non-matching block changed. */ +static int test_GetUserConfMatchNoInherit(void) +{ + int ret = WS_SUCCESS; + WOLFSSHD_CONFIG* head; + WOLFSSHD_CONFIG* conf; + WOLFSSHD_CONFIG* aliceConf = NULL; + WOLFSSHD_CONFIG* staffConf = NULL; + WOLFSSHD_CONFIG* match = NULL; + const char* cmd; + const char* grps[1]; + + head = wolfSSHD_ConfigNew(NULL); + if (head == NULL) + ret = WS_MEMORY_E; + conf = head; + +#define PCL(s) ParseConfigLine(&conf, s, (int)WSTRLEN(s), 0) + if (ret == WS_SUCCESS) ret = PCL("ForceCommand /bin/global"); + if (ret == WS_SUCCESS) ret = PCL("PermitEmptyPasswords yes"); + + /* alice's block overrides several of the global settings */ + if (ret == WS_SUCCESS) ret = PCL("Match User alice"); + if (ret == WS_SUCCESS) ret = PCL("ForceCommand /bin/alice"); + if (ret == WS_SUCCESS) ret = PCL("AuthorizedKeysFile .ssh/alice_keys"); + if (ret == WS_SUCCESS) ret = PCL("PermitEmptyPasswords no"); + if (ret == WS_SUCCESS) aliceConf = conf; + + /* the staff block sets one option, everything else must resolve to the + * global value rather than to alice's */ + if (ret == WS_SUCCESS) ret = PCL("Match Group staff"); + if (ret == WS_SUCCESS) ret = PCL("PubkeyAuthentication no"); + if (ret == WS_SUCCESS) staffConf = conf; +#undef PCL + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: staff user does not inherit alice."); + grps[0] = "staff"; + match = wolfSSHD_GetUserConf(head, "bob", grps, 1, NULL, NULL, + NULL, NULL, NULL); + if (match != staffConf) + ret = WS_FATAL_ERROR; + } + + if (ret == WS_SUCCESS) { + cmd = wolfSSHD_ConfigGetForcedCmd(match); + if (cmd == NULL || XSTRCMP(cmd, "/bin/global") != 0) { + ret = WS_FATAL_ERROR; + } + } + + if (ret == WS_SUCCESS && wolfSSHD_ConfigGetAuthKeysFileSet(match) != 0) { + ret = WS_FATAL_ERROR; + } + + if (ret == WS_SUCCESS && wolfSSHD_ConfigGetPermitEmptyPw(match) != 1) { + ret = WS_FATAL_ERROR; + } + + /* the staff block's own override still applies */ + if (ret == WS_SUCCESS && wolfSSHD_ConfigGetPubKeyAuth(match) != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: alice keeps her own overrides."); + grps[0] = "users"; + match = wolfSSHD_GetUserConf(head, "alice", grps, 1, NULL, NULL, + NULL, NULL, NULL); + if (match != aliceConf) + ret = WS_FATAL_ERROR; + + if (ret == WS_SUCCESS) { + cmd = wolfSSHD_ConfigGetForcedCmd(match); + if (cmd == NULL || XSTRCMP(cmd, "/bin/alice") != 0) + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS && + wolfSSHD_ConfigGetAuthKeysFileSet(match) != 1) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS && + wolfSSHD_ConfigGetPermitEmptyPw(match) != 0) { + ret = WS_FATAL_ERROR; + } + /* alice is not in staff, so she keeps the global pubkey setting */ + if (ret == WS_SUCCESS && wolfSSHD_ConfigGetPubKeyAuth(match) != 1) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } + } + + wolfSSHD_ConfigFree(head); + return ret; +} + + +/* A Match block inside an Include'd file must survive a later Match block in + * the including file. The parse cursor is shared across the include, so the + * outer block appends to the chain instead of overwriting it. */ +static int test_ConfigIncludeMatchChain(void) +{ + int ret; + WOLFSSHD_CONFIG* head = NULL; + WOLFSSHD_CONFIG* match; + const char* cmd; + const char* incPath = "./include_match.conf"; + const char* topPath = "./include_match_top.conf"; + const char* incContents = + "Match User alice\n" + "ForceCommand /bin/alice\n"; + const char* topContents = + "ForceCommand /bin/global\n" + "Include ./include_match.conf\n" + "Match User bob\n" + "ForceCommand /bin/bob\n"; + + ret = WriteConfigFile(incPath, incContents); + if (ret == WS_SUCCESS) { + ret = WriteConfigFile(topPath, topContents); + } + if (ret != WS_SUCCESS) { + (void)WREMOVE(NULL, incPath); + (void)WREMOVE(NULL, topPath); + return ret; + } + + head = wolfSSHD_ConfigNew(NULL); + if (head == NULL) { + ret = WS_MEMORY_E; + } + + if (ret == WS_SUCCESS) { + ret = wolfSSHD_ConfigLoad(head, topPath); + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: included Match block still applies."); + match = wolfSSHD_GetUserConf(head, "alice", NULL, 0, NULL, NULL, + NULL, NULL, NULL); + cmd = wolfSSHD_ConfigGetForcedCmd(match); + if (match == head || cmd == NULL || + XSTRCMP(cmd, "/bin/alice") != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: outer Match block still applies."); + match = wolfSSHD_GetUserConf(head, "bob", NULL, 0, NULL, NULL, + NULL, NULL, NULL); + cmd = wolfSSHD_ConfigGetForcedCmd(match); + if (match == head || cmd == NULL || XSTRCMP(cmd, "/bin/bob") != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } + } + + if (ret == WS_SUCCESS) { + Log(" Testing scenario: unmatched user gets the global config."); + match = wolfSSHD_GetUserConf(head, "carol", NULL, 0, NULL, NULL, + NULL, NULL, NULL); + cmd = wolfSSHD_ConfigGetForcedCmd(match); + if (match != head || cmd == NULL || + XSTRCMP(cmd, "/bin/global") != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } + } + + wolfSSHD_ConfigFree(head); + (void)WREMOVE(NULL, incPath); + (void)WREMOVE(NULL, topPath); + return ret; +} + + /* The public wolfSSHD_ConfigSetAuthKeysFile setter must mark the authorized * keys file as explicitly configured, otherwise certificate public-key logins * skip the authorized-keys check and rely on CA validation alone. */ @@ -3955,7 +4159,12 @@ static int test_ConfigParseAuthorizedUPNDomains(void) ret = WS_FATAL_ERROR; } } - Log(ret == WS_SUCCESS ? " PASSED.\n" : " FAILED.\n"); + if (ret == WS_SUCCESS) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + } } if (conf != NULL) { @@ -5365,6 +5574,8 @@ const TEST_CASE testCases[] = { TEST_DECL(test_ConfigParseAuthorizedUPNDomains), TEST_DECL(test_MatchUPNToUser), TEST_DECL(test_IncludeRecursionBound), + TEST_DECL(test_GetUserConfMatchNoInherit), + TEST_DECL(test_ConfigIncludeMatchChain), TEST_DECL(test_GetUserAuthTypes), TEST_DECL(test_DefaultUserAuthTypesNullArgs), TEST_DECL(test_ConfigSetAuthKeysFile),