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
+481 −22
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
127 changes: 119 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,14 @@

#include "configuration.h"

#ifndef WIN32
#ifndef _WIN32
#include <dirent.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/stat.h>
#include <errno.h>
#endif
#ifdef WIN32
#ifdef _WIN32
#include <process.h>
#endif
#ifdef HAVE_LIMITS_H
Expand Down Expand Up @@ -1779,18 +1795,113 @@ 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;
int created = 1; /* whether this call created the PID file */
struct stat st;
#endif

if (conf->pidFile != NULL) {
WMEMSET(buf, 0, sizeof(buf));
#ifndef _WIN32
/* raw open, not WFOPEN: the O_NOFOLLOW hardening needs a real fd */
#if defined(WOLFSSH_HAVE_SYMLINK) && WOLFSSH_O_NOFOLLOW == 0
/* no O_NOFOLLOW here, so pre-check with a racy best-effort lstat */
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
if (ok) {
/* O_EXCL first so 'created' distinguishes creation from reuse; on
* EEXIST reopen (O_NOFOLLOW still guards a symlink). No O_TRUNC and
* O_NONBLOCK so a planted FIFO fails fast, nothing clobbered. */
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.

@yosuke-wolfssl yosuke-wolfssl Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — confirmed and fixed. The 0644 in open(..., O_CREAT, 0644)
only applies on creation, so a pre-existing PID file kept its old mode and
owner. Two changes in wolfSSHD_ConfigSavePID, both before ftruncate:

  • Owner gate in the fstat check: refuse unless a single-link regular file
    owned by our euid or root, so an attacker-planted file is never adopted.
  • fchmod(fd, 0644) on accepted files, normalizing a stale world-writable
    PID file instead of reusing it as-is.

This runs as root under umask(0) before the privilege drop, so geteuid()==0:
a root-owned reuse is accepted and re-chmod'd, an attacker-owned plant refused.

Regression tests in test_ConfigSavePID: a pre-created 0666 file → rewritten
and mode reset; a foreign-owned file → refused (root-gated CI). Each guard has a
negative control.

@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.

@yosuke-wolfssl yosuke-wolfssl Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Duplicated comment

O_WRONLY | O_CREAT | O_EXCL | O_NONBLOCK |
WOLFSSH_O_NOFOLLOW, 0644);
if (fd < 0 && errno == EEXIST) {
created = 0;
fd = open(conf->pidFile,
O_WRONLY | O_NONBLOCK | WOLFSSH_O_NOFOLLOW, 0);
}
if (fd < 0) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Unable to open PID file %s", conf->pidFile);
}
/* O_NOFOLLOW stops a symlink but not a hard link; demand a
* single-link regular file owned by us or by root. */
else if (fstat(fd, &st) != 0 || !S_ISREG(st.st_mode) ||
st.st_nlink != 1 ||
(st.st_uid != geteuid() && st.st_uid != 0)) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Refusing unsafe PID file %s", conf->pidFile);
close(fd);
}
/* open()'s 0644 applies only on creation, so an existing
* world-writable PID file needs its mode reset here. */
else if (fchmod(fd, 0644) != 0) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Unable to set mode on PID file %s", conf->pidFile);
close(fd);
/* remove only a file we just made, never a valid existing one */
if (created) {
unlink(conf->pidFile);
}
}
else if (ftruncate(fd, 0) != 0) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Unable to truncate PID file %s", conf->pidFile);
close(fd);
if (created) {
unlink(conf->pidFile);
}
}
else {
f = fdopen(fd, "wb");
if (f == NULL) {
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Unable to open PID file stream %s",
conf->pidFile);
close(fd);
/* drop the empty leftover, a reader must not see no PID */
unlink(conf->pidFile);
}
else {
WSNPRINTF(buf, sizeof(buf), "%d", getpid());
pidLen = (word32)WSTRLEN(buf);
writeOk = ((word32)WFWRITE(NULL, buf, 1, pidLen, f) == pidLen);
if (WFCLOSE(NULL, f) != 0) {
writeOk = 0;
}
if (!writeOk) {
/* a reader must not parse a truncated PID */
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);
if (WFCLOSE(NULL, f) != 0) {
writeOk = 0;
}
if (!writeOk) {
/* a reader must not parse a truncated PID */
wolfSSH_Log(WS_LOG_ERROR,
"[SSHD] Failed to write PID file %s", conf->pidFile);
WREMOVE(NULL, conf->pidFile);
}
}
#endif
}
}

Expand Down
Loading
Loading