wolfsshd: reject Match blocks using unimplemented selectors

Author: yosuke-wolfssl

apps/wolfsshd/configuration.c

@@ -913,6 +913,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)
@@ -924,6 +979,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);
@@ -946,6 +1014,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;

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),