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

Author: ejohnstown

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);

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();