From e69693521ce14beca0daa830c0f6374cbcbe12bc Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 21 Sep 2026 11:41:08 -0700 Subject: [PATCH 1/4] sftp: write a zero extension count in attributes SFTP_SetAttributes() sizes and writes only the count for a WOLFSSH_FILEATRB_EXT block, never the records themselves, so the count goes out as zero. A caller's atr->extCount would promise records that do not follow, and the peer's parser reads past the block looking for them. - advance idx past the count, so a later field cannot land on it - wolfSSH_TestSftpSetAttributes() and wolfSSH_TestSftpParseAttributes() reach the encoder and parser for testing - tests/regress.c round-trips the flag combinations Issue: F-2479 --- src/wolfsftp.c | 51 ++++++++++++++++++++++++++--- tests/regress.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/wolfsftp.h | 4 +++ 3 files changed, 131 insertions(+), 4 deletions(-) diff --git a/src/wolfsftp.c b/src/wolfsftp.c index e7cca5487..699d1e886 100644 --- a/src/wolfsftp.c +++ b/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 * diff --git a/tests/regress.c b/tests/regress.c index 9b135211b..b0f3d063d 100644 --- a/tests/regress.c +++ b/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 */ diff --git a/wolfssh/wolfsftp.h b/wolfssh/wolfsftp.h index 676912aa6..9b3c9aefd 100644 --- a/wolfssh/wolfsftp.h +++ b/wolfssh/wolfsftp.h @@ -354,6 +354,10 @@ WOLFSSH_LOCAL void wolfSSH_SFTP_ShowSizes(void); WOLFSSH_API int wolfSSH_TestSftpDoName(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_TestSftpGetHandle(WOLFSSH* ssh, byte* handle, word32* handleSz); + WOLFSSH_API int wolfSSH_TestSftpSetAttributes(byte* buf, word32 bufSz, + WS_SFTP_FILEATRB* atr); + WOLFSSH_API int wolfSSH_TestSftpParseAttributes(byte* buf, word32 bufSz, + WS_SFTP_FILEATRB* atr, word32* idx); WOLFSSH_API int wolfSSH_TestSftpSendCap(WOLFSSH* ssh, word32 cap); WOLFSSH_API int wolfSSH_TestSftpStallPending(WOLFSSH* ssh, word32 count); #if !defined(NO_WOLFSSH_SERVER) && !defined(NO_FILESYSTEM) From 4a8562fbd7724c4dc7cfaae0d4b004de20b56421 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 21 Sep 2026 11:41:08 -0700 Subject: [PATCH 2/4] wolfsshd: warn on unimplemented config options Subsystem, ChallengeResponseAuthentication, UsePAM, X11Forwarding, PrintMotd, AcceptEnv and UseDNS parse and are then dropped. An unknown keyword is fatal, so accepting these in silence reads as support for a setting that is not enforced. Log a warning naming the keyword instead. Rejecting them would fail every config copied from OpenSSH. - OptionName() maps an option tag back to its keyword for the message Issue: F-2868 --- apps/wolfsshd/configuration.c | 44 +++++++++++++++++++---------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index 771bd1a28..4e0cf197b 100644 --- a/apps/wolfsshd/configuration.c +++ b/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 ""; +} + + /* 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; From 1c7544bd3cebef5a94f1d3b094445a6400343a0b Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 21 Sep 2026 11:41:08 -0700 Subject: [PATCH 3/4] wolfsshd: keep a 022 umask in the daemon Daemonizing cleared the umask and nothing set one afterwards, so every per-connection child inherited it: files created from a shell or through the SCP receive path came out 0666, directories 0777. Hold 022, which is what a foreground daemon already inherits from its caller. - WOLFSSHD_DEFAULT_UMASK sets it at build time Issue: F-3671 --- apps/wolfsshd/wolfsshd.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/apps/wolfsshd/wolfsshd.c b/apps/wolfsshd/wolfsshd.c index 6c3ba5623..73ecd7aff 100644 --- a/apps/wolfsshd/wolfsshd.c +++ b/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; } From 8874c04786d54c947c423267a75e27e530d5c05f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 21 Sep 2026 11:41:08 -0700 Subject: [PATCH 4/4] tests: check the default algorithm lists Existing coverage asserts only that the canned KEX, host key, cipher and MAC lists are non-null and that a session inherits them. Read the lists, so an inverted guard cannot put SHA-1 or AES-CBC back into the default proposal unnoticed. - weak entries are expected only under their opt-in macros - modern entries are expected present, so an emptied list still fails Issue: F-13961 --- tests/api.c | 115 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/tests/api.c b/tests/api.c index 51e291d08..916180037 100644 --- a/tests/api.c +++ b/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();