From 02ecaa015e42ff0020f133180b718d4eea5b05ca Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 10 Sep 2026 09:11:55 -0700 Subject: [PATCH 1/3] internal: bare success for a fixed-port forward 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: #1246 --- src/internal.c | 9 ++++--- tests/regress.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 5d1d61124..db7d86202 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12164,11 +12164,14 @@ static int DoGlobalRequestFwd(WOLFSSH* ssh, if (wantReply) { if (ret == WS_SUCCESS) { - if (isCancel) { - ret = SendRequestSuccess(ssh, 1); + /* RFC 4254 7.1 gives the success a trailing bound-port field only + * for a port-0 (dynamic) request. An explicit port, and a cancel, + * get a bare success with no response-specific data. */ + if (!isCancel && requestedPort == 0) { + ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort); } else { - ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort); + ret = SendRequestSuccess(ssh, 1); } } else { diff --git a/tests/regress.c b/tests/regress.c index eaa841542..cce2a02d9 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2151,6 +2151,37 @@ static const byte* ParseGlobalRequestName(const byte* packet, word32 packetSz, return payload + 1 + sizeof(word32); } +/* Bind port of a tcpip-forward global request. Past the request name and the + * want-reply byte come the bind address and then the port. */ +static word32 ParseGlobalRequestFwdBindPort(const byte* packet, + word32 packetSz) +{ + const byte* payload; + const byte* reqName; + word32 reqNameSz; + word32 payloadLen; + word32 idx; + word32 strSz; + word32 port; + + reqName = ParseGlobalRequestName(packet, packetSz, &reqNameSz); + payload = packet + 5; + payloadLen = ParsePayloadLen(packet, packetSz); + idx = (word32)(reqName - payload) + reqNameSz; + + AssertTrue(payloadLen >= idx + 1 + sizeof(word32)); + idx += 1; + + WMEMCPY(&strSz, payload + idx, sizeof(strSz)); + strSz = ntohl(strSz); + idx += (word32)sizeof(word32) + strSz; + + AssertTrue(payloadLen >= idx + sizeof(word32)); + WMEMCPY(&port, payload + idx, sizeof(port)); + + return ntohl(port); +} + static void AssertGlobalRequestReply(const ChannelOpenHarness* harness, byte expectedMsgId) { @@ -2175,7 +2206,14 @@ static void AssertGlobalRequestReply(const ChannelOpenHarness* harness, if (reqNameSz == sizeof("tcpip-forward") - 1 && WMEMCMP(reqName, "tcpip-forward", sizeof("tcpip-forward") - 1) == 0) { - AssertIntEQ(payloadLen, 5); + /* The bound port trails the success only for a port-0 request. */ + if (ParseGlobalRequestFwdBindPort(harness->io.in, + harness->io.inSz) == 0) { + AssertIntEQ(payloadLen, 5); + } + else { + AssertIntEQ(payloadLen, 1); + } } else if (reqNameSz == sizeof("cancel-tcpip-forward") - 1 && WMEMCMP(reqName, "cancel-tcpip-forward", @@ -5583,6 +5621,30 @@ static void TestGlobalRequestFwdPort0ReturnsAllocatedPort(void) FreeChannelOpenHarness(&harness); } +/* RFC 4254 7.1 defines the trailing bound-port field only for a port-0 + * request. An explicit port must be answered with a bare success. */ +static void TestGlobalRequestFwdExplicitPortReplyHasNoPort(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 8022, 0, 1, in, sizeof(in)); + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), + WS_SUCCESS); + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_REQUEST_SUCCESS); + AssertIntEQ(ParsePayloadLen(harness.io.out, harness.io.outSz), 1); + + FreeChannelOpenHarness(&harness); +} + static void TestGlobalRequestFwdPort0NoAllocSendsFailure(void) { ChannelOpenHarness harness; @@ -14955,6 +15017,7 @@ int main(int argc, char** argv) TestGlobalRequestFwdNoCbNoReplyKeepsConnection(); TestGlobalRequestFwdWithCbSendsSuccess(); TestGlobalRequestFwdPort0ReturnsAllocatedPort(); + TestGlobalRequestFwdExplicitPortReplyHasNoPort(); TestGlobalRequestFwdPort0NoAllocSendsFailure(); TestGlobalRequestFwdRemoteSetupErrorSendsFailure(); TestGlobalRequestFwdPort0NoAllocNoReplyKeepsConnection(); From 2618cd1e44ca5ea70f1828887e4df9662f9e4022 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 15 Sep 2026 19:48:19 -0700 Subject: [PATCH 2/3] tests: pin the fixed-port forward reply terms 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: #1246 --- tests/regress.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 72 insertions(+), 2 deletions(-) diff --git a/tests/regress.c b/tests/regress.c index cce2a02d9..de8d98569 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2174,9 +2174,15 @@ static word32 ParseGlobalRequestFwdBindPort(const byte* packet, WMEMCPY(&strSz, payload + idx, sizeof(strSz)); strSz = ntohl(strSz); - idx += (word32)sizeof(word32) + strSz; + idx += (word32)sizeof(word32); - AssertTrue(payloadLen >= idx + sizeof(word32)); + /* Bound each step with a subtraction. idx + strSz is word32 arithmetic on + * a length out of the packet and wraps, which would leave the guard + * passing against a wrapped index. */ + AssertTrue(payloadLen - idx >= strSz); + idx += strSz; + + AssertTrue(payloadLen - idx >= sizeof(word32)); WMEMCPY(&port, payload + idx, sizeof(port)); return ntohl(port); @@ -2356,6 +2362,21 @@ static int AllocatePortFwdCb(WS_FwdCbAction action, void* ctx, return WS_SUCCESS; } +/* Reports an allocated port for every remote setup, including the explicit + * port request where the value has to be ignored. */ +static int AlwaysAllocPortFwdCb(WS_FwdCbAction action, void* ctx, + const char* host, word32 port) +{ + (void)ctx; + (void)host; + (void)port; + + if (action == WOLFSSH_FWD_REMOTE_SETUP) + return REGRESS_FWD_ALLOC_PORT; + + return WS_SUCCESS; +} + /* Accepts the remote setup but never reports an allocated port. Records * whether the server asks it to clean the setup back up. */ static int NoPortFwdCb(WS_FwdCbAction action, void* ctx, @@ -5645,6 +5666,30 @@ static void TestGlobalRequestFwdExplicitPortReplyHasNoPort(void) FreeChannelOpenHarness(&harness); } +/* For an explicit port a callback return at or above WS_FWD_PORT_CHECK is + * ignored and the requested port stands, so the reply still carries none. */ +static void TestGlobalRequestFwdExplicitPortIgnoresAllocCb(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 8022, 0, 1, in, sizeof(in)); + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AlwaysAllocPortFwdCb, NULL), + WS_SUCCESS); + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_REQUEST_SUCCESS); + AssertIntEQ(ParsePayloadLen(harness.io.out, harness.io.outSz), 1); + + FreeChannelOpenHarness(&harness); +} + static void TestGlobalRequestFwdPort0NoAllocSendsFailure(void) { ChannelOpenHarness harness; @@ -5760,6 +5805,29 @@ static void TestGlobalRequestFwdCancelWithCbSendsSuccess(void) FreeChannelOpenHarness(&harness); } +/* A cancel reports no allocated port whatever its bind port, so a port-0 + * cancel is answered bare as well. */ +static void TestGlobalRequestFwdCancelPort0ReplyHasNoPort(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 0, 1, 1, in, sizeof(in)); + InitChannelOpenHarness(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), WS_SUCCESS); + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(ParseMsgId(harness.io.out, harness.io.outSz), + MSGID_REQUEST_SUCCESS); + AssertIntEQ(ParsePayloadLen(harness.io.out, harness.io.outSz), 1); + + FreeChannelOpenHarness(&harness); +} + /* Verify DoRequestSuccess correctly consumes a uint32 port payload (RFC 4254 * sec 4) without treating it as a length prefix, which would overrun the * buffer and produce WS_BUFFER_E. */ @@ -15018,11 +15086,13 @@ int main(int argc, char** argv) TestGlobalRequestFwdWithCbSendsSuccess(); TestGlobalRequestFwdPort0ReturnsAllocatedPort(); TestGlobalRequestFwdExplicitPortReplyHasNoPort(); + TestGlobalRequestFwdExplicitPortIgnoresAllocCb(); TestGlobalRequestFwdPort0NoAllocSendsFailure(); TestGlobalRequestFwdRemoteSetupErrorSendsFailure(); TestGlobalRequestFwdPort0NoAllocNoReplyKeepsConnection(); TestGlobalRequestFwdCancelNoCbSendsFailure(); TestGlobalRequestFwdCancelWithCbSendsSuccess(); + TestGlobalRequestFwdCancelPort0ReplyHasNoPort(); TestRequestSuccessWithPortParsesCorrectly(); #endif #ifdef WOLFSSH_AGENT From 0b5b9e6122e5e10ae671c1af2bc2cb27b7e66234 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 15 Sep 2026 20:08:24 -0700 Subject: [PATCH 3/3] tests: bound the first forward parse guard too 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: #1246 --- tests/regress.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/regress.c b/tests/regress.c index de8d98569..a08dd4d5d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2169,16 +2169,17 @@ static word32 ParseGlobalRequestFwdBindPort(const byte* packet, payloadLen = ParsePayloadLen(packet, packetSz); idx = (word32)(reqName - payload) + reqNameSz; - AssertTrue(payloadLen >= idx + 1 + sizeof(word32)); + /* ParseGlobalRequestName() bounded the name, so idx is within payloadLen. + * Bound every step from here with a subtraction: adding a length out of + * the packet to idx wraps word32 and leaves the guard passing against a + * wrapped index. */ + AssertTrue(payloadLen - idx >= 1 + sizeof(word32)); idx += 1; WMEMCPY(&strSz, payload + idx, sizeof(strSz)); strSz = ntohl(strSz); idx += (word32)sizeof(word32); - /* Bound each step with a subtraction. idx + strSz is word32 arithmetic on - * a length out of the packet and wraps, which would leave the guard - * passing against a wrapped index. */ AssertTrue(payloadLen - idx >= strSz); idx += strSz;