From 47b547c6343b581a698afdae693a0519f4b15471 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 28 Oct 2026 15:46:50 -0700 Subject: [PATCH] ssh, internal: send one KEXINIT per key exchange RFC 4253 7.1 allows one KEXINIT from each side per key exchange. wolfSSH_TriggerKeyExchange() is now a no-op returning WS_SUCCESS while any keying is in progress, and SendKexInit() skips once this side's KEXINIT is out, so the stored copy feeding the exchange hash stays put. - covers the highwater callback, whose byte and message marks can both fire in one rekey - covers the window after SendNewKeys() clears the self-keying flag while the peer's NEWKEYS is still due - regress: second trigger, nested highwater and NEWKEYS-window tests Issue: F-16775 --- src/internal.c | 10 +++ src/ssh.c | 7 +- tests/regress.c | 225 ++++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/ssh.h | 3 +- 4 files changed, 243 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index 3b305d0d3..559237fcb 100644 --- a/src/internal.c +++ b/src/internal.c @@ -15842,6 +15842,16 @@ int SendKexInit(WOLFSSH* ssh) if (ssh == NULL) ret = WS_BAD_ARGUMENT; + /* RFC 4253 7.1: one KEXINIT per key exchange. Already sent one, so + * there is nothing to do; replacing the stored copy would corrupt the + * exchange hash. Not an error: a highwater callback's return reaches + * the caller's unrelated send. */ + if (ret == WS_SUCCESS && (ssh->isKeying & WOLFSSH_SELF_IS_KEYING)) { + WLOG(WS_LOG_DEBUG, "KEXINIT already sent, skipping"); + WLOG(WS_LOG_DEBUG, "Leaving SendKexInit(), ret = %d", ret); + return ret; + } + if (ret == WS_SUCCESS && ssh->ctx->side == WOLFSSH_ENDPOINT_SERVER && ssh->ctx->privateKeyCount == 0) { WLOG(WS_LOG_DEBUG, "Server needs at least one private key"); diff --git a/src/ssh.c b/src/ssh.c index 09bf297fe..63f702eba 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1351,7 +1351,12 @@ int wolfSSH_TriggerKeyExchange(WOLFSSH* ssh) if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) ret = WS_FATAL_ERROR; - if (ret == WS_SUCCESS) { + /* A key exchange is already running, including the stretch after this + * side's NEWKEYS while the peer's is still due. It will rekey anyway. */ + if (ret == WS_SUCCESS && ssh->isKeying != 0) { + WLOG(WS_LOG_DEBUG, "Key exchange in progress, not starting another"); + } + else if (ret == WS_SUCCESS) { ret = SendKexInit(ssh); if (ret != WS_SUCCESS) ssh->error = ret; diff --git a/tests/regress.c b/tests/regress.c index 658405cfb..dedc8301b 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -3088,6 +3088,228 @@ static void TestStrictKexResetsSequenceNumbersOnRekey(void) AssertRekeyResetsSequenceNumbers(0); } +/* Run both ends like PumpDuplexPair(), but record the first fatal error on + * each side instead of asserting, so a test can look at how a broken rekey + * ended. Stops once both queues are empty and neither side is keying, or + * once either side has failed. */ +static void PumpDuplexPairNoAssert(KexReplyHarness* harness, + int* clientErr, int* serverErr) +{ + word32 step; + + *clientErr = WS_SUCCESS; + *serverErr = WS_SUCCESS; + + for (step = 0; step < REGRESS_MAX_HANDSHAKE_STEPS; step++) { + int ret; + + ret = wolfSSH_worker(harness->client, NULL); + if (ret < WS_SUCCESS) { + int err = wolfSSH_get_error(harness->client); + if (!IsPumpRetryable(err)) { + *clientErr = err; + } + } + + ret = wolfSSH_worker(harness->server, NULL); + if (ret < WS_SUCCESS) { + int err = wolfSSH_get_error(harness->server); + if (!IsPumpRetryable(err)) { + *serverErr = err; + } + } + + if (*clientErr != WS_SUCCESS || *serverErr != WS_SUCCESS) { + return; + } + + if (harness->clientIo.inbound.len == 0 && + harness->serverIo.inbound.len == 0 && + harness->client->isKeying == 0 && + harness->server->isKeying == 0) { + return; + } + } +} + +/* RFC 4253 7.1: once a side has sent KEXINIT it must not send another until + * it has sent NEWKEYS. Asking an established session for a rekey twice in a + * row must put one KEXINIT on the wire, keep the stored copy that feeds the + * exchange hash, and let the rekey finish on both ends. */ +static void AssertSecondTriggerSendsNoKexInit(byte serverTriggers) +{ + KexReplyHarness harness; + KexReplyRunResult result; + WOLFSSH* ssh; + word32 msgCount, extraMsgs; + byte cookie[COOKIE_SZ]; + int clientErr, serverErr, sameCookie; + + InitKexReplyHarness(&harness, REGRESS_DEFAULT_KEY_ALGO, + REGRESS_DEFAULT_KEY_PATH, 0, NULL); + RunKexReplyHandshake(&harness, &result); + AssertTrue(result.clientSuccess); + AssertTrue(result.serverSuccess); + + ssh = serverTriggers ? harness.server : harness.client; + + AssertIntEQ(wolfSSH_TriggerKeyExchange(ssh), WS_SUCCESS); + AssertTrue((ssh->isKeying & WOLFSSH_SELF_IS_KEYING) != 0); + AssertNotNull(ssh->handshake); + AssertNotNull(ssh->handshake->kexInit); + msgCount = ssh->txMsgCount; + /* The stored copy is length, msg id, then the cookie. */ + WMEMCPY(cookie, ssh->handshake->kexInit + LENGTH_SZ + MSG_ID_SZ, + COOKIE_SZ); + + AssertIntEQ(wolfSSH_TriggerKeyExchange(ssh), WS_SUCCESS); + extraMsgs = ssh->txMsgCount - msgCount; + AssertNotNull(ssh->handshake); + AssertNotNull(ssh->handshake->kexInit); + sameCookie = WMEMCMP(cookie, + ssh->handshake->kexInit + LENGTH_SZ + MSG_ID_SZ, COOKIE_SZ) == 0; + + PumpDuplexPairNoAssert(&harness, &clientErr, &serverErr); + + /* No second packet, and the stored KEXINIT is the one that was sent. */ + AssertIntEQ(extraMsgs, 0); + AssertTrue(sameCookie); + AssertIntEQ(clientErr, WS_SUCCESS); + AssertIntEQ(serverErr, WS_SUCCESS); + AssertIntEQ(harness.client->isKeying, 0); + AssertIntEQ(harness.server->isKeying, 0); + AssertIntEQ(harness.client->disconnected, 0); + AssertIntEQ(harness.server->disconnected, 0); + + FreeKexReplyHarness(&harness); +} + +static void TestSecondTriggerSendsNoKexInit(void) +{ + AssertSecondTriggerSendsNoKexInit(0); + AssertSecondTriggerSendsNoKexInit(1); +} + +/* HighwaterCheck() latches the byte mark and the message mark separately, + * and both call the default callback, which rekeys. Arm them so the byte + * mark fires on the next send and the message mark fires on the KEXINIT + * that rekey sends: the second firing lands while this side is already + * keying and must not send another KEXINIT. */ +static void TestSecondHighwaterSendsNoKexInit(void) +{ + KexReplyHarness harness; + KexReplyRunResult result; + WOLFSSH* ssh; + word32 msgCount, sentMsgs; + int clientErr, serverErr; + + InitKexReplyHarness(&harness, REGRESS_DEFAULT_KEY_ALGO, + REGRESS_DEFAULT_KEY_PATH, 0, NULL); + RunKexReplyHandshake(&harness, &result); + AssertTrue(result.clientSuccess); + AssertTrue(result.serverSuccess); + + /* The server: after the handshake it has received no more messages + * than it has sent, which the message mark below relies on. */ + ssh = harness.server; + AssertIntEQ(ssh->isKeying, 0); + AssertIntEQ(ssh->highwaterFlag, 0); + AssertIntEQ(ssh->msgHighwaterFlag, 0); + + msgCount = ssh->txMsgCount; + /* The IGNORE crosses the byte mark; the KEXINIT it triggers is the + * second packet and crosses the message mark. Received messages count + * toward that mark too, so start it past them; nothing arrives here + * before the KEXINIT goes out. */ + AssertTrue(ssh->rxMsgCount < msgCount + 2); + ssh->highwaterMark = ssh->txCount + 1; + ssh->msgHighwaterMark = msgCount + 2; + + AssertIntEQ(wolfSSH_SendIgnore(ssh, NULL, 0), WS_SUCCESS); + AssertTrue((ssh->isKeying & WOLFSSH_SELF_IS_KEYING) != 0); + AssertIntEQ(ssh->highwaterFlag, 1); + AssertIntEQ(ssh->msgHighwaterFlag, 1); + sentMsgs = ssh->txMsgCount - msgCount; + + PumpDuplexPairNoAssert(&harness, &clientErr, &serverErr); + + /* The IGNORE and one KEXINIT. */ + AssertIntEQ(sentMsgs, 2); + AssertIntEQ(clientErr, WS_SUCCESS); + AssertIntEQ(serverErr, WS_SUCCESS); + AssertIntEQ(harness.client->isKeying, 0); + AssertIntEQ(harness.server->isKeying, 0); + + FreeKexReplyHarness(&harness); +} + +/* SendNewKeys() clears the self-keying flag as soon as this side's NEWKEYS + * goes out, while the peer's NEWKEYS is still to come and the handshake is + * still live. A rekey request in that window must not send a KEXINIT or + * touch the handshake: the client starts a rekey, the ends are stepped one + * worker call at a time until the probed side has sent its NEWKEYS but not + * yet received the peer's, and that side is asked to rekey. */ +static void AssertTriggerAfterNewKeysSendsNoKexInit(byte probeServer) +{ + KexReplyHarness harness; + KexReplyRunResult result; + WOLFSSH* ssh; + HandshakeInfo* handshake; + byte* kexInit; + word32 step, msgCount; + int clientErr, serverErr, inWindow = 0; + + InitKexReplyHarness(&harness, REGRESS_DEFAULT_KEY_ALGO, + REGRESS_DEFAULT_KEY_PATH, 0, NULL); + RunKexReplyHandshake(&harness, &result); + AssertTrue(result.clientSuccess); + AssertTrue(result.serverSuccess); + + ssh = probeServer ? harness.server : harness.client; + + AssertIntEQ(wolfSSH_TriggerKeyExchange(harness.client), WS_SUCCESS); + + for (step = 0; step < REGRESS_MAX_HANDSHAKE_STEPS && !inWindow; step++) { + WOLFSSH* next = (step & 1) ? harness.client : harness.server; + int ret; + + ret = wolfSSH_worker(next, NULL); + if (ret < WS_SUCCESS) { + AssertTrue(IsPumpRetryable(wolfSSH_get_error(next))); + } + inWindow = (ssh->isKeying == WOLFSSH_PEER_IS_KEYING); + } + AssertTrue(inWindow); + AssertNotNull(ssh->handshake); + + handshake = ssh->handshake; + kexInit = handshake->kexInit; + msgCount = ssh->txMsgCount; + + AssertIntEQ(wolfSSH_TriggerKeyExchange(ssh), WS_SUCCESS); + AssertIntEQ(ssh->txMsgCount, msgCount); + AssertTrue(ssh->handshake == handshake); + AssertTrue(handshake->kexInit == kexInit); + AssertIntEQ(ssh->isKeying, WOLFSSH_PEER_IS_KEYING); + + PumpDuplexPairNoAssert(&harness, &clientErr, &serverErr); + + AssertIntEQ(clientErr, WS_SUCCESS); + AssertIntEQ(serverErr, WS_SUCCESS); + AssertIntEQ(harness.client->isKeying, 0); + AssertIntEQ(harness.server->isKeying, 0); + AssertIntEQ(harness.client->disconnected, 0); + AssertIntEQ(harness.server->disconnected, 0); + + FreeKexReplyHarness(&harness); +} + +static void TestTriggerAfterNewKeysSendsNoKexInit(void) +{ + AssertTriggerAfterNewKeysSendsNoKexInit(1); + AssertTriggerAfterNewKeysSendsNoKexInit(0); +} + #endif /* KEXDH_REPLY_REGRESS_KEX_ALGO */ /* Shared with the client-side forwarding tests below. */ @@ -21813,6 +22035,9 @@ int main(int argc, char** argv) TestStrictKexMarkerNotSentOnRekey(); TestStrictKexResetsSequenceNumbers(); TestStrictKexResetsSequenceNumbersOnRekey(); + TestSecondTriggerSendsNoKexInit(); + TestSecondHighwaterSendsNoKexInit(); + TestTriggerAfterNewKeysSendsNoKexInit(); #ifndef WOLFSSH_NO_AES_GCM TestTerrapinInjectionAcceptedWithoutStrictKex(); #endif diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 000aa9051..4ccd793cc 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -989,7 +989,8 @@ WOLFSSH_API int wolfSSH_stream_exit(WOLFSSH* ssh, int status); WOLFSSH_API int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz); /* Starts a key exchange. A clean start leaves ssh->error alone; only a - * failure records its code there. */ + * failure records its code there. While a key exchange is already in + * progress this is a no-op that returns WS_SUCCESS without sending. */ WOLFSSH_API int wolfSSH_TriggerKeyExchange(WOLFSSH* ssh); /* Returns WS_INVALID_STATE_E before the initial KEX completes when strict * KEX is offered. */