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
65c65ac to
8df1316
Compare
A shell, exec or subsystem request changes the channel only once the callback accepts it. The session type and command are set for the callback to read and put back if it refuses, and CLIENT_DONE follows acceptance alone, so wolfSSH_accept() stays where it is rather than reporting a session it answered CHANNEL_FAILURE as established. - DoChannelRequestSession() carries the three arms, which differed only in the type and the callback consulted - a refusal puts the type and command back, so a grant an earlier request won still stands - FreeChannelCommand() wipes and releases a command line for both ChannelDelete() and the refusal path - unit.c drives a refused shell, exec and subsystem request through DoChannelRequest() and checks nothing was committed, and that a refusal after a grant puts the earlier command back whole - regress.c checks accept() stays at ACCEPT_SERVER_CHANNEL_ACCEPT_SENT on a refused shell, and that the sftp gate and both diverts ask for the grant alone Issue: F-8852
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, by length and bytes, so a name with an embedded NUL is some other subsystem - sshd_bad_subsystem_test.sh asks for an unknown subsystem with the OpenSSH client and expects the refusal
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.
| break; | ||
| } | ||
| #ifdef WOLFSSH_SCP | ||
| if (WSTRNCMP(cmd, "scp", 3) == 0) { |
There was a problem hiding this comment.
SCP prefix is granted as a valid exec request · Logic errors
SessionRequestCb() grants every exec command starting with scp, so an SCP-only daemon accepts names such as scpbackup and routes them into SCP handling. Known #11664 is the downstream classifier; this is the new daemon policy gate.
Suggested fix: Require a length-aware scp token boundary before granting, and reject embedded-NUL or otherwise unparseable SCP commands.
Basis: OpenBSD scp(1) SYNOPSIS defines scp as the command name followed by options and operands, not as a prefix of another command name.
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.What the daemon serves is unchanged.