Fix DoGlobalRequestFwd() so that it rejects the port forwarding if no fwdCb is set. by yosuke-wolfssl · Pull Request #918 · wolfSSL/wolfssh

yosuke-wolfssl · 2026-04-15T02:51:26Z

Background
When WOLFSSH_FWD is compiled in, the server-side handler for tcpip-forward global requests sends SSH_MSG_REQUEST_SUCCESS (via SendGlobalRequestFwdSuccess at line 8500) before invoking the fwdCb policy callback (line 8504). This has two consequences: (1) If no fwdCb is registered (the default), all remote port forwarding requests are silently accepted because the success reply has already been sent. (2) Even if fwdCb IS registered and rejects the request, the client has already received the approval response.

Changes
This PR fixes it so that DoGlobalRequestFwd() rejects the forwarding if no fwdCb is set.
Also, the new regression tests are added.

Copilot

Pull request overview

This PR adjusts server-side handling of tcpip-forward / cancel-tcpip-forward global requests (when WOLFSSH_FWD is enabled) to avoid approving port forwarding before the forwarding-policy callback is consulted, and adds regression tests around the new behavior.

Changes:

Move the global-request success reply so it is only sent after the forwarding callback accepts the request.
Reject remote port forwarding requests when no forwarding callback (fwdCb) is registered.
Add regression tests covering success/failure replies for forward/cancel requests with and without fwdCb.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`src/internal.c`	Reorders `DoGlobalRequestFwd()` reply/decision flow and adds explicit rejection when `fwdCb` is not set.
`tests/regress.c`	Adds packet builder + assertions and new regress tests for global forwarding request replies.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

JacobBarthelmeh · 2026-04-15T21:33:45Z

@yosuke-wolfssl , please resolve the merge conflict

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-15T23:33:07Z

