Match forwarded-tcpip opens to registered forwards - #1148
Conversation
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.
| TestServerOnlyUserauthMsgsBlocked(serverSsh); | ||
| TestServerServiceRequestStateGated(serverSsh); | ||
| #endif | ||
| TestFailedSendClearsPendingPlaintext(); |
There was a problem hiding this comment.
🔴 [High] TestFailedSendClearsPendingPlaintext called outside the #ifdef WOLFSSH_FWD block that defines it · Logic errors
The new TestFailedSendClearsPendingPlaintext() is defined at regress.c:3664, inside the #ifdef WOLFSSH_FWD block spanning lines 2179-3920, but invoked at line 7255, before the #ifdef WOLFSSH_FWD guard that opens at line 7264. WOLFSSH_FWD defaults to off (configure.ac --enable-fwd), so make check fails to build tests/regress.test in a default configuration. The test exercises plainSz handling in SendChannelData() and needs no forwarding support.
Fix: Move the function definition out of the #ifdef WOLFSSH_FWD block so it compiles in every configuration.
| { | ||
| WOLFSSH_FWD_REPLY* reply; | ||
|
|
||
| reply = (WOLFSSH_FWD_REPLY*)WMALLOC(sizeof(WOLFSSH_FWD_REPLY), |
There was a problem hiding this comment.
🔵 [Low] Pending-reply queue has no bound and is only drained by peer replies · Resource leaks
Every want-reply wolfSSH_global_request() and wolfSSH_FwdRemoteSetup()/Cancel() appends a WOLFSSH_FWD_REPLY that is freed only by an inbound REQUEST_SUCCESS/FAILURE or at session teardown. A peer that violates RFC 4254 7.1 by never replying grows the queue without limit and makes each FwdRemoteMatch() scan longer.
Fix: Cap the number of outstanding reply slots per session and fail the request (or disconnect) once the cap is exceeded.
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.
| /* The optional sent out-param reports whether the request reached the peer, | ||
| * which the return does not answer: the highwater callback runs after the last | ||
| * byte goes out, so its failure surfaces as this call's. */ | ||
| WOLFSSH_LOCAL int SendGlobalRequest(WOLFSSH * ssh, | ||
| const unsigned char * data, word32 dataSz, int reply); | ||
| const unsigned char * data, word32 dataSz, int reply, int* sent); |
| WLOG(WS_LOG_WARN, "Remote forward reply named a port already " | ||
| "registered"); | ||
| FwdRemoteUnlink(ssh, ssh->ctx->heap, dup); |
| static int FwdRemoteMatch(WOLFSSH* ssh, const char* addr, word32 port) | ||
| { | ||
| WOLFSSH_FWD_REMOTE* cur; | ||
|
|
||
| if (ssh == NULL || addr == NULL) | ||
| return 0; | ||
|
|
||
| for (cur = ssh->fwdRemoteList; cur != NULL; cur = cur->next) { | ||
| WOLFSSH_FWD_REPLY* newest; | ||
|
|
||
| /* No port to match on until the peer's reply names the one it | ||
| * bound. */ | ||
| if (cur->portPending || cur->bindPort != port) | ||
| continue; | ||
|
|
||
| /* The newest request governs: a cancel stops matching as it goes out, | ||
| * so revoking never waits on the peer, and the peer refusing it puts | ||
| * the forward back. */ | ||
| newest = FwdReplyNewest(ssh, cur); | ||
| if (newest != NULL && newest->isCancel) | ||
| continue; | ||
|
|
||
| /* A forward stands on the peer having bound it, or on a request still | ||
| * owed an answer. With neither, nothing speaks for it. */ | ||
| if (!cur->confirmed && newest == NULL) | ||
| continue; | ||
|
|
||
| if (FwdRemoteAddrIsWild(cur->bindAddr) || | ||
| WSTRCMP(cur->bindAddr, addr) == 0) | ||
| return 1; | ||
| } |
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.
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