diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 9710c683c..72ca55daa 100644 --- a/apps/wolfsshd/configuration.c +++ b/apps/wolfsshd/configuration.c @@ -914,6 +914,61 @@ static int AddRestrictedCase(WOLFSSHD_CONFIG* config, const char* mtch, } +/* returns WS_SUCCESS when every selector keyword in 'value' is one that is + * implemented. Only the User and Group selectors are handled. The Match value + * is a whitespace separated list of "keyword argument" pairs; any keyword that + * is not User or Group (Address, Host, LocalAddress, LocalPort, RDomain, ...), + * and a Match with no selector at all (bare "Match", "Match all"), is rejected. + * This also catches mixed forms such as "User alice Address 10.0.0.0/8" where + * the unsupported selector would otherwise be silently dropped. */ +static int CheckMatchSelectors(const char* value) +{ + int ret = WS_SUCCESS; + int i = 0; + int sz; + int len; + int found = 0; + + sz = (value != NULL) ? (int)XSTRLEN(value) : 0; + + while (ret == WS_SUCCESS && i < sz) { + /* skip whitespace before the keyword */ + i += CountWhitespace(value + i, sz - i, 0); + if (i >= sz) { + break; + } + + /* length of the keyword token */ + len = CountWhitespace(value + i, sz - i, 1); + if (len == (int)(sizeof("User") - 1) && + WSTRNCMP(value + i, "User", sizeof("User") - 1) == 0) { + found = 1; + } + else if (len == (int)(sizeof("Group") - 1) && + WSTRNCMP(value + i, "Group", sizeof("Group") - 1) == 0) { + found = 1; + } + else { + ret = WS_FATAL_ERROR; + } + i += len; + + if (ret == WS_SUCCESS) { + /* skip whitespace then the argument token for this keyword */ + i += CountWhitespace(value + i, sz - i, 0); + i += CountWhitespace(value + i, sz - i, 1); + } + } + + /* a Match with no implemented selector at all is also rejected */ + if (ret == WS_SUCCESS && !found) { + ret = WS_FATAL_ERROR; + } + + return ret; +} + + /* returns WS_SUCCESS on success, on success it update the conf pointed to * and makes it point to the newly created conf node */ static int HandleMatch(WOLFSSHD_CONFIG** conf, const char* value, int valueSz) @@ -925,6 +980,19 @@ static int HandleMatch(WOLFSSHD_CONFIG** conf, const char* value, int valueSz) ret = WS_BAD_ARGUMENT; } + /* Only the User and Group selectors are implemented. Reject any Match + * directive that names an unsupported selector (even when mixed with a + * supported one) or names no selector at all, rather than accepting it and + * silently dropping the unsupported part, which would fail open. */ + if (ret == WS_SUCCESS) { + ret = CheckMatchSelectors(value); + if (ret != WS_SUCCESS) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Unsupported Match selector, only User and Group are " + "handled"); + } + } + /* create new configure for altered options specific to the match */ if (ret == WS_SUCCESS) { newConf = wolfSSHD_ConfigCopy(*conf); @@ -947,6 +1015,12 @@ static int HandleMatch(WOLFSSHD_CONFIG** conf, const char* value, int valueSz) /* @TODO handle , separated user/group list */ + /* on failure free the config that will not be added to the list */ + if (ret != WS_SUCCESS && newConf != NULL) { + wolfSSHD_ConfigFree(newConf); + newConf = NULL; + } + /* update current config being processed */ if (ret == WS_SUCCESS) { (*conf)->next = newConf; diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index cac8585c7..5e01acef9 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -501,6 +501,80 @@ static int test_GetUserConfMatchOverride(void) return ret; } +/* Only the User and Group Match selectors are implemented. A Match block keyed + * on any other selector (Address, Host, LocalAddress, LocalPort, RDomain) would + * otherwise be accepted but never apply, a fail-open misconfiguration. Verify + * that User/Group are accepted while the unsupported selectors are rejected. */ +static int test_MatchUnsupportedSelector(void) +{ + int ret = WS_SUCCESS; + int i; + WOLFSSHD_CONFIG* head; + WOLFSSHD_CONFIG* conf; + + static CONFIG_LINE_VECTOR vectors[] = { + /* supported selectors */ + {"Match User", "Match User testuser", 0}, + {"Match Group", "Match Group testgroup", 0}, + + /* combined supported selectors must be accepted, in either order */ + {"Match User+Group", "Match User alice Group dev", 0}, + {"Match Group+User", "Match Group dev User alice", 0}, + + /* unsupported selectors must be rejected, not silently ignored */ + {"Match Address", "Match Address 10.0.0.0/8", 1}, + {"Match Host", "Match Host example.com", 1}, + {"Match LocalAddress", "Match LocalAddress 192.168.1.1", 1}, + {"Match LocalPort", "Match LocalPort 22", 1}, + {"Match RDomain", "Match RDomain vrf-external", 1}, + + /* no-selector forms must also be rejected, not silently accepted */ + {"Match all", "Match all", 1}, + {"Bare Match", "Match", 1}, + + /* supported selector with no argument: passes the selector check but + * fails while parsing the (missing) name, exercising the cleanup of + * the already allocated config node */ + {"Match User no arg", "Match User", 1}, + + /* mixed supported+unsupported selectors must be rejected; the + * unsupported part must not be silently dropped */ + {"Mixed User+Address", "Match User alice Address 10.0.0.0/8", 1}, + {"Mixed Group+Host", "Match Group dev Host example.com", 1}, + }; + const int numVectors = (int)(sizeof(vectors) / sizeof(*vectors)); + + head = wolfSSHD_ConfigNew(NULL); + if (head == NULL) { + ret = WS_MEMORY_E; + } + conf = head; + + if (ret == WS_SUCCESS) { + for (i = 0; i < numVectors; ++i) { + int rc; + + Log(" Testing scenario: %s.", vectors[i].desc); + + rc = ParseConfigLine(&conf, vectors[i].line, + (int)WSTRLEN(vectors[i].line), 0); + + if ((rc == WS_SUCCESS && !vectors[i].shouldFail) || + (rc != WS_SUCCESS && vectors[i].shouldFail)) { + Log(" PASSED.\n"); + } + else { + Log(" FAILED.\n"); + ret = WS_FATAL_ERROR; + break; + } + } + wolfSSHD_ConfigFree(head); + } + + 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 @@ -936,6 +1010,7 @@ const TEST_CASE testCases[] = { TEST_DECL(test_ParseConfigLine), TEST_DECL(test_ConfigCopy), TEST_DECL(test_GetUserConfMatchOverride), + TEST_DECL(test_MatchUnsupportedSelector), TEST_DECL(test_CAKeysFileDiffers), TEST_DECL(test_IncludeRecursionBound), TEST_DECL(test_ConfigFree),