wolfsshd: open PID file with O_NOFOLLOW and a fixed mode

[yosuke-wolfssl]

wolfsshd: harden PID-file write against symlink, hard-link, and FIFO attacks

Summary

wolfSSHD_ConfigSavePID wrote the PidFile with WFOPEN(..., "wb") — plain
fopen() on POSIX: no O_NOFOLLOW, no explicit mode. The daemon sets
umask(0) during daemonization and calls this as root, before privilege
drop. A local user with write access to the PidFile directory could:

• pre-place a symlink at the PID path → fopen follows it and truncates
  an arbitrary root-owned file, writing the daemon PID into it; and
• under umask(0), have the PID file created mode 0666 (world-writable).

Exploitability is conditional (the default config leaves PidFile unset and
/var/run is root-owned), so impact is limited to non-default PidFile
locations, container/embedded deployments with a non-root-writable PID
directory, or first-install windows.

Addressed by f_5853.

Fix — apps/wolfsshd/configuration.c

The POSIX path now opens without O_TRUNC, validates the descriptor, then
truncates and writes:

fd = open(conf->pidFile, O_WRONLY | O_CREAT | O_NONBLOCK | O_NOFOLLOW, 0644);
/* refuse unless a single-link regular file, then ftruncate + fdopen + write */

Defenses, each mapping to a concrete attack:

Measure	Stops
O_NOFOLLOW (+ lstat fallback where unavailable)	symlink redirect / truncation
explicit 0644 mode	world-writable PID file under umask(0)
fstat → S_ISREG && st_nlink == 1	hard link onto a root-owned file (which O_NOFOLLOW does not cover)
O_NONBLOCK on open	a planted FIFO stalling sshd startup before the S_ISREG check
no O_TRUNC; ftruncate only after validation	clobbering a refused target
WFWRITE/WFCLOSE return checked; unlink on failure	leaving a truncated / zero-length PID file

Other notes:

• Truncation now happens (ftruncate) only after the descriptor is proven a
  single-link regular file, so nothing is clobbered when a target is refused.
• _XOPEN_SOURCE/_GNU_SOURCE are defined before the system headers (as
  auth.c does) so fdopen/ftruncate/lstat/S_ISLNK are declared under
  strict -std= builds.
• The Windows branch keeps the portable WFOPEN path, with the same
  WFWRITE/WFCLOSE checks and WREMOVE-on-failure.

Supporting change — wolfssh/port.h

The WOLFSSH_O_NOFOLLOW portability macro (and its <fcntl.h> include) was
gated behind (WOLFSSH_SFTP || WOLFSSH_SCP), so it was undefined in an
--enable-sshd-only build. It is moved into a WOLFSSH_HAVE_SYMLINK block with
a header-wide 0 catch-all, so SFTP, SCP, and SSHD share one definition.

Tests — apps/wolfsshd/test/test_configuration.c

New test_ConfigSavePID (POSIX) drives the real config loader +
wolfSSHD_ConfigSavePID:

• normal PidFile written with the daemon PID, not group/world writable
  even under umask(0);
• a symlinked PID path is refused, link target intact;
• a hard-linked PID path is refused, link target intact;
• a FIFO at the PID path is refused (fast-fail) and left in place.

The symlink-refusal assertion is gated on WOLFSSH_HAVE_SYMLINK so the test is
also correct under a WOLFSSH_NO_SYMLINK_CHECK build.

Scope note

This intentionally does not alter the daemon-wide umask. The PID file is
protected by its explicit 0644 mode, so changing the process umask (which
would also affect SFTP/SCP client-requested upload modes) was left out to keep
the change scoped to the finding. This matches OpenSSH, which relies on explicit
modes plus a root-owned PidFile directory and does no symlink/hard-link checks
itself; operators should still place PidFile in a root-owned directory.

Verification

• Builds clean (0 compiler warnings) across --enable-all, --enable-scp,
  --enable-sftp, --enable-scp --disable-sftp, --enable-sftp --disable-scp,
  and --enable-sshd (no SFTP/SCP).
• Negative controls: built against the pre-fix source and confirmed the symlink,
  mode, and hard-link assertions all fail without the fix; a standalone
  check confirms O_NONBLOCK is what prevents the FIFO open from blocking.
• codespell clean.

Files

apps/wolfsshd/configuration.c           | O_NOFOLLOW + 0644, st_nlink/S_ISREG, O_NONBLOCK, ftruncate-after-check, WFWRITE/WFCLOSE checks, feature-test macros
apps/wolfsshd/test/test_configuration.c | test_ConfigSavePID (normal / mode / symlink / hard-link / FIFO)
wolfssh/port.h                          | centralize WOLFSSH_O_NOFOLLOW for SSHD

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1074

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1074

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

[aidangarske]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 1 total — 1 posted, 0 skipped

Posted findings

• [High] Existing regular PID files keep unsafe ownership or permissions — apps/wolfsshd/configuration.c:1828-1845

Review generated by Skoll.

[aidangarske]

🟠 [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]

🐺 Skoll Code Review

Overall recommendation: REQUEST_CHANGES
Findings: 1 total — 1 posted, 0 skipped

Posted findings

• [High] Existing regular PID files keep unsafe ownership or permissions — apps/wolfsshd/configuration.c:1828-1845

Review generated by Skoll.

[aidangarske]

🟠 [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.