Skip to content

wolfsshd: add StrictModes and secure loading of trust anchors#1042

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/strictModes
Open

wolfsshd: add StrictModes and secure loading of trust anchors#1042
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/strictModes

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Combines the fixes for findings 4114 (wolfsshd has no StrictModes
equivalent) and 5901 (trust-anchor files loaded without an ownership or
symlink check) into one change, built on a single secure-open routine so every
security-critical file wolfsshd opens goes through the same gate.

Supersedes #1010 (4114) and #1016 (5901).

Background

  • 4114: SearchForPubKey opened a user's authorized_keys with no
    ownership/permission check, and there was no StrictModes directive. A local
    user able to write the file (or a parent dir) could inject a key and
    impersonate that account.
  • 5901: getBufferFromFile loaded the host key, host certificate, and
    TrustedUserCAKeys with a plain fopen("rb") — no lstat/fstat/O_NOFOLLOW/
    ownership gate. A local user who plants a symlink (or controls a writable
    ancestor directory) before the root daemon starts could substitute an attacker
    CA and authenticate as anyone.

The two findings overlap on the host key and previously made contradictory
decisions there. This PR unifies them.

Approach

New helper wolfSSHD_OpenSecureFile() (apps/wolfsshd/auth.c, declared in
auth.h) opens a trusted file and fails closed on a symlink, bad ownership,
or unsafe permissions. It replaces the previous wolfSSHD_CheckFilePermissions()
(removed). On POSIX it:

  1. lstat() — rejects a symlinked or non-regular leaf.
  2. realpath() + ancestor walk to / — each parent directory must not be
    group/world writable (tolerated only when sticky, e.g. /tmp), which is
    what would let another user rename and swap the file. Ancestor ownership is
    not enforced: an attacker controlling a non-writable directory still cannot
    plant a file owned by the target user/root, which the leaf owner-check below
    rejects.
  3. open(O_RDONLY|O_NOFOLLOW|O_NONBLOCK) then fstat() on the fd — the file
    must be a regular file owned by the expected user or root, not group/world
    writable, and (for secrets) not group/world readable; st_dev/st_ino are
    compared against the pre-open lstat() to close the swap window where
    O_NOFOLLOW compiles to 0.

On _WIN32 it falls back to a plain open (no POSIX uid model; relies on ACLs).

4114 — StrictModes

  • Adds the StrictModes config directive (default yes, OpenSSH semantics).
  • SearchForPubKey opens authorized_keys through the gate (owner = the user,
    no symlink, no group/world-writable path component) when StrictModes is on, and
    falls back to a plain open when off.

5901 — trust-anchor substitution

  • getBufferFromFile gains a load class (NORMAL/TRUST/SECRET) and delegates
    trust-anchor loads to the gate: host key = SECRET (also rejected if
    group/world readable), host cert and user CA = TRUST, owned by root or the
    daemon's euid. The banner stays a direct open.

The trust-anchor checks always run, independent of StrictModes (matching
OpenSSH, which never relaxes host-key/system-file checks). The earlier inline
host-key check in SetupCTX is removed — the host key is now covered by
getBufferFromFile(SECRET), which is strictly stronger. A secure-gate rejection
now surfaces as WS_BAD_FILE_E rather than WS_MEMORY_E.

Memory-safety fixes (pre-existing, in touched functions)

  • getBufferFromFile allocated fileSz + 1 but never wrote the terminating
    '\0'; the banner buffer is consumed as a C string by
    wolfSSH_CTX_SetBanner/WSTRLEN, which could read past the allocation. Now
    NUL-terminated.
  • SearchForPubKey allocated lineBuf but never freed it, leaking MAX_LINE_SZ
    bytes per authentication attempt. Now freed on the return path.

Behavior changes

  • A symlinked authorized_keys is no longer accepted (previously followed).
  • A group/world-writable path component is rejected (unless sticky), matching
    the StrictModes footgun 4114 targets.

