Repository navigation
Refuse channels the server opens back, and type the startup command into the shell - #148
Open
Sadykhzadeh wants to merge 6 commits into
Open
Sadykhzadeh wants to merge 6 commits into
Sadykhzadeh wants to merge 6 commits into
Conversation
PR Summary by QodoReject server-initiated SSH channels and run startup commands in the shell
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo |
Sadykhzadeh
force-pushed
the
security/refuse-server-channels
branch
from
October 16, 2026 23:08
e186201 to
0f286f8
Compare
…nto the shell russh's client handler accepts every channel a server opens towards the client: agent and X11 forwarding, forwarded and direct TCP/streamlocal, even a session. Clavyn requests none of those, and an accepted channel that nothing reads stays registered for the life of the connection, so a server could open them without limit. SshHandler now rejects them all. The host startup command was sent as an exec request after the shell request on the same channel. A channel takes one of shell, exec or subsystem, and OpenSSH ignores an exec on a channel that is already running a shell, so the command never ran. It is now written into the shell followed by a carriage return, as if typed.
The command was written before the reader task started, so a long command whose echo filled the receive window could stall the connect waiting on its own output. It now goes through the session's ordinary write path after the reader is up, and a command that cannot be sent is logged instead of failing a session that is otherwise usable.
Sadykhzadeh
force-pushed
the
security/refuse-server-channels
branch
from
October 17, 2026 06:44
0f286f8 to
8bc2f93
Compare
Unknown server-initiated channel types were refused only because russh's should_accept_unknown_server_channel happens to default to false. The dependency is a caret requirement, so a patch release could change that default or add a typed hook whose default accepts, reopening the leak the other refusals close. The handler now answers both unknown-type hooks itself, and a unit test pins the refusal regardless of russh's default.
The startup write looked the session up by id again after inserting it. A shell that exits at once frees the id, so a session that took it in that gap would receive this one's command. The write now goes through the Arc that was just inserted, which also keeps the id reserved until the send is over. write() shares the same send path.
The refusal test opened only agent, x11, forwarded-tcpip and session channels, so dropping the forwarded-streamlocal, direct-tcpip or direct-streamlocal override would have passed unnoticed. It now opens all seven types russh can open back. The in-process loopback server, its config, the password host and the known-hosts setup were copied across server_requests, session_isolation and ssh_roundtrip. They now live in tests/common/loopback.rs.
The command was written into the PTY right after the shell request, before the login shell had run its startup files. Anything there that reads input could take it, and the terminal echoed it in the middle of the login banner and again after the prompt. It now waits for the shell's output to start and then pause for a second, which is how a shell sitting at its prompt looks from outside, with a five-second cap so a shell that prints nothing, or never stops, still gets it. The wait runs on its own task, so the connect returns as before. The command takes the session's first write turn before the session is reachable, so input sent while it waits reaches the shell after it, in order, instead of being spliced in front of it.
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.
Closes #147. Stacked on #140 (
session.rs). Merge #140 first; this PR's base then becomesmain.What this changes
Server-initiated channels are refused
SshHandlernow overrides everyserver_channel_open_*hook russh 0.63.3 offers and rejects each one withAdministrativelyProhibited:Unknown channel types were already refused, since russh's
should_accept_unknown_server_channeldefaults tofalse. Clavyn requests no forwarding of any kind, so no legitimate flow is affected. If agent forwarding is added later, it has to opt back in here, deliberately.The startup command is typed into the shell
A session channel takes one of shell, exec or subsystem. OpenSSH ignores an exec that arrives after the shell, so
startup_commandnever ran. It is now written into the shell followed by\r, the bytes Enter sends, which matches what "startup command" means in a terminal client: the shell starts and the command runs in it.Tests
core/tests/server_requests.rsruns against an in-process russh server.channels_the_server_opens_back_are_refused: once the shell starts, the server tries to open agent, x11, forwarded-tcpip and session channels, and each open must fail.the_startup_command_is_typed_into_the_shell: the server must receive exactlytmux attach\ron the shell channel.Both fail on the base branch and pass here.
Test plan
cargo test -p clavyn-core: all suites pass (118 + 5 + 32 + 2 + 3 + 9)node scripts/verify/check-comments.mjs: okNeed help on this PR? Tag
@codesmith-botwith what you need.