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..a08dd4d5d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2151,6 +2151,44 @@ 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; + + /* 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); + + AssertTrue(payloadLen - idx >= strSz); + idx += 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 +2213,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", @@ -2318,6 +2363,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, @@ -5583,6 +5643,54 @@ 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); +} + +/* 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; @@ -5698,6 +5806,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. */ @@ -14955,11 +15086,14 @@ int main(int argc, char** argv) TestGlobalRequestFwdNoCbNoReplyKeepsConnection(); TestGlobalRequestFwdWithCbSendsSuccess(); TestGlobalRequestFwdPort0ReturnsAllocatedPort(); + TestGlobalRequestFwdExplicitPortReplyHasNoPort(); + TestGlobalRequestFwdExplicitPortIgnoresAllocCb(); TestGlobalRequestFwdPort0NoAllocSendsFailure(); TestGlobalRequestFwdRemoteSetupErrorSendsFailure(); TestGlobalRequestFwdPort0NoAllocNoReplyKeepsConnection(); TestGlobalRequestFwdCancelNoCbSendsFailure(); TestGlobalRequestFwdCancelWithCbSendsSuccess(); + TestGlobalRequestFwdCancelPort0ReplyHasNoPort(); TestRequestSuccessWithPortParsesCorrectly(); #endif #ifdef WOLFSSH_AGENT