Fix wolfsshd's cleared umask and the SFTP attribute encoder

apps/wolfsshd/configuration.c

@@ -1375,6 +1375,21 @@ static int CheckNotInMatch(const WOLFSSHD_CONFIG* conf, const char* option)
     return ret;
 }
 
+/* Returns the keyword a config option tag came from, for log messages. */
+static const char* OptionName(int opt)
+{
+    int idx;
+
+    for (idx = 0; idx < NUM_OPTIONS; ++idx) {
+        if (options[idx].tag == opt) {
+            return options[idx].name;
+        }
+    }
+
+    return "<unknown>";
+}
+
+
 /* returns WS_SUCCESS on success */
 /* NOLINTNEXTLINE(misc-no-recursion): bounded by WOLFSSHD_MAX_INCLUDE_DEPTH */
 static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
@@ -1404,28 +1419,21 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
         case OPT_PERMIT_EMPTY_PW:
             ret = HandlePermitEmptyPw(*conf, value);
             break;
+        /* @TODO Recognized for sshd_config compatibility, but nothing reads
+         * them. An unknown keyword is fatal, so accepting these in silence
+         * reads as support for a setting that is not enforced. Warn and carry
+         * on: rejecting them would turn every config copied from OpenSSH into
+         * a startup failure. */
         case OPT_SUBSYSTEM:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_CHALLENGE_RESPONSE_AUTH:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_USE_PAM:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_X11_FORWARDING:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_PRINT_MOTD:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_ACCEPT_ENV:
-            /* TODO */
+        case OPT_USE_DNS:
+            wolfSSH_Log(WS_LOG_WARN,
+                "[SSHD] %s is recognized but not implemented, it has no "
+                "effect", OptionName(opt));
             ret = WS_SUCCESS;
             break;
         case OPT_PROTOCOL:
@@ -1479,10 +1487,6 @@ static int HandleConfigOption(WOLFSSHD_CONFIG** conf, int opt,
         case OPT_PERMIT_ROOT:
             ret = HandlePermitRoot(*conf, value);
             break;
-        case OPT_USE_DNS:
-            /* TODO */
-            ret = WS_SUCCESS;
-            break;
         case OPT_INCLUDE:
             ret = HandleInclude(*conf, value, depth);
             break;

apps/wolfsshd/wolfsshd.c

@@ -109,6 +109,11 @@
     #define WOLFSSHD_TIMEOUT 1
 #endif
 
+/* The umask the daemon holds, and so the one every session inherits. */
+#ifndef WOLFSSHD_DEFAULT_UMASK
+    #define WOLFSSHD_DEFAULT_UMASK 022
+#endif
+
 #ifdef EXAMPLE_BUFFER_SZ
     #warning use WOLFSSHD_SHELL_BUFFER_SZ instead of EXAMPLE_BUFFER_SZ
     #define WOLFSSHD_SHELL_BUFFER_SZ EXAMPLE_BUFFER_SZ
@@ -4556,7 +4561,11 @@ static int StartSSHD(int argc, char** argv)
                 exit(EXIT_SUCCESS);
             }
 
-            umask(0);
+            /* Not umask(0): every per-connection child inherits this, and
+             * nothing sets one later, so a cleared mask reaches the user's
+             * shell and the SCP receive path. Files created there came out
+             * 0666 and directories 0777. */
+            umask(WOLFSSHD_DEFAULT_UMASK);
             if (chdir("/") < 0) {
                 ret = WS_FATAL_ERROR;
             }

src/wolfsftp.c

@@ -1142,9 +1142,9 @@ static int SFTP_AttributesSz(WOLFSSH* ssh, WS_SFTP_FILEATRB* atr)
 
     /* check if extended attributes are present */
     if (atr->flags & WOLFSSH_FILEATRB_EXT) {
+        /* @TODO handle extended attributes. Only the count is sized, and
+         * SFTP_SetAttributes writes it as zero to match. */
         sz += UINT32_SZ;
-
-        /* @TODO handle extended attributes */
     }
 
     return sz;
