Match forwarded-tcpip opens to registered forwards - #1148
Open
ejohnstown wants to merge 2 commits into
Open
Conversation
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and
a lite review from Copilot
August 6, 2026 23:15
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1148
Scan targets checked: wolfssh-bugs, wolfssh-src
Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)
This review was generated automatically by Fenrir. Findings are non-blocking.
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR tightens forwarded-tcpip channel-open handling to only accept opens that correspond to tcpip-forward registrations made via wolfSSH_FwdRemoteSetup() (per RFC 4254 §7.2), and fixes two wolfSSH_SendPacket() error-handling issues discovered during testing.
Changes:
- Track per-session remote-forward registrations and enforce matching for inbound
forwarded-tcpipopens (with wildcard + cancel/overlap semantics). - Introduce a send-order reply queue to pair REQUEST_SUCCESS/FAILURE with want-reply global requests/forward requests.
- Fix
wolfSSH_SendPacket()handling forWS_CBIO_ERR_ISRandWS_CBIO_ERR_GENERAL, and expand regression/API tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents updated remote-forward enforcement semantics and global-request reply-queue behavior. |
| wolfssh/internal.h | Adds forward tracking structs, reply queue state, and a flush counter to disambiguate post-send errors. |
| src/ssh.c | Integrates forward prepare/commit/discard around sends; enforces port-0 want-reply; queues want-reply global requests. |
| src/internal.c | Implements forward tracking + reply pairing; enforces forwarded-tcpip matching; fixes wolfSSH_SendPacket() ISR/general error behavior. |
| tests/regress.c | Adds extensive regression coverage for matching, cancellation semantics, reentrancy, send-order pairing, and send error paths. |
| tests/api.c | Adds API bad-args coverage for port-0 without want-reply. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
RFC 4254 7.2 says a forwarded-tcpip answers a forward the client asked for, so refuse an open naming anything else. - Register each wolfSSH_FwdRemoteSetup() per session. Enforcement starts at the first registration, so a client that frames tcpip-forward itself is unaffected. A wildcard bind matches on port alone, and port 0 now requires want-reply. - Repeat setups of one bind share a registration. A cancel stops matching as it goes out, but a want-reply cancel stays registered until the peer answers, since a refusal leaves the listener up. - Replies carry no request id, so a per-session queue pairs them in send order. A want-reply wolfSSH_global_request() takes a slot as well. - Registration is split around the send: allocate first, link only once the output buffer flushed. The highwater callback reports a rekey's errors as the send's, so the return code cannot decide it. - Sending runs application callbacks, so a request resolves its registration on commit rather than carrying it across the send. - Tests in the client-side channel-open harness cover matching, cancel, overlapping requests, send-order pairing, port 0, registration around the send, and reentrancy from a callback. Contracts for wolfSSH_FwdRemoteSetup(), wolfSSH_FwdRemoteCancel() and wolfSSH_global_request() are in wolfssh/ssh.h. Issue: ZD-22195
Both fixes are in wolfSSH_SendPacket(), so they cover every sender. - WS_CBIO_ERR_ISR fell through to WS_SOCKET_ERROR_E. A signal interrupted the send, so nothing went out and the session is unharmed; retry it, as ReceiveData() already does. - Callers that discard a packet on error, like the KEX and userauth sends, were throwing away framed output the peer never refused. - On WS_CBIO_ERR_GENERAL the output buffer is shrunk with the packet still counted in plainSz, so SendChannelData() flushes nothing and calls it a success. Clear it with the packet it described. - Tests pin the retry from a forwarding sender and a plain global request, and drive a channel send through a would-block and then a refused flush.
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.
RFC 4254 7.2 says a
forwarded-tcpipopen answers a forward the client asked for, but we accepted any of them. This tracks whatwolfSSH_FwdRemoteSetup()registered and refuses opens naming anything else. Enforcement starts at the first registration, so a client that framestcpip-forwarditself is unaffected. TwowolfSSH_SendPacket()bugs found while testing it are fixed here too.WS_CBIO_ERR_ISRfell through toWS_SOCKET_ERROR_E, discarding framed output the peer never refused; retry instead, asReceiveData()does. AndWS_CBIO_ERR_GENERALleft the discarded packet counted inplainSz, soSendChannelData()flushed nothing and called it a success.tests/regress.ccover matching, cancel, overlapping requests, send-order pairing, port 0, registration around the send, reentrancy from a callback, and both send paths.The three affected API contracts are documented in
wolfssh/ssh.h; no signatures changed.Issue: ZD-22195