From 2f531f487ed832ab720bdca3e0dd1dd44a07467a Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 11:20:11 -0700 Subject: [PATCH] fix: reject service messages while keying RFC 4253 section 7.1 bars SERVICE_REQUEST and SERVICE_ACCEPT between a KEXINIT and the matching NEWKEYS. The admission check now consults ssh->isKeying, which acceptState and connectState do not track. It tests WOLFSSH_PEER_IS_KEYING, as the KEXINIT gate below it does: a peer that has not seen our KEXINIT yet may still legally send. - gate MSGID_SERVICE_REQUEST on isKeying in IsMessageAllowedServer() - gate MSGID_SERVICE_ACCEPT on isKeying in IsMessageAllowedClient() - record the refusals as WS_MSGID_NOT_ALLOWED_E, the code DoReceive() stores anyway, rather than WS_REKEYING, a soft retry-later status - add unit and receive-path coverage in tests/regress.c Issue: F-10569 --- src/internal.c | 26 +++++++++++--- tests/regress.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 115 insertions(+), 4 deletions(-) diff --git a/src/internal.c b/src/internal.c index 41168633a..9cd061911 100644 --- a/src/internal.c +++ b/src/internal.c @@ -710,6 +710,15 @@ INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg) } if (msg == MSGID_SERVICE_REQUEST) { + /* RFC 4253 section 7.1: no service request once the peer's KEXINIT + * lands. acceptState doesn't track a rekey. */ + if (ssh->isKeying & WOLFSSH_PEER_IS_KEYING) { + WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s", + msg, "server", "when keying"); + ssh->error = WS_MSGID_NOT_ALLOWED_E; + return 0; + } + if (ssh->acceptState == ACCEPT_KEYED) { return 1; } @@ -732,7 +741,7 @@ INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg) if (msg == MSGID_KEXINIT) { WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s", msg, "server", "when keying"); - ssh->error = WS_REKEYING; + ssh->error = WS_MSGID_NOT_ALLOWED_E; return 0; } @@ -744,7 +753,7 @@ INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg) WLOG(WS_LOG_DEBUG, "Message ID %u not the expected message %u", msg, ssh->handshake->expectMsgId); - ssh->error = WS_REKEYING; + ssh->error = WS_MSGID_NOT_ALLOWED_E; return 0; } else { @@ -823,6 +832,15 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg) } if (msg == MSGID_SERVICE_ACCEPT) { + /* RFC 4253 section 7.1: no service accept once the peer's KEXINIT + * lands. connectState doesn't track a rekey. */ + if (ssh->isKeying & WOLFSSH_PEER_IS_KEYING) { + WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s", + msg, "client", "when keying"); + ssh->error = WS_MSGID_NOT_ALLOWED_E; + return 0; + } + if (ssh->connectState == CONNECT_CLIENT_USERAUTH_REQUEST_SENT) { return 1; } @@ -845,7 +863,7 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg) if (msg == MSGID_KEXINIT) { WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s", msg, "client", "when keying"); - ssh->error = WS_REKEYING; + ssh->error = WS_MSGID_NOT_ALLOWED_E; return 0; } @@ -857,7 +875,7 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg) WLOG(WS_LOG_DEBUG, "Message ID %u not the expected message %u", msg, ssh->handshake->expectMsgId); - ssh->error = WS_REKEYING; + ssh->error = WS_MSGID_NOT_ALLOWED_E; return 0; } else { diff --git a/tests/regress.c b/tests/regress.c index 76ff4618a..00a9630da 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2142,6 +2142,39 @@ static void TestChannelAllowedAfterAuth(WOLFSSH* ssh) } +/* A service accept is allowed only when no key exchange is in flight. + * connectState does not see a rekey, so isKeying is checked too. */ +static void TestClientServiceAcceptBlockedDuringKeying(WOLFSSH* ssh) +{ + int allowed; + + ResetSession(ssh); + ssh->connectState = CONNECT_CLIENT_USERAUTH_REQUEST_SENT; + + allowed = wolfSSH_TestIsMessageAllowed(ssh, MSGID_SERVICE_ACCEPT, + WS_MSG_RECV); + AssertTrue(allowed); + + /* Same connectState, peer's rekey KEXINIT arrived first. */ + ssh->isKeying = WOLFSSH_PEER_IS_KEYING; + + allowed = wolfSSH_TestIsMessageAllowed(ssh, MSGID_SERVICE_ACCEPT, + WS_MSG_RECV); + AssertFalse(allowed); + AssertIntEQ(ssh->error, WS_MSGID_NOT_ALLOWED_E); + + /* Allowed when only this side has started a rekey: RFC 4253 section 7.1 + * requires tolerating what the peer sent before it saw our KEXINIT. */ + ResetSession(ssh); + ssh->connectState = CONNECT_CLIENT_USERAUTH_REQUEST_SENT; + ssh->isKeying = WOLFSSH_SELF_IS_KEYING; + + allowed = wolfSSH_TestIsMessageAllowed(ssh, MSGID_SERVICE_ACCEPT, + WS_MSG_RECV); + AssertTrue(allowed); +} + + /* 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. */ @@ -2300,6 +2333,64 @@ static void TestServerServiceRequestStateGated(WOLFSSH* ssh) } +/* Drive the receive path with a SERVICE_REQUEST arriving mid-rekey. + * acceptState sits at ACCEPT_KEYED throughout, so only isKeying catches + * it. */ +static void TestServerServiceRequestRejectedDuringKeying(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte payload[64]; + byte pkt[128]; + byte out[128]; + word32 pktSz; + word32 idx = 0; + + idx = AppendString(payload, sizeof(payload), idx, "ssh-userauth"); + pktSz = WrapPacket(MSGID_SERVICE_REQUEST, payload, idx, pkt, sizeof(pkt)); + + 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, pkt, pktSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ssh->acceptState = ACCEPT_KEYED; + /* The peer's rekey KEXINIT arrived, its NEWKEYS has not. */ + ssh->isKeying = WOLFSSH_PEER_IS_KEYING; + + AssertIntEQ(wolfSSH_TestDoReceive(ssh), WS_FATAL_ERROR); + AssertIntEQ(ssh->error, WS_MSGID_NOT_ALLOWED_E); + /* 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); + + /* Allowed when only this side has started a rekey. */ + ssh->error = 0; + ssh->isKeying = WOLFSSH_SELF_IS_KEYING; + AssertTrue(wolfSSH_TestIsMessageAllowed(ssh, MSGID_SERVICE_REQUEST, + WS_MSG_RECV)); + + /* Allowed once the key exchange is done. */ + ssh->error = 0; + ssh->isKeying = 0; + AssertTrue(wolfSSH_TestIsMessageAllowed(ssh, MSGID_SERVICE_REQUEST, + WS_MSG_RECV)); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + static void TestChannelOpenCallbackRejectSendsOpenFail(void) { ChannelOpenHarness harness; @@ -8006,6 +8097,7 @@ int main(int argc, char** argv) TestChannelBlockedBeforeAuth(ssh); TestChannelBlockedEveryPreAuthState(ssh); TestChannelAllowedAfterAuth(ssh); + TestClientServiceAcceptBlockedDuringKeying(ssh); TestChannelOpenRejectedBeforeKex(CONNECT_CLIENT_KEXINIT_SENT); TestChannelOpenRejectedBeforeKex(CONNECT_CLIENT_KEXDH_INIT_SENT); #ifndef NO_WOLFSSH_SERVER @@ -8014,6 +8106,7 @@ int main(int argc, char** argv) TestServerUserauthBlockedBeforeKeyed(serverSsh); TestServerOnlyUserauthMsgsBlocked(serverSsh); TestServerServiceRequestStateGated(serverSsh); + TestServerServiceRequestRejectedDuringKeying(); TestChannelOpenCallbackRejectSendsOpenFail(); TestSecondSessionChannelRejected(); TestUsernameChangeDisconnects();