Guard remote forwards on a client, disconnect on a disallowed message - #1214
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves RFC conformance in wolfSSH’s core receive/dispatch path and forwarding global-request handling by (1) adding a client-side role guard for remote-forward global requests and (2) sending an SSH DISCONNECT message when an incoming message is rejected by the session state machine.
Changes:
- Reject
tcpip-forwardandcancel-tcpip-forwardglobal requests when received by a client (optionally replying withREQUEST_FAILUREwithout parsing the request body). - Send
SSH_MSG_DISCONNECTwithPROTOCOL_ERRORwhenIsMessageAllowed()rejects an inbound message id (unless already disconnected). - Expand regress tests to cover the new client/server forwarding behavior and the disconnect-on-disallowed-message behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/internal.c |
Adds client-side guard for remote-forward global requests and sends DISCONNECT on state-disallowed inbound messages. |
tests/regress.c |
Adds harness support for client-side forwarding tests and updates/adds regress coverage for new disconnect and guard behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1214
Scan targets checked: wolfssh-bugs, wolfssh-src
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.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1214
Scan targets checked: wolfssh-bugs, wolfssh-src
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.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1214
Scan targets checked: wolfssh-bugs, wolfssh-src
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.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1214
Scan targets checked: wolfssh-bugs, wolfssh-src
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 client answers tcpip-forward and cancel-tcpip-forward with a failure, RFC 4254 section 7.1, before the request body is parsed. DoPacket() disconnects on a refused id this build implements and on any refused id of 80 or higher, the range RFC 4252 section 6 names; a refused id below 80 is answered UNIMPLEMENTED, RFC 4253 section 11.4. - MsgIdKnown() carries DoPacket()'s dispatch ids, build guards included - answer a refused id off the dispatch, so one cannot reach a handler if MsgIdKnown() falls behind - skip the disconnect once the session is over, RFC 4253 section 11.1 forbids sending after one - cover ids 53, 79, 200 and a channel open before user auth on one keyed-server helper - cover a client refusing both request names, with a reply asked for and without, and a server still succeeding Issue: wolfSSL#1047 (3), wolfSSL#1047 (7), F-10576, F-10581, F-12574
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1214
Scan targets checked: wolfssh-bugs, wolfssh-src
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Two of the RFC conformance items reported in #1047. Of the other five, items 1, 5 and 6 are already fixed on master (
90692e69,df635a3e,9e02350a), item 2 is PR #1148, and item 4 was considered and declined --direct-tcpipis deliberately not direction-checked, and the default no-fwdCbpath already answersOPEN_ADMINISTRATIVELY_PROHIBITED. These two were the remainder.Item 3 --
tcpip-forwardhandled without a role guard (RFC 4254 section 7.1).DoGlobalRequest()dispatchedtcpip-forwardandcancel-tcpip-forwardtoDoGlobalRequestFwd()whatever side it was on, so a client with afwdCbregistered answeredREQUEST_SUCCESSand acted on a forward the peer asked for. The callback was the only gate. A client now refuses both, answeringREQUEST_FAILUREwhen the peer asked for a reply and staying quiet when it did not. The answer is given before the request body is parsed, so no forward state is touched on the way to the refusal.Item 7 -- no disconnect for a message the state disallows (RFC 4252 section 6).
DoPacket()returnedWS_MSGID_NOT_ALLOWED_EandDoReceive()turned that into a localWS_FATAL_ERROR, so the socket simply went away and the peer was left to guess. It now sendsDISCONNECTwithPROTOCOL_ERRORwhereDoPacket()rejects the message id, skipped once the session is already over since RFC 4253 section 11.1 forbids sending after a disconnect. Every rejection there is a protocol error, not only the message-id range the RFC names, so the disconnect covers everywhereIsMessageAllowed()refuses a received message -- and the client side with it,DoPacket()being common to both.Worth being precise on that second one: the RFC's MUST, respond by disconnecting, was already satisfied by the teardown. What was missing is the "preferably with a proper disconnect message" half.
Regress coverage:
TestGlobalRequestFwdOnClientSendsFailure,TestGlobalRequestFwdCancelOnClientSendsFailure: both request names refused on a client, with afwdCbregistered throughout -- without the role check that callback is the only gate and would answer successTestGlobalRequestFwdOnClientNoReplyStaysQuiet: nothing goes back when the peer did not ask for a replyTestGlobalRequestFwdOnServerStillSucceeds: the same request a client refuses is still honoured on a server, so the role check costs the server nothingTestServerHighMsgIdBeforeAuthDisconnects: the case the RFC names, keyed but short of user authTestChannelOpenRejectedBeforeKexandTestServerServiceRequestRejectedDuringKeyingboth asserted silence on the reject path; they now assert the disconnectregress.test,unit.testandapi.testpass.