+static void AssertGlobalRequestReply(const ChannelOpenHarness* harness,
+        byte expectedMsgId)
+{
+    AssertTrue(harness->io.outSz > 0);
+    AssertIntEQ(ParseMsgId(harness->io.out, harness->io.outSz), expectedMsgId);


These new forwarding tests only assert the outbound message ID. To catch protocol-format regressions, also assert the reply payload shape: REQUEST_FAILURE should have payloadLen==1 (msgId only), tcpip-forward REQUEST_SUCCESS should include the extra uint32 port (payloadLen==5), and cancel-tcpip-forward REQUEST_SUCCESS should not include a port (payloadLen==1). You can derive payloadLen from packetLen/padLen in the SSH binary packet header.

Suggested change

static void AssertGlobalRequestReply(const ChannelOpenHarness* harness,

byte expectedMsgId)

{

AssertTrue(harness->io.outSz > 0);

AssertIntEQ(ParseMsgId(harness->io.out, harness->io.outSz), expectedMsgId);

static word32 ParsePayloadLen(const byte* packet, word32 packetSz)

{

word32 packetLen;

byte padLen;

AssertNotNull(packet);

AssertTrue(packetSz >= 6);

WMEMCPY(&packetLen, packet, sizeof(packetLen));

packetLen = ntohl(packetLen);

padLen = packet[4];

AssertTrue(packetLen >= (word32)padLen + 1);

AssertTrue(packetSz >= packetLen + 4);

return packetLen - padLen - 1;

}

static const byte* ParseGlobalRequestName(const byte* packet, word32 packetSz,

word32* nameSz)

{

word32 packetLen;

word32 payloadLen;

word32 strSz;

const byte* payload;

AssertNotNull(packet);

AssertNotNull(nameSz);

AssertTrue(packetSz >= 10);

WMEMCPY(&packetLen, packet, sizeof(packetLen));

packetLen = ntohl(packetLen);

AssertTrue(packetSz >= packetLen + 4);

payloadLen = ParsePayloadLen(packet, packetSz);

payload = packet + 5;

AssertTrue(payloadLen >= 1 + sizeof(word32));

AssertIntEQ(payload[0], MSGID_GLOBAL_REQUEST);

WMEMCPY(&strSz, payload + 1, sizeof(strSz));

strSz = ntohl(strSz);

AssertTrue(payloadLen >= 1 + sizeof(word32) + strSz);

*nameSz = strSz;

return payload + 1 + sizeof(word32);

}

static void AssertGlobalRequestReply(const ChannelOpenHarness* harness,

byte expectedMsgId)

{

byte msgId;

word32 payloadLen;

AssertTrue(harness->io.outSz > 0);

msgId = ParseMsgId(harness->io.out, harness->io.outSz);

AssertIntEQ(msgId, expectedMsgId);

payloadLen = ParsePayloadLen(harness->io.out, harness->io.outSz);

if (expectedMsgId == MSGID_REQUEST_FAILURE) {

AssertIntEQ(payloadLen, 1);

}

else if (expectedMsgId == MSGID_REQUEST_SUCCESS) {

const byte* reqName;

word32 reqNameSz;

reqName = ParseGlobalRequestName(harness->io.in, harness->io.inSz,

&reqNameSz);

if (reqNameSz == sizeof("tcpip-forward") - 1 &&

WMEMCMP(reqName, "tcpip-forward",

sizeof("tcpip-forward") - 1) == 0) {

AssertIntEQ(payloadLen, 5);

}

else if (reqNameSz == sizeof("cancel-tcpip-forward") - 1 &&

WMEMCMP(reqName, "cancel-tcpip-forward",

sizeof("cancel-tcpip-forward") - 1) == 0) {

AssertIntEQ(payloadLen, 1);

}

else {

Fail(("unexpected global request name"),

("%.*s", (int)reqNameSz, reqName));

}

}

Copilot · 2026-04-15T23:33:07Z

+    else if (ret == WS_UNIMPLEMENTED_E) {
+        /* No reply expected; silently reject without terminating connection. */
+        ret = WS_SUCCESS;
    }


When wantReply==0, a normal policy denial from fwdCb (e.g., returning WS_FWD_SETUP_E / WS_FWD_NOT_AVAILABLE) will currently be returned up to DoReceive() as a non-success status. This makes a no-reply global request look like a transport/API error even though the request was simply denied. Consider treating the forward callback’s non-success return codes as “request denied” and returning WS_SUCCESS when wantReply==0 (similar to the WS_UNIMPLEMENTED_E path), while still keeping real parse/IO errors fatal.

Copilot · 2026-04-15T23:33:07Z

+
+    if (wantReply) {
+        if (ret == WS_SUCCESS) {
+            ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort);


This success-reply path always uses SendGlobalRequestFwdSuccess(..., 1, bindPort), which encodes a uint32 port in SSH_MSG_REQUEST_SUCCESS. For cancel-tcpip-forward, the SSH spec’s REQUEST_SUCCESS carries no extra fields; sending a port for cancel responses can break strict clients. Consider using the generic SendRequestSuccess() (no port) for cancel requests, or extending the helper so the port is only included for tcpip-forward successes.

Suggested change

ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort);

if (isCancel) {

ret = SendRequestSuccess(ssh);

}

else {

ret = SendGlobalRequestFwdSuccess(ssh, 1, bindPort);

}

yosuke-wolfssl self-assigned this Apr 15, 2026

Copilot AI review requested due to automatic review settings April 15, 2026 02:51

Copilot started reviewing on behalf of yosuke-wolfssl April 15, 2026 02:51 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Comment thread src/internal.c Outdated

Comment thread src/internal.c Outdated

Comment thread tests/regress.c Outdated

Comment thread tests/regress.c Outdated

yosuke-wolfssl force-pushed the f_2867 branch from 51c53c8 to 0a795af Compare April 15, 2026 03:58

yosuke-wolfssl assigned wolfSSL-Bot and unassigned yosuke-wolfssl Apr 15, 2026

JacobBarthelmeh previously approved these changes Apr 15, 2026

View reviewed changes

JacobBarthelmeh assigned yosuke-wolfssl and unassigned wolfSSL-Bot Apr 15, 2026

Fix DoGlobalRequestFwd and Add the regress tests

ad38cc3

Copilot AI review requested due to automatic review settings April 15, 2026 23:26

yosuke-wolfssl dismissed JacobBarthelmeh’s stale review via ad38cc3 April 15, 2026 23:26

yosuke-wolfssl force-pushed the f_2867 branch from 0a795af to ad38cc3 Compare April 15, 2026 23:26

Copilot started reviewing on behalf of yosuke-wolfssl April 15, 2026 23:29 View session

Copilot AI reviewed Apr 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix DoGlobalRequestFwd() so that it rejects the port forwarding if no fwdCb is set.#918

Fix DoGlobalRequestFwd() so that it rejects the port forwarding if no fwdCb is set.#918
yosuke-wolfssl wants to merge 1 commit intowolfSSL:masterfrom
yosuke-wolfssl:f_2867

yosuke-wolfssl commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobBarthelmeh commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

Copilot AI Apr 15, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

-static void AssertGlobalRequestReply(const ChannelOpenHarness* harness,
-        byte expectedMsgId)
-{
-    AssertTrue(harness->io.outSz > 0);
-    AssertIntEQ(ParseMsgId(harness->io.out, harness->io.outSz), expectedMsgId);
+static word32 ParsePayloadLen(const byte* packet, word32 packetSz)
+{
+    word32 packetLen;
+    byte padLen;
+    AssertNotNull(packet);
+    AssertTrue(packetSz >= 6);
+    WMEMCPY(&packetLen, packet, sizeof(packetLen));
+    packetLen = ntohl(packetLen);
+    padLen = packet[4];
+    AssertTrue(packetLen >= (word32)padLen + 1);
+    AssertTrue(packetSz >= packetLen + 4);
+    return packetLen - padLen - 1;
+}
+static const byte* ParseGlobalRequestName(const byte* packet, word32 packetSz,
+        word32* nameSz)
+{
+    word32 packetLen;
+    word32 payloadLen;
+    word32 strSz;
+    const byte* payload;
+    AssertNotNull(packet);
+    AssertNotNull(nameSz);
+    AssertTrue(packetSz >= 10);
+    WMEMCPY(&packetLen, packet, sizeof(packetLen));
+    packetLen = ntohl(packetLen);
+    AssertTrue(packetSz >= packetLen + 4);
+    payloadLen = ParsePayloadLen(packet, packetSz);
+    payload = packet + 5;
+    AssertTrue(payloadLen >= 1 + sizeof(word32));
+    AssertIntEQ(payload[0], MSGID_GLOBAL_REQUEST);
+    WMEMCPY(&strSz, payload + 1, sizeof(strSz));
+    strSz = ntohl(strSz);
+    AssertTrue(payloadLen >= 1 + sizeof(word32) + strSz);
+    *nameSz = strSz;
+    return payload + 1 + sizeof(word32);
+}
+static void AssertGlobalRequestReply(const ChannelOpenHarness* harness,
+        byte expectedMsgId)
+{
+    byte msgId;
+    word32 payloadLen;
+    AssertTrue(harness->io.outSz > 0);
+    msgId = ParseMsgId(harness->io.out, harness->io.outSz);
+    AssertIntEQ(msgId, expectedMsgId);
+    payloadLen = ParsePayloadLen(harness->io.out, harness->io.outSz);
+    if (expectedMsgId == MSGID_REQUEST_FAILURE) {
+        AssertIntEQ(payloadLen, 1);
+    }
+    else if (expectedMsgId == MSGID_REQUEST_SUCCESS) {
+        const byte* reqName;
+        word32 reqNameSz;
+        reqName = ParseGlobalRequestName(harness->io.in, harness->io.inSz,
+                &reqNameSz);
+        if (reqNameSz == sizeof("tcpip-forward") - 1 &&
+                WMEMCMP(reqName, "tcpip-forward",
+                sizeof("tcpip-forward") - 1) == 0) {
+            AssertIntEQ(payloadLen, 5);
+        }
+        else if (reqNameSz == sizeof("cancel-tcpip-forward") - 1 &&
+                WMEMCMP(reqName, "cancel-tcpip-forward",
+                sizeof("cancel-tcpip-forward") - 1) == 0) {
+            AssertIntEQ(payloadLen, 1);
+        }
+        else {
+            Fail(("unexpected global request name"),
+                    ("%.*s", (int)reqNameSz, reqName));
+        }
+    }

Conversation

yosuke-wolfssl commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JacobBarthelmeh commented Apr 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants