From 8fb87f89f8e9c26a88861ec65626815baedc5d6d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 31 Aug 2026 10:56:02 -0700 Subject: [PATCH] Guard remote forwards and disallowed msg ids A client answers tcpip-forward and cancel-tcpip-forward with a failure, RFC 4254 section 7.1, before the request body is parsed. DoPacket() disconnects on a refused id this build implements and on any refused id of 80 or higher, the range RFC 4252 section 6 names; a refused id below 80 is answered UNIMPLEMENTED, RFC 4253 section 11.4. - MsgIdKnown() carries DoPacket()'s dispatch ids, build guards included - answer a refused id off the dispatch, so one cannot reach a handler if MsgIdKnown() falls behind - skip the disconnect once the session is over, RFC 4253 section 11.1 forbids sending after one - cover ids 53, 79, 200 and a channel open before user auth on one keyed-server helper - cover a client refusing both request names, with a reply asked for and without, and a server still succeeding Issue: #1047 (3), #1047 (7), F-10576, F-10581, F-12574 --- src/internal.c | 85 +++++++++++++++++- tests/regress.c | 223 ++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 302 insertions(+), 6 deletions(-) diff --git a/src/internal.c b/src/internal.c index be4ae3c25..4c61cd6a9 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11617,6 +11617,22 @@ static int DoGlobalRequest(WOLFSSH* ssh, } if (ret == WS_SUCCESS) { +#ifdef WOLFSSH_FWD + /* RFC 4254 section 7.1: a remote forward is the client's to ask for, + * so a client that receives the request answers a failure rather than + * registering a forward on the peer's say-so. Answered here and not + * in DoGlobalRequestFwd(), so the request body is never parsed and no + * forward state is touched. */ + if ((globReqId == ID_GLOBREQ_TCPIP_FWD + || globReqId == ID_GLOBREQ_TCPIP_FWD_CANCEL) + && ssh->ctx->side == WOLFSSH_ENDPOINT_CLIENT) { + WLOG(WS_LOG_WARN, "DGR: rejecting %s received by a client", name); + if (wantReply) { + ret = SendRequestSuccess(ssh, 0); + } + } + else +#endif switch (globReqId) { #ifdef WOLFSSH_FWD case ID_GLOBREQ_TCPIP_FWD: @@ -12928,6 +12944,57 @@ static int DoChannelExtendedData(WOLFSSH* ssh, } +/* Has DoPacket()'s dispatch a case for this id? Keep in step with it, guards + * included: a message compiled out is one this build does not recognize. */ +INLINE static int MsgIdKnown(byte msg) +{ + switch (msg) { + case MSGID_DISCONNECT: + case MSGID_IGNORE: + case MSGID_UNIMPLEMENTED: + case MSGID_REQUEST_SUCCESS: + case MSGID_REQUEST_FAILURE: + case MSGID_DEBUG: + case MSGID_EXT_INFO: + case MSGID_KEXINIT: + case MSGID_NEWKEYS: + case MSGID_KEXDH_INIT: + case MSGID_KEXDH_REPLY: +#ifndef WOLFSSH_NO_DH_GEX_SHA256 + case MSGID_KEXDH_GEX_REQUEST: +#endif + case MSGID_KEXDH_GEX_INIT: + case MSGID_KEXDH_GEX_REPLY: + case MSGID_SERVICE_REQUEST: + case MSGID_SERVICE_ACCEPT: + case MSGID_USERAUTH_REQUEST: +#ifdef WOLFSSH_KEYBOARD_INTERACTIVE + case MSGID_USERAUTH_INFO_RESPONSE: + case MSGID_USERAUTH_INFO_REQUEST: +#endif + case MSGID_USERAUTH_FAILURE: + case MSGID_USERAUTH_SUCCESS: + case MSGID_USERAUTH_BANNER: + case MSGID_GLOBAL_REQUEST: + case MSGID_CHANNEL_OPEN: + case MSGID_CHANNEL_OPEN_CONF: + case MSGID_CHANNEL_OPEN_FAIL: + case MSGID_CHANNEL_WINDOW_ADJUST: + case MSGID_CHANNEL_DATA: + case MSGID_CHANNEL_EXTENDED_DATA: + case MSGID_CHANNEL_EOF: + case MSGID_CHANNEL_CLOSE: + case MSGID_CHANNEL_REQUEST: + case MSGID_CHANNEL_SUCCESS: + case MSGID_CHANNEL_FAILURE: + return 1; + + default: + return 0; + } +} + + static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) { byte* buf = (byte*)ssh->inputBuffer.buffer; @@ -12939,6 +13006,7 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) byte padSz; byte msg; word32 payloadIdx = 0; + int msgAllowed; int ret = WS_SUCCESS; WLOG(WS_LOG_DEBUG, "DoPacket sequence number: %d", ssh->peerSeq); @@ -12973,7 +13041,15 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) return WS_OVERFLOW_E; } - if (!IsMessageAllowed(ssh, msg, WS_MSG_RECV)) { + msgAllowed = IsMessageAllowed(ssh, msg, WS_MSG_RECV); + + if (!msgAllowed && (MsgIdKnown(msg) || MSGIDLIMIT_POST_USERAUTH(msg))) { + /* RFC 4252 section 6: disconnect on a known id at the wrong time, + * and on any id of 80 or higher, which IsMessageAllowed() refuses + * only before auth. Silent once over, RFC 4253 section 11.1. */ + if (!ssh->disconnected) { + (void)SendDisconnect(ssh, WOLFSSH_DISCONNECT_PROTOCOL_ERROR); + } return WS_MSGID_NOT_ALLOWED_E; } @@ -12987,6 +13063,12 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) WLOG(WS_LOG_DEBUG, "Ignoring message ID %u after a disconnect", (word32)msg); } + else if (!msgAllowed) { + /* Refused, unimplemented, below 80. Answered off the dispatch so a + * refused id cannot reach a handler if MsgIdKnown() drifts. */ + WLOG(WS_LOG_DEBUG, "Unimplemented message ID (%d)", msg); + ret = SendUnimplemented(ssh); + } else switch (msg) { case MSGID_DISCONNECT: @@ -13190,6 +13272,7 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) break; default: + /* RFC 4253 section 11.4, reached whatever the state allows. */ WLOG(WS_LOG_DEBUG, "Unimplemented message ID (%d)", msg); #ifdef SHOW_UNIMPLEMENTED DumpOctetString(buf + idx, payloadSz); diff --git a/tests/regress.c b/tests/regress.c index 079d581d2..37f5e7b99 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2297,8 +2297,9 @@ static void TestClientServiceAcceptBlockedDuringKeying(WOLFSSH* ssh) /* Drive the whole receive path with a CHANNEL_OPEN sent from a pre-auth - * connectState: no channel created, no reply emitted. The connectState - * gate does the rejecting; isKeying below is scene-setting only. */ + * connectState: no channel created, and the only thing sent back is the + * disconnect RFC 4252 section 6 asks for. The connectState gate does the + * rejecting; isKeying below is scene-setting only. */ static void TestChannelOpenRejectedBeforeKex(byte connectState) { WOLFSSH_CTX* ctx; @@ -2330,7 +2331,11 @@ static void TestChannelOpenRejectedBeforeKex(byte connectState) AssertIntEQ(ssh->error, WS_MSGID_NOT_ALLOWED_E); AssertNull(ssh->channelList); AssertIntEQ(ssh->channelListSz, 0); - AssertIntEQ(io.outSz, 0); + /* Not silence: the peer is told why the session ended, and the message + * is a disconnect rather than anything answering the channel open. */ + AssertTrue(io.outSz > 0); + AssertIntEQ(ParseMsgId(io.out, io.outSz), MSGID_DISCONNECT); + AssertTrue(ssh->disconnected); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); @@ -2403,6 +2408,103 @@ static void TestServerUserauthBlockedBeforeKeyed(WOLFSSH* ssh) } +/* One packet fed to a keyed but unauthenticated server. The cases below + * differ only in the packet and the answer it draws. */ +static void RunServerMsgIdAtKeyed(const byte* pkt, word32 pktSz, + int expectRet, int expectErr, byte expectReplyMsgId, + int expectDisconnected) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[256]; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + + MemIoInit(&io, (byte*)pkt, pktSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Past the key exchange, short of user auth being answered. */ + ssh->acceptState = ACCEPT_KEYED; + + AssertIntEQ(wolfSSH_TestDoReceive(ssh), expectRet); + AssertIntEQ(ssh->error, expectErr); + /* Nothing the filter refuses may leave a channel behind. */ + AssertNull(ssh->channelList); + AssertIntEQ(ssh->channelListSz, 0); + AssertTrue(io.outSz > 0); + AssertIntEQ(ParseMsgId(io.out, io.outSz), expectReplyMsgId); + AssertIntEQ(ssh->disconnected != 0, expectDisconnected); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* The case RFC 4252 section 6 names: an id of 80 or higher before user auth. + * Keyed, so the post-userauth limit rejects it, not the pre-keyed range. */ +static void TestServerHighMsgIdBeforeAuthDisconnects(void) +{ + byte pkt[256]; + word32 pktSz; + + pktSz = BuildChannelOpenPacket("session", 0, 131072, 16384, NULL, 0, + pkt, sizeof(pkt)); + + RunServerMsgIdAtKeyed(pkt, pktSz, WS_FATAL_ERROR, WS_MSGID_NOT_ALLOWED_E, + MSGID_DISCONNECT, 1); +} + + +/* Id 79 is refused too, but it is unimplemented and below 80, so RFC 4253 + * section 11.4 rules: UNIMPLEMENTED, and the session lives. */ +static void TestServerUnknownMsgIdBeforeAuthUnimplemented(void) +{ + byte pkt[256]; + word32 pktSz; + + pktSz = WrapPacket(79, NULL, 0, pkt, sizeof(pkt)); + + RunServerMsgIdAtKeyed(pkt, pktSz, WS_SUCCESS, WS_SUCCESS, + MSGID_UNIMPLEMENTED, 0); +} + + +/* Below 80 the dispatch decides: 53 has a case, so it disconnects where + * 79 does not. */ +static void TestServerKnownAuthMsgIdBeforeAuthDisconnects(void) +{ + byte pkt[256]; + word32 pktSz; + + pktSz = WrapPacket(MSGID_USERAUTH_BANNER, NULL, 0, pkt, sizeof(pkt)); + + RunServerMsgIdAtKeyed(pkt, pktSz, WS_FATAL_ERROR, WS_MSGID_NOT_ALLOWED_E, + MSGID_DISCONNECT, 1); +} + + +/* At 80 and above the range decides instead: id 200 disconnects though + * nothing dispatches it. */ +static void TestServerUnknownHighMsgIdBeforeAuthDisconnects(void) +{ + byte pkt[256]; + word32 pktSz; + + pktSz = WrapPacket(200, NULL, 0, pkt, sizeof(pkt)); + + RunServerMsgIdAtKeyed(pkt, pktSz, WS_FATAL_ERROR, WS_MSGID_NOT_ALLOWED_E, + MSGID_DISCONNECT, 1); +} + + /* Reject the user auth messages that only the server sends, while still * accepting the keyboard-interactive info response that it receives. */ static void TestServerOnlyUserauthMsgsBlocked(WOLFSSH* ssh) @@ -2492,8 +2594,11 @@ static void TestServerServiceRequestRejectedDuringKeying(void) /* Not dispatched, so acceptState did not advance. */ AssertIntEQ(ssh->clientState, CLIENT_BEGIN); AssertIntEQ(ssh->acceptState, ACCEPT_KEYED); - /* Nothing emitted in reply. */ - AssertIntEQ(io.outSz, 0); + /* Not silence: nothing answers the service request, but the peer is told + * the session ended on a protocol error. */ + AssertTrue(io.outSz > 0); + AssertIntEQ(ParseMsgId(io.out, io.outSz), MSGID_DISCONNECT); + AssertTrue(ssh->disconnected); /* Allowed when only this side has started a rekey. */ ssh->error = 0; @@ -3077,6 +3182,104 @@ static void TestGlobalRequestFwdNoCbSendsFailure(void) FreeChannelOpenHarness(&harness); } +#ifndef NO_WOLFSSH_CLIENT +/* A client is the side that asks for a remote forward, so a tcpip-forward it + * receives is answered with a failure and never registered. RFC 4254 section + * 7.1. A fwdCb is registered throughout: without the role check that callback + * is the only gate, and it would answer success. */ +static void TestGlobalRequestFwdOnClientSendsFailure(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 2222, 0, 1, in, sizeof(in)); + InitChannelOpenHarnessClient(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), + WS_SUCCESS); + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertGlobalRequestReply(&harness, MSGID_REQUEST_FAILURE); + + FreeChannelOpenHarness(&harness); +} + +/* cancel-tcpip-forward travels the same direction, so a client refuses it on + * the same grounds. */ +static void TestGlobalRequestFwdCancelOnClientSendsFailure(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 2222, 1, 1, in, sizeof(in)); + InitChannelOpenHarnessClient(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, AcceptFwdCb, NULL), + WS_SUCCESS); + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertGlobalRequestReply(&harness, MSGID_REQUEST_FAILURE); + + FreeChannelOpenHarness(&harness); +} + +/* The refusal is silent when the peer did not ask for a reply: nothing goes + * back, and the session carries on. Silence alone would also be the answer + * without the role check, since a request the callback accepts and that asks + * for no reply sends nothing either, so the callback is the discriminator + * here: the request must be turned away before it reaches one. */ +static void TestGlobalRequestFwdOnClientNoReplyStaysQuiet(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 2222, 0, 0, in, sizeof(in)); + InitChannelOpenHarnessClient(&harness, in, inSz); + AssertIntEQ(wolfSSH_CTX_SetFwdCb(harness.ctx, CountingFwdCb, NULL), + WS_SUCCESS); + fwdCbCallCount = 0; + + ret = DoReceive(harness.ssh); + + AssertIntEQ(ret, WS_SUCCESS); + AssertIntEQ(harness.io.outSz, 0); + AssertIntEQ(fwdCbCallCount, 0); + + FreeChannelOpenHarness(&harness); +} + +#endif /* !NO_WOLFSSH_CLIENT */ + +/* The role check must not cost the server anything: the same request that a + * client refuses is still honoured here. */ +static void TestGlobalRequestFwdOnServerStillSucceeds(void) +{ + ChannelOpenHarness harness; + byte in[256]; + word32 inSz; + int ret; + + inSz = BuildGlobalRequestFwdPacket("0.0.0.0", 2222, 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); + AssertGlobalRequestReply(&harness, MSGID_REQUEST_SUCCESS); + + FreeChannelOpenHarness(&harness); +} + static void TestGlobalRequestFwdNoCbNoReplyKeepsConnection(void) { ChannelOpenHarness harness; @@ -10816,6 +11019,10 @@ int main(int argc, char** argv) TestServerChannelBlockedBeforeAuth(serverSsh); TestServerChannelAllowedAfterAuth(serverSsh); TestServerUserauthBlockedBeforeKeyed(serverSsh); + TestServerHighMsgIdBeforeAuthDisconnects(); + TestServerUnknownMsgIdBeforeAuthUnimplemented(); + TestServerKnownAuthMsgIdBeforeAuthDisconnects(); + TestServerUnknownHighMsgIdBeforeAuthDisconnects(); TestServerOnlyUserauthMsgsBlocked(serverSsh); TestServerServiceRequestStateGated(serverSsh); TestServerServiceRequestRejectedDuringKeying(); @@ -10836,6 +11043,12 @@ int main(int argc, char** argv) TestDirectTcpipFwdCbRejectsChannelId(); TestForwardedTcpipOnServerSendsOpenFail(); TestGlobalRequestFwdNoCbSendsFailure(); +#ifndef NO_WOLFSSH_CLIENT + TestGlobalRequestFwdOnClientSendsFailure(); + TestGlobalRequestFwdCancelOnClientSendsFailure(); + TestGlobalRequestFwdOnClientNoReplyStaysQuiet(); +#endif + TestGlobalRequestFwdOnServerStillSucceeds(); TestGlobalRequestFwdNoCbNoReplyKeepsConnection(); TestGlobalRequestFwdWithCbSendsSuccess(); TestGlobalRequestFwdPort0ReturnsAllocatedPort();