fix: security hardening across wolfSSH#1098
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
Batch of Fenrir static-analysis findings in the security_sensitive theme: - #1684 src/agent.c: stop logging the plaintext passphrase in the agent PostLock/PostUnlock stubs; log only that a (un)lock was requested and ForceZero the stack buffer before return. - #4108 apps/wolfsshd/configuration.c: QNX Include-dir filter lstat'd dir->d_name (relative to CWD, not the include dir); build the full path with WSNPRINTF and treat snprintf/lstat failure as non-directory so the count and populate passes stay consistent. - #4794 src/wolfterm.c: reset escState to WC_ESC_NONE after the pre-loop CSI/OSC handling so a stale escape state is not carried over, and bounds-guard the buf[i] read in wolfSSH_DoControlSeq. - #5849 apps/wolfsshd/configuration.c: reject symlinks (DT_LNK) at both POSIX Include-dir filter sites (CWE-61) so a symlink in the Include dir is not followed by fopen and parsed as config. - #5852 src/internal.c: in wolfSSH_CleanPath parent-collapse, only match a '..' component that ends at a delimiter or NUL, guarding against '..X'.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR applies a set of security hardening fixes across wolfSSH in response to Fenrir static-analysis findings, focusing on preventing sensitive-data leakage, tightening path/config handling, and fixing edge-case parsing behavior.
Changes:
- Remove plaintext passphrase logging in agent lock/unlock stubs and scrub the temporary stack buffer.
- Harden Include-dir handling in
wolfsshd(QNX full-pathlstat(); POSIX symlink filtering viaDT_LNK). - Fix terminal escape-sequence parsing edge cases (prevent OOB read; reset stale escape-state) and tighten
wolfSSH_CleanPath..component matching.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/wolfterm.c | Adds bounds checking in CSI parsing and clears stale escape state between calls. |
| src/internal.c | Tightens .. component detection to avoid misinterpreting ..X as parent traversal. |
| src/agent.c | Stops logging passphrases and zeroizes the temporary stack buffer used for lock/unlock. |
| apps/wolfsshd/configuration.c | Fixes QNX include-dir lstat() to use full path and adds symlink filtering in POSIX include-dir scans. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+476
to
+478
| if (i >= bufSz) { | ||
| return WS_FATAL_ERROR; | ||
| } |
Comment on lines
+796
to
+798
| if (qnxRet < 0 || qnxRet >= PATH_MAX || | ||
| lstat(filepath, &s) != 0 || | ||
| !S_ISDIR(s.st_mode)) |
Comment on lines
+838
to
+840
| if (qnxRet < 0 || qnxRet >= PATH_MAX || | ||
| lstat(filepath, &s) != 0 || | ||
| !S_ISDIR(s.st_mode)) |
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.
Batch of security_sensitive findings from Fenrir static analysis. All fixes build-verified together against wolfSSL (--enable-all --enable-ssh) with
./configure --enable-all && make -j8(exit 0); the QNX and Windows-only paths were source/syntax-analyzed (gcc -D__QNX__ -fsyntax-only).src/agent.c— the agentPostLock/PostUnlockstubs logged the plaintext passphrase. Log only that a (un)lock was requested andForceZerothe stack buffer before return.apps/wolfsshd/configuration.c— the QNX Include-dir filter calledlstat(dir->d_name, ...), which resolves relative to CWD, not the include dir. Build the full path withWSNPRINTFand treat snprintf/lstat failure as non-directory so the count pass and populate pass stay consistent.src/wolfterm.c— resetescStatetoWC_ESC_NONEafter the pre-loop CSI/OSC handling so a stale escape state is not carried into the next call, and bounds-guard thebuf[i]read inwolfSSH_DoControlSeq(OOB read).apps/wolfsshd/configuration.c— reject symlinks (DT_LNK) at both POSIX Include-dir filter sites (CWE-61) so a symlink in the Include dir is not followed byfopenand parsed as config.src/internal.c— inwolfSSH_CleanPathparent-collapse, only match a..component that ends at a delimiter or NUL, guarding against..Xcomponents being mistaken for parent references.Reported by Fenrir static analysis.