@@ -1191,14 +1191,43 @@ static int SFTP_SetAttributes(WOLFSSH* ssh, byte* buf, word32 bufSz,
 
     /* check if extended attributes are present */
     if (atr->flags & WOLFSSH_FILEATRB_EXT) {
-        /* @TODO handle attribute extensions */
-        c32toa(atr->extCount, buf + idx);
+        /* @TODO handle attribute extensions. Until they are written, the
+         * count goes out as zero: atr->extCount would promise records that
+         * follow, and the peer's decoder reads past the end looking for
+         * them. */
+        c32toa(0, buf + idx); idx += UINT32_SZ;
     }
 
     return WS_SUCCESS;
 }
 
 
+#ifdef WOLFSSH_TEST_INTERNAL
+/* Encode atr with the real encoder for unit testing. Returns the number of
+ * bytes written, or a negative error. */
+int wolfSSH_TestSftpSetAttributes(byte* buf, word32 bufSz,
+        WS_SFTP_FILEATRB* atr)
+{
+    int sz;
+
+    if (buf == NULL || atr == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+
+    sz = SFTP_AttributesSz(NULL, atr);
+    if (sz < 0 || (word32)sz > bufSz) {
+        return WS_BUFFER_E;
+    }
+
+    if (SFTP_SetAttributes(NULL, buf, bufSz, atr) != WS_SUCCESS) {
+        return WS_FATAL_ERROR;
+    }
+
+    return sz;
+}
+#endif
+
+
 static INLINE int SFTP_GetSz(byte* buf, word32* sz,
         word32 lowerBound, word32 upperBound)
 {
@@ -7094,6 +7123,20 @@ int SFTP_ParseAttributes_buffer(WOLFSSH* ssh,  WS_SFTP_FILEATRB* atr, byte* buf,
 }
 
 
+#ifdef WOLFSSH_TEST_INTERNAL
+/* Decode attributes with the real parser for unit testing. */
+int wolfSSH_TestSftpParseAttributes(byte* buf, word32 bufSz,
+        WS_SFTP_FILEATRB* atr, word32* idx)
+{
+    if (buf == NULL || atr == NULL || idx == NULL) {
+        return WS_BAD_ARGUMENT;
+    }
+
+    return SFTP_ParseAttributes_buffer(NULL, atr, buf, idx, bufSz);
+}
+#endif
+
+
 #if 0
 /* parse out file attributes from I/O stream
  *

tests/api.c

@@ -7578,6 +7578,120 @@ static void test_wolfSSH_SetAlgoList(void)
 }
 
 
+/* Is name an exact entry in a comma separated algorithm list? Substring
+ * matching would confuse hmac-sha1 with hmac-sha1-96. */
+static int AlgoListHas(const char* list, const char* name)
+{
+    const char* p = list;
+    word32 nameSz = (word32)WSTRLEN(name);
+
+    if (list == NULL) {
+        return 0;
+    }
+
+    while (*p != '\0') {
+        const char* end = WSTRCHR(p, ',');
+        word32 sz = (end != NULL) ? (word32)(end - p) : (word32)WSTRLEN(p);
+
+        if (sz == nameSz && WSTRNCMP(p, name, nameSz) == 0) {
+            return 1;
+        }
+        if (end == NULL) {
+            break;
+        }
+        p = end + 1;
+    }
+
+    return 0;
+}
+
+
+/* fenrir 13961: the canned default lists leave SHA-1 and AES-CBC out unless
+ * the build opts back in. Nothing asserted on their contents, so an inverted
+ * guard or a stray list edit could put a weak algorithm back in the default
+ * proposal without failing a test. Check what the defaults actually hold. */
+static void test_wolfSSH_DefaultAlgoListsExcludeWeak(void)
+{
+    WOLFSSH_CTX* ctx;
+    const char* kex;
+    const char* key;
+    const char* cipher;
+    const char* mac;
+
+    /* A client context: the server derives its key lists from the host keys
+     * it has loaded, so they are still null on a fresh one. */
+    ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL);
+    AssertNotNull(ctx);
+
+    kex = wolfSSH_CTX_GetAlgoListKex(ctx);
+    key = wolfSSH_CTX_GetAlgoListKey(ctx);
+    cipher = wolfSSH_CTX_GetAlgoListCipher(ctx);
+    mac = wolfSSH_CTX_GetAlgoListMac(ctx);
+    AssertNotNull(kex);
+    AssertNotNull(key);
+    AssertNotNull(cipher);
+    AssertNotNull(mac);
+
+    /* The lists are not empty: a guard that removed everything would other-
+     * wise pass every absence check below. */
+#ifndef WOLFSSH_NO_HMAC_SHA2_256
+    AssertIntEQ(AlgoListHas(mac, "hmac-sha2-256"), 1);
+#endif
+#ifndef WOLFSSH_NO_AES_CTR
+    AssertIntEQ(AlgoListHas(cipher, "aes256-ctr"), 1);
+#endif
+#ifndef WOLFSSH_NO_AES_GCM
+    AssertIntEQ(AlgoListHas(cipher, "aes256-gcm@openssh.com"), 1);
+#endif
+#ifndef WOLFSSH_NO_DH_GROUP14_SHA256
+    AssertIntEQ(AlgoListHas(kex, "diffie-hellman-group14-sha256"), 1);
+#endif
+
+    /* SHA-1 KEX, host key and MAC: in only under the opt-in macro. */
+#ifdef WOLFSSH_NO_SHA1_SOFT_DISABLE
+    #ifndef WOLFSSH_NO_DH_GROUP14_SHA1
+    AssertIntEQ(AlgoListHas(kex, "diffie-hellman-group14-sha1"), 1);
+    #endif
+    #ifndef WOLFSSH_NO_SSH_RSA_SHA1
+    AssertIntEQ(AlgoListHas(key, "ssh-rsa"), 1);
+    #endif
+    #ifndef WOLFSSH_NO_HMAC_SHA1
+    AssertIntEQ(AlgoListHas(mac, "hmac-sha1"), 1);
+    #endif
+#else
+    AssertIntEQ(AlgoListHas(kex, "diffie-hellman-group14-sha1"), 0);
+    AssertIntEQ(AlgoListHas(kex, "diffie-hellman-group1-sha1"), 0);
+    AssertIntEQ(AlgoListHas(key, "ssh-rsa"), 0);
+    AssertIntEQ(AlgoListHas(key, "x509v3-ssh-rsa"), 0);
+    AssertIntEQ(AlgoListHas(mac, "hmac-sha1"), 0);
+    AssertIntEQ(AlgoListHas(mac, "hmac-sha1-96"), 0);
+#endif
+
+    /* AES-CBC: the same, under its own macro. */
+#if defined(WOLFSSH_NO_AES_CBC_SOFT_DISABLE) && !defined(WOLFSSH_NO_AES_CBC)
+    AssertIntEQ(AlgoListHas(cipher, "aes256-cbc"), 1);
+#else
+    AssertIntEQ(AlgoListHas(cipher, "aes256-cbc"), 0);
+    AssertIntEQ(AlgoListHas(cipher, "aes192-cbc"), 0);
+    AssertIntEQ(AlgoListHas(cipher, "aes128-cbc"), 0);
+#endif
+
+    /* A fresh session inherits the context's policy, so the same holds. */
+    {
+        WOLFSSH* ssh = wolfSSH_new(ctx);
+
+        AssertNotNull(ssh);
+        AssertPtrEq(wolfSSH_GetAlgoListKex(ssh), kex);
+        AssertPtrEq(wolfSSH_GetAlgoListKey(ssh), key);
+        AssertPtrEq(wolfSSH_GetAlgoListCipher(ssh), cipher);
+        AssertPtrEq(wolfSSH_GetAlgoListMac(ssh), mac);
+        wolfSSH_free(ssh);
+    }
+
+    wolfSSH_CTX_free(ctx);
+}
+
+
 /* Exercise CheckAlgoList()'s rejection paths through the public setters. */
 static void test_wolfSSH_CheckAlgoList(void)
 {
@@ -8249,6 +8363,7 @@ int wolfSSH_ApiTest(int argc, char** argv)
     test_wolfSSH_SetMaxAuthAttempts();
     test_wolfSSH_AlgoListKeyInSync();
     test_wolfSSH_SetAlgoList();
+    test_wolfSSH_DefaultAlgoListsExcludeWeak();
     test_wolfSSH_CheckAlgoList();
 #ifdef WOLFSSH_FWD
     test_wolfSSH_FwdRemote_badArgs();

tests/regress.c

@@ -11879,6 +11879,84 @@ static void TestOct2DecRejectsInvalidNonLeadingDigit(void)
     wolfSSH_CTX_free(ctx);
 }
 
+
+/* fenrir 2479: the attribute encoder and decoder must agree. Extensions are
+ * unimplemented, so an encode of WOLFSSH_FILEATRB_EXT writes a zero count and
+ * the peer's decoder consumes the block instead of reading past it looking for
+ * records that were never written. */
+static void TestSftpAttributesRoundTrip(void)
+{
+    static const struct {
+        word32 flags;
+        word32 sz;
+    } cases[] = {
+        { 0, 4 },
+        { WOLFSSH_FILEATRB_PERM, 8 },
+        { WOLFSSH_FILEATRB_SIZE | WOLFSSH_FILEATRB_UIDGID
+            | WOLFSSH_FILEATRB_PERM | WOLFSSH_FILEATRB_TIME, 32 },
+        { WOLFSSH_FILEATRB_EXT, 8 },
+        { WOLFSSH_FILEATRB_PERM | WOLFSSH_FILEATRB_EXT, 12 },
+        { WOLFSSH_FILEATRB_SIZE | WOLFSSH_FILEATRB_UIDGID
+            | WOLFSSH_FILEATRB_PERM | WOLFSSH_FILEATRB_TIME
+            | WOLFSSH_FILEATRB_EXT, 36 },
+    };
+    word32 i;
+
+    for (i = 0; i < (word32)(sizeof(cases) / sizeof(cases[0])); i++) {
+        WS_SFTP_FILEATRB in, out;
+        byte   buf[64];
+        word32 idx = 0;
+        int    encSz;
+
+        WMEMSET(&in, 0, sizeof(in));
+        WMEMSET(&out, 0, sizeof(out));
+        WMEMSET(buf, 0xEE, sizeof(buf));
+
+        in.flags = cases[i].flags;
+        in.sz[0] = 0x44332211;
+        in.sz[1] = 0x88776655;
+        in.uid   = 1000;
+        in.gid   = 1001;
+        in.per   = 0640;
+        in.atime = 0x5A5A5A5A;
+        in.mtime = 0x6B6B6B6B;
+        /* a caller that set the flag may well have set a count too */
+        in.extCount = 3;
+
+        encSz = wolfSSH_TestSftpSetAttributes(buf, (word32)sizeof(buf), &in);
+        AssertIntEQ(encSz, (int)cases[i].sz);
+
+        /* the encoder writes exactly the size it advertises */
+        AssertTrue(encSz < (int)sizeof(buf));
+        AssertIntEQ(buf[encSz], 0xEE);
+
+        /* the decoder consumes exactly that, over a buffer bounded to it */
+        AssertIntEQ(wolfSSH_TestSftpParseAttributes(buf, (word32)encSz, &out,
+                    &idx), WS_SUCCESS);
+        AssertIntEQ((int)idx, encSz);
+
+        AssertIntEQ((int)out.flags, (int)in.flags);
+        if (in.flags & WOLFSSH_FILEATRB_SIZE) {
+            AssertIntEQ((int)out.sz[0], (int)in.sz[0]);
+            AssertIntEQ((int)out.sz[1], (int)in.sz[1]);
+        }
+        if (in.flags & WOLFSSH_FILEATRB_UIDGID) {
+            AssertIntEQ((int)out.uid, (int)in.uid);
+            AssertIntEQ((int)out.gid, (int)in.gid);
+        }
+        if (in.flags & WOLFSSH_FILEATRB_PERM) {
+            AssertIntEQ((int)out.per, (int)in.per);
+        }
+        if (in.flags & WOLFSSH_FILEATRB_TIME) {
+            AssertIntEQ((int)out.atime, (int)in.atime);
+            AssertIntEQ((int)out.mtime, (int)in.mtime);
+        }
+        /* no extension records are written, so none come back */
+        AssertIntEQ((int)out.extCount, 0);
+        AssertNull(out.exts);
+    }
+}
+
 #endif /* WOLFSSH_SFTP */
 
 #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256))
@@ -16447,6 +16525,8 @@ int main(int argc, char** argv)
 #ifdef WOLFSSH_SFTP
     TestOct2DecRejectsInvalidNonLeadingDigit();
     TestSftpBufferSendPendingOutput();
+    /* the attribute encoder and decoder agree, extensions included */
+    TestSftpAttributesRoundTrip();
     #if !defined(NO_WOLFSSH_SERVER) && !defined(USE_WINDOWS_API) && \
             !defined(NO_FILESYSTEM)
     /* fenrir 4232/4343/4346/4349: forged SFTP file handles must be rejected */