Skip to content

wolfsshd: open PID file with O_NOFOLLOW and a fixed mode #1074

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yosuke-wolfssl wants to merge 1 commit into
base: master
Choose a base branch
from
Open

wolfsshd: open PID file with O_NOFOLLOW and a fixed mode #1074

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 110 additions & 8 deletions apps/wolfsshd/configuration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 */

Expand All @@ -51,10 +63,13 @@

#include "configuration.h"

#ifndef WIN32
#ifndef _WIN32
#include <dirent.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/stat.h>
#endif
#ifdef WIN32
#ifdef _WIN32
#include <process.h>
#endif
#ifdef HAVE_LIMITS_H
Expand Down Expand Up @@ -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,
Comment thread
yosuke-wolfssl marked this conversation as resolved.

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [High] Existing regular PID files keep unsafe ownership or permissions
🚫 BLOCK bug

The new POSIX path relies on open(..., O_CREAT, 0644) to ensure the PID file is not writable by other users, but that mode is only applied when the file is newly created. If the PID path already exists as a single-link regular file, including an attacker-created file in the writable PID directory described by the PR or a stale 0666 file from a prior vulnerable run, the new fstat check accepts it and ftruncate rewrites it without normalizing permissions or rejecting unsafe ownership. That leaves the PID file writable by the existing owner or group/world despite the PR's explicit-mode hardening. The new test only covers creation of a fresh PID file, so this bypass is not exercised.

Recommendation: After fstat, reject files not owned by the daemon's effective uid/root and normalize the mode on accepted existing files before truncating, for example by checking st.st_uid and calling fchmod(fd, 0644). Add a regression test that pre-creates the PID path as a regular file with 0666 permissions, and preferably one owned by a different uid where feasible. Fix the existing-file path so the promised fixed permissions apply to reused PID files, or refuse unsafe existing files before truncation.

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [High] Existing regular PID files keep unsafe ownership or permissions
🚫 BLOCK bug

The new POSIX path relies on open(..., O_CREAT, 0644) to ensure the PID file is not writable by other users, but that mode is only applied when the file is newly created. If the PID path already exists as a single-link regular file, including an attacker-created file in the writable PID directory described by the PR or a stale 0666 file from a prior vulnerable run, the new fstat check accepts it and ftruncate rewrites it without normalizing permissions or rejecting unsafe ownership. That leaves the PID file writable by the existing owner or group/world despite the PR's explicit-mode hardening. The new test only covers creation of a fresh PID file, so this bypass is not exercised.

Recommendation: After fstat, reject files not owned by the daemon's effective uid/root and normalize the mode on accepted existing files before truncating, for example by checking st.st_uid and calling fchmod(fd, 0644). Add a regression test that pre-creates the PID path as a regular file with 0666 permissions, and preferably one owned by a different uid where feasible. Fix the existing-file path so the promised fixed permissions apply to reused PID files, or refuse unsafe existing files before truncation.

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
}
}

Expand Down
191 changes: 191 additions & 0 deletions apps/wolfsshd/test/test_configuration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[] = {
Expand All @@ -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),
Expand Down
Loading
Loading