fix for test correctness, use new build options probe

Author: JacobBarthelmeh

README.md

@@ -595,7 +595,9 @@ rejected inside a `Match` block):
 Note that the pre-existing `HostKey` and `HostCertificate` directives are now
 also rejected when they appear after a `Match` block (matching OpenSSH); they
 were previously accepted there and silently ignored, so a config that relied
-on that will now stop the daemon at startup with a parse error.
+on that will now stop the daemon at startup with a parse error. Builds made
+with `WOLFSSH_IGNORE_UNKNOWN_CONFIG` instead log a warning and ignore the
+directive, preserving the old behavior as a migration path.
 
 Without FPKI, a client certificate is bound to the requested account by a
 case-insensitive subject CN match only; keep the trusted CA set narrow. Note

apps/wolfsshd/configuration.c

@@ -855,7 +855,7 @@ static int HandleInclude(WOLFSSHD_CONFIG *conf, const char *value, int depth)
         /* Ignore trailing whitespace */
         ptr = value + WSTRLEN(value) - 1;
         while (ptr != value) {
-            if (WISSPACE(*ptr)) {
+            if (WISSPACE((unsigned char)*ptr)) {
                 ptr--;
             }
             else {
@@ -1412,14 +1412,32 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
         case OPT_HOST_KEY:
             /* TODO: Add logic to check if file exists? */
             ret = CheckNotInMatch(*conf, "HostKey");
-            if (ret == WS_SUCCESS)
+            if (ret == WS_SUCCESS) {
                 ret = wolfSSHD_ConfigSetHostKeyFile(*conf, value);
+            }
+        #ifdef WOLFSSH_IGNORE_UNKNOWN_CONFIG
+            else if (*conf != NULL) {
+                /* Earlier releases accepted (and never used) this placement,
+                 * so ignore-unknown builds keep a migration path. */
+                wolfSSH_Log(WS_LOG_WARN,
+                    "[SSHD] Ignoring HostKey inside a Match block");
+                ret = WS_SUCCESS;
+            }
+        #endif
             break;
         case OPT_HOST_CERT:
             /* TODO: Add logic to check if file exists? */
             ret = CheckNotInMatch(*conf, "HostCertificate");
-            if (ret == WS_SUCCESS)
+            if (ret == WS_SUCCESS) {
                 ret = wolfSSHD_ConfigSetHostCertFile(*conf, value);
+            }
+        #ifdef WOLFSSH_IGNORE_UNKNOWN_CONFIG
+            else if (*conf != NULL) {
+                wolfSSH_Log(WS_LOG_WARN,
+                    "[SSHD] Ignoring HostCertificate inside a Match block");
+                ret = WS_SUCCESS;
+            }
+        #endif
             break;
         case OPT_PASSWORD_AUTH:
             ret = HandlePwAuth(*conf, value);
@@ -1626,16 +1644,33 @@ WOLFSSHD_STATIC int ParseConfigLine(WOLFSSHD_CONFIG** conf, const char* l,
         }
     }
     else {
-    #ifdef WOLFSSH_IGNORE_UNKNOWN_CONFIG
-        /* WARN, not DEBUG: a known option in the unsupported Keyword=value
-         * form also lands here, and silently dropping a directive such as
-         * PasswordAuthentication=no must stay visible without -d. */
-        wolfSSH_Log(WS_LOG_WARN, "[SSHD] ignoring config line %s.", l);
-        ret = WS_SUCCESS;
-    #else
-        wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error parsing config line.");
-        ret = WS_FATAL_ERROR;
-    #endif
+        int isEqForm = 0;
+
+        /* A known keyword in the OpenSSH Keyword=value form must stay a
+         * fatal error even on builds that ignore unknown lines: dropping a
+         * directive such as PasswordAuthentication=no would fail open. */
+        for (idx = 0; idx < NUM_OPTIONS; ++idx) {
+            sz = (int)WSTRLEN(options[idx].name);
+            if (lSz > sz && WSTRNCMP(l, options[idx].name, sz) == 0 &&
+                    l[sz] == '=') {
+                isEqForm = 1;
+                break;
+            }
+        }
+        if (isEqForm) {
+            wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Keyword=value form is not "
+                "supported, use \"Keyword value\" : %s.", l);
+            ret = WS_FATAL_ERROR;
+        }
+        else {
+        #ifdef WOLFSSH_IGNORE_UNKNOWN_CONFIG
+            wolfSSH_Log(WS_LOG_WARN, "[SSHD] ignoring config line %s.", l);
+            ret = WS_SUCCESS;
+        #else
+            wolfSSH_Log(WS_LOG_ERROR, "[SSHD] Error parsing config line.");
+            ret = WS_FATAL_ERROR;
+        #endif
+        }
     }
 
     return ret;

apps/wolfsshd/test/create_sshd_config.sh

@@ -32,11 +32,12 @@ EOF
 
 # wolfSSHd refuses to start when AuthorizedUPNDomains is set on a build that
 # cannot enforce it (wolfSSL without FPKI), so only write the directive when
-# the daemon binary reports FPKI support. sshd_x509_upn_fail.sh skips itself
-# on such builds for the same reason.
+# the build reports FPKI support. sshd_x509_upn_fail.sh skips itself on such
+# builds using the same probe.
+. ./wolfssh_options.sh
 UPN_DOMAIN_GOOD=""
 UPN_DOMAIN_BAD=""
-if ../wolfsshd "-?" 2>&1 | grep -q "FPKI"; then
+if wolfssh_has FPKI; then
     UPN_DOMAIN_GOOD="AuthorizedUPNDomains example"
     UPN_DOMAIN_BAD="AuthorizedUPNDomains other.example"
 fi

apps/wolfsshd/test/run_all_sshd_tests.sh

@@ -174,7 +174,7 @@ EOF
 run_upn_unenforceable_negative_test() {
     printf "AuthorizedUPNDomains unenforceable-build negative test ... "
     TOTAL=$((TOTAL+1))
-    if ../wolfsshd "-?" 2>&1 | grep -q "FPKI"; then
+    if wolfssh_has FPKI; then
         printf "SKIPPED (FPKI build enforces the directive)\n"
         SKIPPED=$((SKIPPED+1))
         return
@@ -206,12 +206,10 @@ EOF
     # branch logs "Ignoring AuthorizedUPNDomains ... cannot enforce it" and
     # keeps running, which must not pass as the startup refusal. Also require
     # that the host key loaded: a "Refusing to load" failure would exit before
-    # the UPN gate. Finally require that nothing was left listening on the
-    # test port.
+    # the UPN gate.
     if grep -q "AuthorizedUPNDomains is set" upn_nofpki_log.txt &&
             grep -q "but this build cannot enforce it" upn_nofpki_log.txt &&
-            ! grep -q "Refusing to load" upn_nofpki_log.txt &&
-            ! nc -z 127.0.0.1 22623 2>/dev/null; then
+            ! grep -q "Refusing to load" upn_nofpki_log.txt; then
         printf "PASSED\n"
     else
         printf "FAILED!\n"

apps/wolfsshd/test/test_configuration.c

@@ -295,12 +295,17 @@ static int test_ParseConfigLine(void)
 
         /* The option matcher requires whitespace (or end of line) after the
          * matched name, so an unknown name that extends a real one must not
-         * prefix-match it. */
+         * prefix-match it. Ignore-unknown builds accept such lines with a
+         * warning, so only assert rejection where it is observable. */
+    #ifndef WOLFSSH_IGNORE_UNKNOWN_CONFIG
         {"Unknown extension of Port", "PortFoo 22", 1},
         {"Unknown extension of HostKey", "HostKeyFoo /tmp/x", 1},
         {"Unknown extension of HostKeyStore", "HostKeyStoreX MY", 1},
         {"Unknown extension of TrustedUserCAStore",
             "wolfSSH_TrustedUserCAStoreX yes", 1},
+    #endif
+        /* A known keyword in Keyword=value form is a hard error on every
+         * build; ignoring it would silently drop the directive. */
         {"Keyword=value form is rejected", "Port=22", 1},
 
         /* The two store-trust toggles follow the same yes/no/invalid

apps/wolfsshd/wolfsshd.c

@@ -311,6 +311,9 @@ static void wolfSSHDLoggingCb(enum wolfSSH_LogLevel lvl, const char *const str)
      * level messages. Warnings carry the security relevant notices, e.g. that
      * a certificate was bound to an account by subject CN alone, so they must
      * not depend on -d. */
+    if (logFile == NULL) {
+        return;
+    }
     if (lvl == WS_LOG_ERROR || lvl == WS_LOG_WARN || debugMode) {
 #ifdef _WIN32
         AcquireSRWLockExclusive(&logRepeatLock);
@@ -640,7 +643,7 @@ static int LoadUserCACertsFromStore(const WOLFSSHD_CONFIG* conf,
     if (providerStr != NULL &&
             WSTRCMP(providerStr, "CERT_STORE_PROV_SYSTEM") != 0) {
         wolfSSH_Log(WS_LOG_ERROR,
-            "[SSHD] wolfSSH_WinUserStores='%s' is not supported; only "
+            "[SSHD] wolfSSH_WinUserStores='%.48s' is not supported; only "
             "CERT_STORE_PROV_SYSTEM is supported", providerStr);
         return WS_BAD_ARGUMENT;
     }
@@ -704,7 +707,7 @@ static int LoadUserCACertsFromStore(const WOLFSSHD_CONFIG* conf,
             wStoreName);
     if (hStore == NULL) {
         wolfSSH_Log(WS_LOG_ERROR,
-            "[SSHD] Unable to open user CA cert store '%s', error %lu",
+            "[SSHD] Unable to open user CA cert store '%.48s', error %lu",
             storeNameStr, (unsigned long)GetLastError());
         WFREE(wStoreName, heap, DYNTYPE_SSHD);
         return WS_FATAL_ERROR;
@@ -797,6 +800,32 @@ static int LoadUserCACertsFromStore(const WOLFSSHD_CONFIG* conf,
 }
 #endif /* WOLFSSH_CERTS && WOLFSSH_WINDOWS_CERT_STORE */
 
+#if defined(WOLFSSH_CERTS) && (defined(_WIN32) || defined(WOLFSSL_FPKI))
+/* Returns non-zero when any config node configures a certificate trust
+ * anchor. TrustedUserCAKeys may live inside a Match block, so the whole list
+ * must be walked; the two store flags are global-only and live on the head
+ * node. */
+static int AnyNodeHasCertTrustAnchor(const WOLFSSHD_CONFIG* conf)
+{
+    const WOLFSSHD_CONFIG* cur;
+    int found = 0;
+
+    if (wolfSSHD_ConfigGetUserCAStore(conf) ||
+            wolfSSHD_ConfigGetSystemCA(conf)) {
+        found = 1;
+    }
+    cur = conf;
+    while (!found && cur != NULL) {
+        if (wolfSSHD_ConfigGetUserCAKeysFile(cur) != NULL) {
+            found = 1;
+        }
+        cur = wolfSSHD_ConfigGetNext(cur);
+    }
+
+    return found;
+}
+#endif /* WOLFSSH_CERTS && (_WIN32 || WOLFSSL_FPKI) */
+
 /* Initializes and sets up the WOLFSSH_CTX struct based on the configure options
  * return WS_SUCCESS on success
  */
@@ -1239,10 +1268,7 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
      * fixed by the wolfSSL build, not by configuration, so this cannot be
      * derived from the config file. */
     #if defined(WOLFSSH_CERTS) && !defined(WOLFSSL_FPKI) && defined(_WIN32)
-    if (ret == WS_SUCCESS &&
-            (wolfSSHD_ConfigGetUserCAKeysFile(conf) != NULL ||
-             wolfSSHD_ConfigGetUserCAStore(conf) ||
-             wolfSSHD_ConfigGetSystemCA(conf))) {
+    if (ret == WS_SUCCESS && AnyNodeHasCertTrustAnchor(conf)) {
         wolfSSH_Log(WS_LOG_WARN,
             "[SSHD] WARNING: client certificates are bound to an account by "
             "subject CN only.");
@@ -1344,10 +1370,7 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
      * callback writes WARN unconditionally, so a per-attempt WARN would let
      * a peer grow the log). Only emitted when a certificate trust anchor is
      * actually configured; with no CA there is no UPN check to relax. */
-    if (ret == WS_SUCCESS &&
-            (wolfSSHD_ConfigGetUserCAKeysFile(conf) != NULL ||
-             wolfSSHD_ConfigGetUserCAStore(conf) ||
-             wolfSSHD_ConfigGetSystemCA(conf))) {
+    if (ret == WS_SUCCESS && AnyNodeHasCertTrustAnchor(conf)) {
         const WOLFSSHD_CONFIG* cur;
         const char* domains;
 
@@ -1376,10 +1399,7 @@ static int SetupCTX(WOLFSSHD_CONFIG* conf, WOLFSSH_CTX** ctx,
      * no CN check to skip. With FPKI the UPN identity check always runs, so
      * there is nothing to note. */
     #if defined(WOLFSSH_CERTS) && !defined(WOLFSSL_FPKI) && defined(_WIN32)
-    if (ret == WS_SUCCESS &&
-            (wolfSSHD_ConfigGetUserCAKeysFile(conf) != NULL ||
-             wolfSSHD_ConfigGetUserCAStore(conf) ||
-             wolfSSHD_ConfigGetSystemCA(conf))) {
+    if (ret == WS_SUCCESS && AnyNodeHasCertTrustAnchor(conf)) {
         const WOLFSSHD_CONFIG* cur;
 
         cur = conf;
@@ -3229,6 +3249,9 @@ static void* HandleConnection(void* arg)
 {
     int ret = WS_SUCCESS;
     int error;
+#ifdef _WIN32
+    byte threaded = 0;
+#endif
 
     WOLFSSHD_CONNECTION* conn = NULL;
     WOLFSSH* ssh = NULL;
@@ -3572,8 +3595,24 @@ static void* HandleConnection(void* arg)
         WCLOSESOCKET(conn->fd);
     }
     wolfSSH_Log(WS_LOG_INFO, "[SSHD] Return from closing connection = %d", ret);
+#ifdef _WIN32
+    if (conn != NULL) {
+        threaded = conn->isThreaded;
+    }
+#endif
     WFREE(conn, NULL, DYNTYPE_SSHD);
+
+    /* The repeat state is per connection only when each connection is its own
+     * process (POSIX fork) or the lone in-process connection. Windows daemon
+     * threads share it, so flushing here would clear a streak another live
+     * connection thread still owns; the shutdown flush covers that path. */
+#ifdef _WIN32
+    if (!threaded) {
+        wolfSSHDLoggingFlush();
+    }
+#else
     wolfSSHDLoggingFlush();
+#endif
 
 #ifdef _WIN32
     return 0;
@@ -3865,6 +3904,9 @@ static int StartSSHD(int argc, char** argv)
             ret = WFOPEN(NULL, &logFile, myoptarg, "ab");
             if (ret != 0 || logFile == WBADFILE) {
                 fprintf(stderr, "Unable to open log file %s\n", myoptarg);
+                /* option parsing continues and may log before the error is
+                 * acted on, so never leave the stream NULL */
+                logFile = stderr;
                 ret = WS_FATAL_ERROR;
             }
             break;

examples/echoserver/echoserver.c

@@ -3104,8 +3104,8 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
     const char* cipherList = NULL;
     ES_HEAP_HINT* heap = NULL;
     #ifdef WOLFSSH_TPM
-        static char* tpmKeyPath = NULL;
-        static char* tpmHostKeyPath = NULL;
+        char* tpmKeyPath = NULL;
+        char* tpmHostKeyPath = NULL;
     #endif
     int multipleConnections = 1;
     int userEcc = 0;
@@ -3276,23 +3276,25 @@ THREAD_RETURN WOLFSSH_THREAD echoserver_test(void* args)
     }
     myoptind = 0;      /* reset for test cases */
 
-    wc_InitMutex(&doneLock);
-
-#ifdef WOLFSSH_TEST_BLOCK
-    if (!nonBlock) {
-        ES_ERROR("Use -N when testing forced non-blocking\n");
-    }
-#endif
-
 #if defined(WOLFSSH_TPM) && defined(WOLFSSH_WINDOWS_CERT_STORE)
     /* Both register a host key on the same CTX; loading both would leave
      * which key the server presents up to algorithm negotiation. The SFTP
-     * client and wolfsshd reject the equivalent mixes the same way. */
+     * client and wolfsshd reject the equivalent mixes the same way.
+     * Checked before wc_InitMutex(&doneLock) so ES_ERROR's return path
+     * does not leak an initialized mutex. */
     if (tpmHostKeyPath != NULL && certStoreSpec != NULL) {
         ES_ERROR("-W cannot be combined with -G\n");
     }
 #endif
 
+    wc_InitMutex(&doneLock);
+
+#ifdef WOLFSSH_TEST_BLOCK
+    if (!nonBlock) {
+        ES_ERROR("Use -N when testing forced non-blocking\n");
+    }
+#endif
+
 #ifdef WOLFSSH_NO_RSA
     /* If wolfCrypt isn't built with RSA, force ECC on. */
     userEcc = 1;