wolfsshd: refuse sessions it cannot serve - #1237
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Public API/documentation mismatches and an overly broad SCP command match should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates server-side session/channel request handling so unsupported shell/exec/subsystem requests are refused immediately (with CHANNEL_FAILURE) instead of being accepted and later dropped, and adds an “application-driven channels” mode where wolfSSH_accept() returns after user authentication and the application drives channels via wolfSSH_worker() and callbacks.
Changes:
- Add
appChannelsmode (ctx + session setters) and updatewolfSSH_accept()to optionally stop atACCEPT_SERVER_USERAUTH_SENT. - Refactor session channel-request handling so callbacks can reject without committing session type/command or advancing to
CLIENT_DONE. - Add unit/regression coverage and a new OpenSSH-based
wolfsshdtest for refusing an unknown subsystem.
File summaries
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents new application-driven channel mode and adds public setters. |
| wolfssh/internal.h | Adds appChannels fields to WOLFSSH_CTX and WOLFSSH. |
| wolfssh/agent.h | Adds public API declaration for opening the agent forwarding channel in app-driven mode. |
| src/ssh.c | Implements appChannels accept stop-state behavior and adds setters; uses new agent channel-open helper. |
| src/internal.c | Introduces DoChannelRequestSession() to avoid committing session state on rejected requests. |
| src/agent.c | Adds wolfSSH_AGENT_ChannelOpen() implementation for server-side agent forwarding channel open. |
| src/wolfsftp.c | Adjusts wolfSSH_SFTP_accept() preconditions for app-driven accept behavior. |
| apps/wolfsshd/wolfsshd.c | Adds a session-request callback to refuse unserviceable session types/commands; wires request context. |
| tests/unit.c | Adds unit coverage ensuring rejected session requests do not commit state and respond with failure. |
| tests/regress.c | Adds regression coverage for app-driven accept behavior and rejected session requests. |
| apps/wolfsshd/test/sshd_bad_subsystem_test.sh | New OpenSSH client regression test for refusing unknown subsystems. |
| apps/wolfsshd/test/run_all_sshd_tests.sh | Adds the new subsystem test to the test runner. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f0671c9 to
7bfe298
Compare
8df1316 to
fb79efb
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1237
Scan targets checked: wolfssh-src, wolfssh-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
fb79efb to
4f5869d
Compare
DoChannelRequestSession() serves the shell, exec and subsystem arms together, and its debug line labels the string it read. A subsystem request labels that string "subsystem" rather than "command". - unit.c: renumber the shell-after-exec failure to follow the codes above it
4f5869d to
2e3519a
Compare
A shell, exec or subsystem request is answered as it arrives, through the channel request callbacks, so a session this build cannot serve, or a second one on a channel already running one, is refused with CHANNEL_FAILURE rather than accepted and then dropped once the session is up. What the daemon serves is unchanged. - SessionRequestCb() takes a shell with WOLFSSH_SHELL, an exec with WOLFSSH_SHELL or an scp command with WOLFSSH_SCP, and the sftp subsystem with WOLFSSH_SFTP; anything else is refused and logged - a request whose command did not fit is refused rather than read through a NULL - a second program start is refused on a channel whose grant already stands, so sftp or scp cannot take over a running session - the sftp name is matched whole and scp only as its own token, both by length and bytes, so "scpbackup" or a name with an embedded NUL is some other command - sshd_bad_subsystem_test.sh asks for an unknown subsystem with the OpenSSH client and expects the refusal
2e3519a to
7a8ff75
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1237
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
padelsbach
left a comment
padelsbach
left a comment
There was a problem hiding this comment.
Couple minor points, but looks good overall
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1237
Scan targets checked: wolfssh-src, wolfssh-bugs
Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Reported findings require changes before merge.
wolfSSH_ChannelCommandIsScp() reports whether a channel's session command starts an SCP transfer, taking "scp" only as its own token. The divert in wolfSSH_accept() and an application's exec callback both ask it, so the two cannot disagree about what the SCP server is handed. - an exec of "scpbackup foo" runs as an ordinary exec - a command carrying a NUL within the recorded command size is not an SCP command; ParseScpCommand() walks a C string, so a NUL would drop whatever follows it - wolfsshd's session callback asks through the same helper
wolfSSH_ChannelGetSessionGranted() reports whether a shell, exec or subsystem request on a channel has been answered CHANNEL_SUCCESS, so an application can tell a second request from the first without reaching into WOLFSSH_CHANNEL. - the flag is still clear for the request a session callback is answering, so a set flag is an earlier request's grant - wolfsshd's session callback reads the grant through it
9609cc9 to
f31d9c5
Compare
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1237
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Fenrir's latest completed scan found no issues; clearing the prior automated change request.
A shell, exec or subsystem request is answered as it arrives, so a session this build cannot serve is refused with CHANNEL_FAILURE rather than accepted and then dropped once the session is up.
SessionRequestCb()takes a shell withWOLFSSH_SHELL, an exec withWOLFSSH_SHELLor an scp command withWOLFSSH_SCP, and the sftp subsystem withWOLFSSH_SFTP; anything else is refused and logged.NULL.wolfSSH_ChannelCommandIsScp()takes "scp" only as its own token and is shared with the SCP divert inwolfSSH_accept(), so an exec ofscpbackup foono longer reaches the built-in SCP server.wolfSSH_ChannelGetSessionGranted()reports a channel's grant, so an application need not readWOLFSSH_CHANNEL.What the daemon serves is otherwise unchanged.