Match and resolve remote forwards for the whole session - #1220
Conversation
The match only ran once wolfSSH_FwdRemoteSetup() had registered something, so a client that never called it had every forwarded-tcpip open go unchecked -- the case RFC 4254 7.2 is clearest about, since a session that asked for no forward has nothing an open could answer for. - Refuse the open whenever it names no registration, empty list included. wolfSSH_SetFwdRemoteMatch(WOLFSSH_FWD_MATCH_OFF) is the opt-out for an application that keeps its own bind list. - Drop fwdRemoteTracked, which nothing reads now. - wolfSSH_global_request() cannot carry the bind address and port RFC 4254 7.1 puts after the want-reply boolean, so no request that registers a forward can be framed outside wolfSSH_FwdRemoteSetup(). Issue: ZD-22195
There was a problem hiding this comment.
Pull request overview
This PR tightens and stabilizes client-side remote forwarding (“tcpip-forward” / inbound “forwarded-tcpip”) behavior across an entire session, including reentrant callback scenarios, by making matching always active (unless explicitly opted out) and by tracking requests that are “in flight” during their send window.
Changes:
- Make forwarded-tcpip matching apply from session start (empty forward list now refuses all forwarded-tcpip opens unless opted out via
WOLFSSH_FWD_MATCH_OFF). - Introduce an in-flight pending-request list (
fwdPendingHead) so reentrant sends can resolve/see the correct forward consistently across a send. - Add/adjust regression tests to cover no-forward behavior, opt-out behavior, and reentrancy during send.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| wolfssh/ssh.h | Updates API documentation to reflect session-wide matching and opt-out semantics. |
| wolfssh/internal.h | Adds fwdPendingHead tracking and extends pending-request bookkeeping for reentrant-safe resolution. |
| src/internal.c | Implements pending push/pop/void logic; updates forwarded-tcpip match logic and commit/discard flow. |
| tests/regress.c | Updates existing tests and adds new tests for “no forwards” refusal, opt-out acceptance, and send-window reentrancy cases. |
💡 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 #1220
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. Reported findings require changes before merge.
A request looked its registration up again at commit, after the send had run the application's send and highwater callbacks. A callback that reenters the library sends behind the request that ran it, so the lookup found what the callback did rather than what the request asked for: a cancel from a first setup's send found nothing to take, and that setup's commit then registered the forward the peer had just been told to drop. - Requests in flight hang off ssh->fwdPendingHead for the length of their send, so a reentrant request resolves to the forward the request it interrupted is registering, not to nothing. - FwdRemotePrepare() settles what the request names and holds it on the pending; FwdPendingCommit() uses that instead of a fresh lookup, which also drops the dedupe branch the double lookup needed. - FwdRemoteUnlink() voids the pending pointers along with the queued slots, so freeing a forward mid-send leaves no commit naming it, and FwdPendingDiscard() goes through it rather than freeing raw. - FwdRemoteMatch() reads the pending list under the rules the scan over registered forwards uses: a setup mid-send is on the wire and can feed channels; a cancel behind it or a refusal on its slot stops matching. - Drop pend->bindAddr and pend->bindPort, which nothing reads now. - Tests drive a cancel, a setup, an inbound forwarded-tcpip open and a mid-send REQUEST_FAILURE from the IO send callback, the window the highwater tests no longer reach. Issue: ZD-22195
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1220
Scan targets checked: wolfssh-src
Failed targets: wolfssh-bugs
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.
FwdPendingCommit() looks for a registration of the bind its entry names before linking it. An answer pumped in from the send can settle a port-pending forward onto that port, and the fold in FwdRemoteSettle() runs before the committing entry joins the list. - replies queued against the folded entry move to the one that stands at the bind - test drives that answer from the IO send callback; the bind takes two registrations without the fold, and one cancel leaves one matching
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1220
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.
FwdRemoteSettle() folds a port-0 forward the peer bound onto a registration already standing at that port. A request still in its send window resolved its forward before the send, so it names the survivor from here on, the same as the queued replies do. - Add FwdPendingRebind() and call it ahead of the unlink, so a cancel whose send pumped in the answer commits naming a forward rather than nothing. - Regression test drives the port-0 answer in from the cancel's send callback.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1220
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 only matched inbound "forwarded-tcpip" opens once it had registered a forward, and a request that had registered one resolved it again after its own send, by which point a reentrant callback could have changed the answer.
wolfSSH_SetFwdRemoteMatch(WOLFSSH_FWD_MATCH_OFF)is the opt-out for an application keeping its own bind list.Issue: ZD-22195