Guard remote forwards on a client, disconnect on a disallowed message - #1214
Guard remote forwards on a client, disconnect on a disallowed message#1214ejohnstown wants to merge 2 commits into
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.
RFC 4254 section 7.1 says tcpip-forward and cancel-tcpip-forward are normally only sent by the client, and that client implementations should reject them. DoGlobalRequest() dispatched both to DoGlobalRequestFwd() whatever side it was on, so a client with a forward callback registered answered REQUEST_SUCCESS and acted on a forward the peer asked for. The callback was the only gate. - refuse both requests on a client, answering REQUEST_FAILURE when the peer asked for a reply and staying quiet when it did not - answer before the request body is parsed, so no forward state is touched on the way to the refusal - add regress coverage for both request names, the no-reply case, and a server still succeeding Issue: wolfSSL#1047 (3)
RFC 4252 section 6 has the server disconnect on a message id of 80 or higher arriving before user authentication finishes, preferably with a disconnect message so the other end can tell what happened. DoPacket() returned WS_MSGID_NOT_ALLOWED_E and DoReceive() turned that into a local WS_FATAL_ERROR, so the socket simply went away and the peer was left to guess. None of the existing SendDisconnect() call sites covered this path. Every rejection here is a protocol error, not only the message-id range the RFC names, so the disconnect is sent wherever IsMessageAllowed() refuses a received message. - send DISCONNECT with PROTOCOL_ERROR where DoPacket() rejects the message id - skip it once the session is already over, since RFC 4253 section 11.1 forbids sending after a disconnect - cover the server case the RFC names, keyed but short of user auth - TestChannelOpenRejectedBeforeKex() and TestServerServiceRequestRejectedDuringKeying() asserted silence on the reject path; both now assert the disconnect Issue: wolfSSL#1047 (7)
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.
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.