Send a bare success for a fixed-port tcpip-forward - #1254
Merged
Merged
Conversation
RFC 4254 7.1 gives a tcpip-forward success a trailing bound-port field only for a port-0 (dynamic) request. DoGlobalRequestFwd() now sends that field only when the peer asked the server to allocate a port, and answers an explicit port with a bare SSH_MSG_REQUEST_SUCCESS, as OpenSSH and libssh do. - Key the reply builder off requestedPort, which the port-0 compliance check already tracks. - Check the reply payload length against the requested port in the regress global-request helper, which had baked in the trailing field. - Add ParseGlobalRequestFwdBindPort() to recover that port from the request the harness fed in. - Cover the explicit-port reply. Issue: wolfSSL#1246
Contributor
There was a problem hiding this comment.
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.
Pull request overview
Updates tcpip-forward global-request success replies to match RFC 4254 §7.1 by omitting the trailing bound-port field for explicit (non-zero) ports, while retaining it for port-0 (dynamic) requests.
Changes:
- Adjusted forwarding global-request reply construction to send a bare
SSH_MSG_REQUEST_SUCCESSfor explicit ports (and cancels). - Updated regress helper logic to validate reply payload length based on the requested port.
- Added a regression test covering the explicit-port success reply.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/regress.c | Adds parsing helper for requested bind port, updates success-payload assertions, and adds a test ensuring explicit-port replies contain no trailing port. |
| src/internal.c | Changes tcpip-forward success reply to include a bound-port only when the requested port is 0; otherwise sends a bare success. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The reply builder is chosen by !isCancel and a zero requested port, and the suite exercised neither term on its own. A port-0 cancel now covers the first, and an explicit port answered by a port-reporting callback covers the documented case where that return is ignored. - drop the !isCancel term and the port-0 cancel test fails, finding a five byte payload where the bare success is one - bound the bind-address length with a subtraction in ParseGlobalRequestFwdBindPort, where advancing by it wrapped word32 - add AlwaysAllocPortFwdCb, reporting a port for any remote setup Issue: wolfSSL#1246
The helper states that every bound is a subtraction because adding a packet-derived length to the index wraps, and its own first guard was still an addition. It could not wrap, since ParseGlobalRequestName() already bounded the name, but the mixed forms invite the weaker one into the next guard added here. Issue: wolfSSL#1246
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1254
Scan targets checked: wolfssh-src, wolfssh-bugs
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
philljj
self-requested a review
September 17, 2026 19:34
philljj
approved these changes
Sep 17, 2026
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 section 7.1 gives a
tcpip-forwardsuccess a trailing bound-port field only for a port-0 (dynamic) request. An explicit port now gets a bareSSH_MSG_REQUEST_SUCCESS, as OpenSSH and libssh send.Fixes #1246. Found by the tlspuffin team (Tom Gouville, Lucca Hirschi, Steve Kremer, Natael Baffou, Olivier Demengeon (Loria, Inria)) via sshpuffin differential fuzzing.