fix: security hardening across wolfSSH

[MarkAtwood]

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

• #1684 src/agent.c — the agent PostLock/PostUnlock stubs logged the plaintext passphrase. Log only that a (un)lock was requested and ForceZero the stack buffer before return.
• #4108 apps/wolfsshd/configuration.c — the QNX Include-dir filter called lstat(dir->d_name, ...), which resolves relative to CWD, not the include dir. Build the full path with WSNPRINTF and treat snprintf/lstat failure as non-directory so the count pass and populate pass 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 into the next call, and bounds-guard the buf[i] read in wolfSSH_DoControlSeq (OOB read).
• #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 components being mistaken for parent references.

Reported by Fenrir static analysis.

[Copilot]

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-path lstat(); POSIX symlink filtering via DT_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.

[aidangarske]

🐺 Skoll Code Review

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

Posted findings

• [High] QNX Include filtering still follows symlinked config files — apps/wolfsshd/configuration.c:796-840
• [High] Split CSI arguments dropped / force fatal console conversion error after new bounds guard — src/wolfterm.c:474-478

Review generated by Skoll.

[aidangarske]

🟠 [High] QNX Include filtering still follows symlinked config files
🚫 BLOCK bug

The PR adds explicit DT_LNK symlink rejection in the non-QNX dir->d_type branch, but the changed QNX branch still treats every non-directory lstat() result as an includable config file. For a symlink, lstat(filepath, &s) returns link metadata, !S_ISDIR(s.st_mode) is true, the name is added to fileNames, and ConfigLoad(conf, filepath, depth) opens it with WFOPEN, which follows the symlink. Data flow: admin-configured Include dir/*.conf -> attacker-controlled symlink entry in that include dir -> QNX lstat accepts link -> ConfigLoad/WFOPEN parses the symlink target as sshd config. This leaves the QNX path exposed and the symlink filter incomplete in both the count and populate passes. Severity views differ (review: High/BLOCK; security: Low, Medium confidence, limited local/configuration-bound issue); the stricter severity is retained because the gap is in the PR's own symlink filter.

Recommendation: After a successful lstat(), include only entries that are neither directories nor symlinks, and treat lstat failure as skip/fail-closed, applying the same predicate in both the count and populate passes: if (qnxRet >= 0 && qnxRet < PATH_MAX && lstat(filepath, &s) == 0 && !S_ISDIR(s.st_mode) && !S_ISLNK(s.st_mode)). Add an Include wildcard regression test that plants a symlink in the include directory.

[aidangarske]

🟠 [High] Split CSI arguments dropped / force fatal console conversion error after new bounds guard
🚫 BLOCK bug

The new i >= bufSz guard correctly prevents the OOB buf[i] read, but in the resume path where ssh->escState == WS_ESC_CSI and ssh->escBufSz == 0, i >= bufSz means the CSI sequence is incomplete, not invalid. A valid split sequence such as ESC [ then 31/12 then m no longer buffers the partial argument chunk: wolfSSH_DoControlSeq() returns WS_FATAL_ERROR, and since wolfSSH_ConvertConsole() only returns on WS_WANT_READ, it clears escState and emits the partial CSI bytes as normal text, letting a remote peer controlling terminal output abort Windows console conversion. Data flow: remote SSH channel output -> wolfSSH_ConvertConsole -> stored WS_ESC_CSI state -> next partial CSI argument buffer -> getArgs consumes it -> new return WS_FATAL_ERROR. Existing tests cover ESC, [, then a complete 4m, but not a second partial argument fragment. Severity views differ (review: High/BLOCK; security: Low, High confidence); the stricter severity is retained.

Recommendation: Mirror the fresh-CSI path: preserve the incomplete fragment and return WS_WANT_READ instead of WS_FATAL_ERROR, e.g. if (i >= bufSz) { if (bufSz - *idx > WOLFSSL_MAX_ESCBUF) return WS_FATAL_ERROR; WMEMCPY(ssh->escBuf, buf + *idx, bufSz - *idx); ssh->escBufSz = bufSz - *idx; ssh->escState = WS_ESC_CSI; return WS_WANT_READ; }. Add a Windows wolfSSH_ConvertConsole() test for ESC [ / 31 / m split across three calls.

[ejohnstown]

Closing -- four of the five findings have landed on master since this was opened, and the fifth was declined.

Already on master, verified site by site against e393bcb5:

• #1684 src/agent.c -- PostLock/PostUnlock now gate the passphrase log behind SHOW_SECRETS and WS_FORCEZERO the buffer (which also moved to the heap).
• #4108 apps/wolfsshd/configuration.c -- the QNX filter builds the full path with WSNPRINTF and lstats that, at both the count and the populate pass. Master's form is the safer one: it excludes an entry when snprintf/lstat fails, where this PR includes it.
• #4794 src/wolfterm.c -- escState is reset to WC_ESC_NONE in both wolfSSH_ConvertConsole branches, and the truncated-CSI case is handled. Worth noting the PR's version of that second half would be a regression: it returns WS_FATAL_ERROR when getArgs() runs off the end, where master saves the leftovers and returns WS_WANT_READ. Taking it would re-break split CSI sequences and re-open the BLOCK already filed on this PR.
• #5852 src/internal.c -- wolfSSH_CleanPath has the identical (path[i+3] == WS_DELIM || path[i+3] == '\0') guard on the parent-collapse.

#5849 (the Include-dir symlink filter) is the only hunk left, and Fenrir closed it wont_fix: planting a symlink in the Include directory requires write access to that directory, and write access already grants full config injection through a plain .conf file. The symlink adds no capability.

Rebasing it onto master to see what it would cost showed the change is a net negative. Loading a config whose Include directory holds a symlinked .conf that sets PermitEmptyPasswords yes:

master           : ConfigLoad ret=0  PermitEmptyPw=1   -> symlinked config parsed
with this change : ConfigLoad ret=0  PermitEmptyPw=0   -> symlinked config skipped

ret=0 either way -- the config is dropped with no error and no log line. Symlinking into a conf.d-style directory is a common admin idiom, and OpenSSH's own Include globs and opens with no symlink filter at all, so this makes wolfsshd stricter than the thing it models and fails closed silently.

It also would not clear this PR's own blocker: the change touches only the POSIX branch, so the High/BLOCK on the QNX branch survives it.

Separately, if we do want to harden this path later, refusing to open symlinks isn't the shape of the fix -- checking the resolved target's ownership and permissions the way StrictModes already does for authorized_keys is. That's a design decision rather than an extra term in an if.

Thanks for the batch -- the four that landed came out of it.