diff --git a/apps/wolfsshd/configuration.c b/apps/wolfsshd/configuration.c index c16c63b16..61c7a9b6a 100644 --- a/apps/wolfsshd/configuration.c +++ b/apps/wolfsshd/configuration.c @@ -29,6 +29,18 @@ #endif #ifdef WOLFSSH_SSHD + +/* Define POSIX feature-test macros before any system header (as auth.c does) so + * fdopen/ftruncate/lstat/S_ISLNK are declared under strict -std= builds. */ +#ifdef __linux__ + #ifndef _XOPEN_SOURCE + #define _XOPEN_SOURCE + #endif + #ifndef _GNU_SOURCE + #define _GNU_SOURCE + #endif +#endif + /* functions for parsing out options from a config file and for handling loading * key/certs using the env. filesystem */ @@ -51,10 +63,13 @@ #include "configuration.h" -#ifndef WIN32 +#ifndef _WIN32 #include +#include +#include +#include #endif -#ifdef WIN32 +#ifdef _WIN32 #include #endif #ifdef HAVE_LIMITS_H @@ -1779,18 +1794,105 @@ void wolfSSHD_ConfigSavePID(const WOLFSSHD_CONFIG* conf) { FILE* f; char buf[12]; /* large enough to hold 'int' type with null terminator */ + word32 pidLen; + int writeOk; +#ifndef _WIN32 + int fd; + int ok = 1; + struct stat st; +#endif if (conf->pidFile != NULL) { WMEMSET(buf, 0, sizeof(buf)); + /* Reached only on real POSIX (wolfsshd requires fork/getpwnam), so raw + * open/fstat/ftruncate/fdopen are safe; the O_NOFOLLOW hardening needs + * a real fd that WFOPEN does not expose. */ +#ifndef _WIN32 +#if defined(WOLFSSH_HAVE_SYMLINK) && WOLFSSH_O_NOFOLLOW == 0 + /* Symlinks are a concern here but O_NOFOLLOW is unavailable, so reject a + * symlinked leaf with a best-effort lstat pre-check (racy, the closest + * guard without the atomic primitive). */ + if (lstat(conf->pidFile, &st) == 0 && S_ISLNK(st.st_mode)) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Refusing symlinked PID file %s", conf->pidFile); + ok = 0; + } +#endif + /* Open with O_NOFOLLOW and an explicit 0644 mode so a pre-positioned + * symlink at the PID file path cannot redirect the write to another + * file, and the result is never world-writable under a relaxed umask. */ + if (ok) { + /* No O_TRUNC (truncate only after the fstat check below) and + * O_NONBLOCK so a planted FIFO fails fast instead of stalling the + * open before that check runs. */ + fd = open(conf->pidFile, + O_WRONLY | O_CREAT | O_NONBLOCK | WOLFSSH_O_NOFOLLOW, + 0644); + if (fd < 0) { + /* covers a refused symlink leaf (O_NOFOLLOW) as well as any + * other open failure. */ + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Unable to open PID file %s", conf->pidFile); + } + /* O_NOFOLLOW blocks a symlink but not a hard link onto a root-owned + * file; require a single-link regular file before truncating. */ + else if (fstat(fd, &st) != 0 || !S_ISREG(st.st_mode) || + st.st_nlink != 1) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Refusing unsafe PID file %s", conf->pidFile); + close(fd); + } + else if (ftruncate(fd, 0) != 0) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Unable to truncate PID file %s", conf->pidFile); + close(fd); + } + else { + /* O_NONBLOCK left set is a no-op on a regular-file write. */ + f = fdopen(fd, "wb"); + if (f == NULL) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Unable to open PID file stream %s", + conf->pidFile); + close(fd); + /* the file was created or truncated above; remove the empty + * leftover so no reader parses a zero-length PID. */ + unlink(conf->pidFile); + } + else { + WSNPRINTF(buf, sizeof(buf), "%d", getpid()); + pidLen = (word32)WSTRLEN(buf); + writeOk = ((word32)WFWRITE(NULL, buf, 1, pidLen, f) == pidLen); + /* always close, then drop a short-written or unflushed PID + * file so no reader parses a truncated value. */ + if (WFCLOSE(NULL, f) != 0) { + writeOk = 0; + } + if (!writeOk) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Failed to write PID file %s", conf->pidFile); + unlink(conf->pidFile); + } + } + } + } +#else if (WFOPEN(NULL, &f, conf->pidFile, "wb") == 0) { - #ifndef WIN32 - WSNPRINTF(buf, sizeof(buf), "%d", getpid()); - #else WSNPRINTF(buf, sizeof(buf), "%d", _getpid()); - #endif - WFWRITE(NULL, buf, 1, WSTRLEN(buf), f); - WFCLOSE(NULL, f); + pidLen = (word32)WSTRLEN(buf); + writeOk = ((word32)WFWRITE(NULL, buf, 1, pidLen, f) == pidLen); + /* always close, then drop a short-written or unflushed PID file so + * no reader parses a truncated value. */ + if (WFCLOSE(NULL, f) != 0) { + writeOk = 0; + } + if (!writeOk) { + wolfSSH_Log(WS_LOG_ERROR, + "[SSHD] Failed to write PID file %s", conf->pidFile); + WREMOVE(NULL, conf->pidFile); + } } +#endif } } diff --git a/apps/wolfsshd/test/test_configuration.c b/apps/wolfsshd/test/test_configuration.c index aa5c410de..cc58f6b66 100644 --- a/apps/wolfsshd/test/test_configuration.c +++ b/apps/wolfsshd/test/test_configuration.c @@ -1898,6 +1898,196 @@ static int test_OpenSecureFile(void) return ret; } + +/* load a config whose PidFile is 'pidTarget' and run wolfSSHD_ConfigSavePID + * under umask(0), restoring the umask afterward so later tests are unaffected */ +static int pidSave(const char* confPath, const char* pidTarget) +{ + int ret; + mode_t old; + WOLFSSHD_CONFIG* cfg; + FILE* f; + + f = fopen(confPath, "w"); + if (f == NULL) { + return WS_FATAL_ERROR; + } + fprintf(f, "PidFile %s\n", pidTarget); + fclose(f); + + cfg = wolfSSHD_ConfigNew(NULL); + if (cfg == NULL) { + return WS_MEMORY_E; + } + ret = wolfSSHD_ConfigLoad(cfg, confPath); + if (ret == WS_SUCCESS) { + /* deliberate worst case: prove the explicit 0644 mode holds even when + * the umask would not mask any bits */ + old = umask(0); + wolfSSHD_ConfigSavePID(cfg); + umask(old); + } + wolfSSHD_ConfigFree(cfg); + return ret; +} + +/* wolfSSHD_ConfigSavePID must refuse a symlink at the PID path so a planted + * link cannot truncate another file, and must create the PID file without + * group or world write even under a permissive umask. */ +static int test_ConfigSavePID(void) +{ + int ret = WS_SUCCESS; + int rd; + long pid = -1; + char base[] = "/tmp/wolfsshd_pidXXXXXX"; + char conf[80] = ""; + char pidPath[96] = ""; + char victim[96] = ""; + char linkPath[96] = ""; + char hvictim[96] = ""; + char hlink[96] = ""; + char fifo[96] = ""; + const char* secret = "VICTIM-CONTENTS\n"; + FILE* f = NULL; + struct stat st; + char rbuf[64]; + + if (mkdtemp(base) == NULL) { + Log(" mkdtemp failed.\n"); + ret = WS_FATAL_ERROR; + } + + if (ret == WS_SUCCESS) { + snprintf(conf, sizeof(conf), "%s/sshd_config", base); + snprintf(pidPath, sizeof(pidPath), "%s/wolfsshd.pid", base); + snprintf(victim, sizeof(victim), "%s/victim", base); + snprintf(linkPath, sizeof(linkPath), "%s/link.pid", base); + snprintf(hvictim, sizeof(hvictim), "%s/hvictim", base); + snprintf(hlink, sizeof(hlink), "%s/hlink.pid", base); + snprintf(fifo, sizeof(fifo), "%s/fifo.pid", base); + } + + /* Scenario 1: a normal path is written with our PID and is not group or + * world writable despite umask(0). */ + if (ret == WS_SUCCESS) { + ret = pidSave(conf, pidPath); + } + if (ret == WS_SUCCESS) { + if (stat(pidPath, &st) != 0) { + ret = WS_FATAL_ERROR; + } + else { + f = fopen(pidPath, "r"); + rd = (f != NULL) ? fscanf(f, "%ld", &pid) : 0; + if (f != NULL) { + fclose(f); + } + ret = smExpect("normal PID file written with our PID", + (rd == 1 && pid == (long)getpid()) ? WS_SUCCESS + : WS_FATAL_ERROR, 1); + if (ret == WS_SUCCESS) { + ret = smExpect("PID file not group or world writable", + (st.st_mode & (S_IWGRP | S_IWOTH)) ? WS_FATAL_ERROR + : WS_SUCCESS, 1); + } + } + } + + /* Scenario 2: a symlink at the PID path is refused, link target untouched. + * The common build exercises the atomic O_NOFOLLOW path; the lstat fallback + * needs a symlink-capable platform without O_NOFOLLOW. */ + if (ret == WS_SUCCESS) { + f = fopen(victim, "w"); + if (f == NULL) { + ret = WS_FATAL_ERROR; + } + else { + fputs(secret, f); + fclose(f); + } + } + if (ret == WS_SUCCESS && symlink(victim, linkPath) != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + ret = pidSave(conf, linkPath); + } + if (ret == WS_SUCCESS) { + f = fopen(victim, "r"); + WMEMSET(rbuf, 0, sizeof(rbuf)); + rd = (f != NULL) ? (int)fread(rbuf, 1, sizeof(rbuf) - 1, f) : -1; + if (f != NULL) { + fclose(f); + } + (void)rd; + /* A WOLFSSH_NO_SYMLINK_CHECK build does no symlink check by design, so + * only assert the target survived when the check is compiled in. */ +#ifdef WOLFSSH_HAVE_SYMLINK + ret = smExpect("symlinked PID path not followed, target intact", + (WSTRCMP(rbuf, secret) == 0) ? WS_SUCCESS : WS_FATAL_ERROR, 1); +#else + (void)rbuf; +#endif + } + + /* Scenario 3: a hard link at the PID path is refused (O_NOFOLLOW stops a + * symlink but not a hard link), leaving the link target untouched. The + * st_nlink check that enforces this is unconditional, so this always runs. */ + if (ret == WS_SUCCESS) { + f = fopen(hvictim, "w"); + if (f == NULL) { + ret = WS_FATAL_ERROR; + } + else { + fputs(secret, f); + fclose(f); + } + } + if (ret == WS_SUCCESS && link(hvictim, hlink) != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + ret = pidSave(conf, hlink); + } + if (ret == WS_SUCCESS) { + f = fopen(hvictim, "r"); + WMEMSET(rbuf, 0, sizeof(rbuf)); + rd = (f != NULL) ? (int)fread(rbuf, 1, sizeof(rbuf) - 1, f) : -1; + if (f != NULL) { + fclose(f); + } + (void)rd; + ret = smExpect("hard-linked PID path refused, target intact", + (WSTRCMP(rbuf, secret) == 0) ? WS_SUCCESS : WS_FATAL_ERROR, 1); + } + + /* Scenario 4: a FIFO at the PID path is rejected (O_NONBLOCK fast-fails the + * open and the S_ISREG check refuses it), and the FIFO is left in place + * rather than replaced by a regular PID file. */ + if (ret == WS_SUCCESS && mkfifo(fifo, 0600) != 0) { + ret = WS_FATAL_ERROR; + } + if (ret == WS_SUCCESS) { + ret = pidSave(conf, fifo); + } + if (ret == WS_SUCCESS) { + ret = smExpect("FIFO PID path refused, still a FIFO", + (lstat(fifo, &st) == 0 && S_ISFIFO(st.st_mode)) ? WS_SUCCESS + : WS_FATAL_ERROR, 1); + } + + /* cleanup */ + unlink(fifo); + unlink(linkPath); + unlink(victim); + unlink(hlink); + unlink(hvictim); + unlink(pidPath); + unlink(conf); + rmdir(base); + + return ret; +} #endif /* !_WIN32 */ const TEST_CASE testCases[] = { @@ -1920,6 +2110,7 @@ const TEST_CASE testCases[] = { TEST_DECL(test_ConfigFree), #ifndef _WIN32 TEST_DECL(test_OpenSecureFile), + TEST_DECL(test_ConfigSavePID), #endif #ifdef WOLFSSL_BASE64_ENCODE TEST_DECL(test_CheckAuthKeysLine), diff --git a/wolfssh/port.h b/wolfssh/port.h index 983818378..377876dd0 100644 --- a/wolfssh/port.h +++ b/wolfssh/port.h @@ -1577,20 +1577,15 @@ extern "C" { #define WOLFSSH_HAVE_SYMLINK #endif -/* wIsSymlink lives in the always-compiled port.c, but its filesystem - * dependencies (WSTAT_T/WLSTAT/S_ISLNK on POSIX, WS_GetFileAttributesExA on - * Windows) and its only callers exist solely in the SFTP and SCP code, so - * gate it on those features in addition to the platform capability. Shared by - * the SFTP path-confinement guard and the SCP send guards. */ -#if defined(WOLFSSH_HAVE_SYMLINK) && \ - (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) - WOLFSSH_LOCAL int wIsSymlink(const char* path); - - /* Open flag that refuses to follow a final-component symbolic link, so the - * open is atomic against a swapped link. Maps to the platform primitive - * where available (POSIX O_NOFOLLOW) and to 0 elsewhere (Windows and any - * filesystem lacking it), where wFopenNoFollow falls back to a wIsSymlink - * check before the open. */ +/* No-follow open flags, defined for every filesystem that can store a symlink + * (POSIX and Windows) so the SFTP, SCP, and SSHD code share one definition. */ +#ifdef WOLFSSH_HAVE_SYMLINK + #ifndef USE_WINDOWS_API + #include /* supplies O_NOFOLLOW/O_DIRECTORY on POSIX */ + #endif + /* Open flag refusing to follow a final-component symlink, so the open is + * atomic against a swapped link. 0 where unavailable (Windows, link-less + * filesystems), where the caller falls back to a wIsSymlink check. */ #ifdef O_NOFOLLOW #define WOLFSSH_O_NOFOLLOW O_NOFOLLOW #else @@ -1603,6 +1598,22 @@ extern "C" { #else #define WOLFSSH_O_DIRECTORY 0 #endif +#endif /* WOLFSSH_HAVE_SYMLINK */ + +/* Catch-all so callers can use WOLFSSH_O_NOFOLLOW unconditionally; resolves to + * 0 when symlinks are not a concern (no-symlink filesystems, opt-out builds). */ +#ifndef WOLFSSH_O_NOFOLLOW + #define WOLFSSH_O_NOFOLLOW 0 +#endif + +/* wIsSymlink lives in the always-compiled port.c, but its filesystem + * dependencies (WSTAT_T/WLSTAT/S_ISLNK on POSIX, WS_GetFileAttributesExA on + * Windows) and its only callers exist solely in the SFTP and SCP code, so + * gate it on those features in addition to the platform capability. Shared by + * the SFTP path-confinement guard and the SCP send guards. */ +#if defined(WOLFSSH_HAVE_SYMLINK) && \ + (defined(WOLFSSH_SFTP) || defined(WOLFSSH_SCP)) + WOLFSSH_LOCAL int wIsSymlink(const char* path); /* Open a file for reading without following a final-component symlink. * Returns 0 on success, non-zero on failure, matching the wfopen