Testing

  • Unit (test_configuration.c): new test_OpenSecureFile — good/bad owner,
    group/world writable and readable, sticky vs non-sticky ancestors, symlink-leaf
    rejection, non-regular target. 14/14 pass, clean under ASan + UBSan.
  • Functional (run_all_sshd_tests.sh): self-contained host-key
    ownership/symlink gate test; the host-key negative test asserts the always-on
    gate.
  • start_sshd.sh: copies configured host key/cert/CA into a private dir,
    makes the copies root-owned and 0600, and points a temp config at them so the
    sudo-launched daemon loads them.
  • CI workflows: steps launching wolfsshd directly under sudo make the host
    key root-owned + 0600 (and cert/CA root-owned) before launch.
  • Runtime checks: symlinked TrustedUserCAKeys refused (attacker CA not
    installed); host key as symlink / world-writable / group-readable / under a
    world-writable dir all refused; a valid 0600 key loads, including under a
    directory owned by a third party; banner loads with no ASan over-read.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Jun 17, 2026
Copilot AI review requested due to automatic review settings June 17, 2026 04:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens wolfsshd’s file-loading paths by introducing an OpenSSH-like StrictModes control for authorized_keys and enforcing always-on secure loading for trust-anchor files (host key/cert and TrustedUserCAKeys) via a shared secure-open gate.

Changes:

  • Added StrictModes configuration directive (default enabled) and routed authorized_keys loading through the secure gate when enabled.
  • Introduced a unified secure-open helper (wolfSSHD_OpenSecureFile) and updated trust-anchor loads to use it with load classes (NORMAL/TRUST/SECRET).
  • Expanded unit/functional tests and adjusted scripts/workflows to ensure trust-anchor files satisfy the new ownership/permission requirements under sudo.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
apps/wolfsshd/wolfsshd.c Adds load classes for getBufferFromFile() and routes trust-anchor loads through wolfSSHD_OpenSecureFile().
apps/wolfsshd/auth.c Implements wolfSSHD_OpenSecureFile() and applies StrictModes gating to authorized_keys open path.
apps/wolfsshd/auth.h Declares wolfSSHD_OpenSecureFile() for shared use.
apps/wolfsshd/configuration.c Adds StrictModes parsing/storage/copy behavior and a getter.
apps/wolfsshd/configuration.h Exposes wolfSSHD_ConfigGetStrictModes().
apps/wolfsshd/test/test_configuration.c Adds StrictModes parse/default/copy assertions and a POSIX secure-open unit test.
apps/wolfsshd/test/start_sshd.sh Copies configured trust anchors to a private temp dir and makes them root-owned/0600 for sudo-launched daemons.
apps/wolfsshd/test/run_all_sshd_tests.sh Adds negative tests for trust-anchor gating and StrictModes behavior.
.github/workflows/x509-interop.yml Ensures host key/cert/CA meet secure-gate requirements when starting wolfsshd under sudo.
.github/workflows/sshd-test.yml Ensures host key is root-owned and 0600 for sudo-launched wolfsshd in CI.
.github/workflows/paramiko-sftp-test.yml Ensures host key ownership/permissions satisfy secure-gate checks under sudo.
Comments suppressed due to low confidence (1)

apps/wolfsshd/auth.c:760

  • SearchForPubKey() mixes the WFILE/WFOPEN API with XBADFILE (used with XFOPEN). If XBADFILE differs from WBADFILE on a given port, this can cause incorrect open/close logic. Initialize to WBADFILE to match the rest of the function’s WFILE usage.
    int ret = WSSHD_AUTH_SUCCESS;
    char authKeysPath[MAX_PATH_SZ];
    WFILE *f = XBADFILE;
    char* lineBuf = NULL;
    char* current;

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread apps/wolfsshd/wolfsshd.c
Comment thread apps/wolfsshd/auth.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Fenrir Automated Review — PR #1042

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants