wolfsshd: add StrictModes and secure loading of trust anchors#1042
Open
yosuke-wolfssl wants to merge 1 commit into
Open
wolfsshd: add StrictModes and secure loading of trust anchors#1042yosuke-wolfssl wants to merge 1 commit into
yosuke-wolfssl wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
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
StrictModesconfiguration directive (default enabled) and routedauthorized_keysloading 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.
ddc091c to
a0fb323
Compare
15e93e1 to
c91dcca
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1042
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Combines the fixes for findings 4114 (wolfsshd has no
StrictModesequivalent) 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
SearchForPubKeyopened a user'sauthorized_keyswith noownership/permission check, and there was no
StrictModesdirective. A localuser able to write the file (or a parent dir) could inject a key and
impersonate that account.
getBufferFromFileloaded the host key, host certificate, andTrustedUserCAKeyswith a plainfopen("rb")— nolstat/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 inauth.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:
lstat()— rejects a symlinked or non-regular leaf.realpath()+ ancestor walk to/— each parent directory must not begroup/world writable (tolerated only when sticky, e.g.
/tmp), which iswhat 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.
open(O_RDONLY|O_NOFOLLOW|O_NONBLOCK)thenfstat()on the fd — the filemust 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_inoarecompared against the pre-open
lstat()to close the swap window whereO_NOFOLLOWcompiles to0.On
_WIN32it falls back to a plain open (no POSIX uid model; relies on ACLs).4114 — StrictModes
StrictModesconfig directive (defaultyes, OpenSSH semantics).SearchForPubKeyopensauthorized_keysthrough 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
getBufferFromFilegains a load class (NORMAL/TRUST/SECRET) and delegatestrust-anchor loads to the gate: host key =
SECRET(also rejected ifgroup/world readable), host cert and user CA =
TRUST, owned by root or thedaemon'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
SetupCTXis removed — the host key is now covered bygetBufferFromFile(SECRET), which is strictly stronger. A secure-gate rejectionnow surfaces as
WS_BAD_FILE_Erather thanWS_MEMORY_E.Memory-safety fixes (pre-existing, in touched functions)
getBufferFromFileallocatedfileSz + 1but never wrote the terminating'\0'; the banner buffer is consumed as a C string bywolfSSH_CTX_SetBanner/WSTRLEN, which could read past the allocation. NowNUL-terminated.
SearchForPubKeyallocatedlineBufbut never freed it, leakingMAX_LINE_SZbytes per authentication attempt. Now freed on the return path.
Behavior changes
authorized_keysis no longer accepted (previously followed).the StrictModes footgun 4114 targets.
Testing
test_configuration.c): newtest_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.
run_all_sshd_tests.sh): self-contained host-keyownership/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 thesudo-launched daemon loads them.
wolfsshddirectly under sudo make the hostkey root-owned +
0600(and cert/CA root-owned) before launch.TrustedUserCAKeysrefused (attacker CA notinstalled); host key as symlink / world-writable / group-readable / under a
world-writable dir all refused; a valid
0600key loads, including under adirectory owned by a third party; banner loads with no ASan over-read.