From e6b324d14f1fa26f5459bcaec60b75fb557c664d Mon Sep 17 00:00:00 2001 From: John Safranek Date: Fri, 21 Aug 2026 15:06:46 -0700 Subject: [PATCH] Make a disconnect end the session SSH_MSG_DISCONNECT left nothing behind but ssh->error, which wolfSSH_stream_read() clears on entry. An application looping on the stream calls lost the code and went back to a connection already over. - Add WOLFSSH.disconnected, set by DoDisconnect() and SendDisconnect(). - DoDisconnect() sets it before decoding the payload, so a malformed message still ends the session. RFC 4253 section 11.1. - wolfSSH_stream_read() and wolfSSH_stream_send() report WS_DISCONNECT from the flag instead of reaching for the transport again. - Both guards run ahead of the channelList NULL test, so a torn-down session reports the disconnect rather than WS_BAD_ARGUMENT. - ssh.h states that undrained channel data goes with the session; internal.h states which calls the flag gates and which it does not. - regress.c: the receive side, the send side, and both of those again on a session with an open channel. Issue: F-8837 The test channel credits the peer's window too. Left at 0, SendChannelData() bails with WS_WINDOW_FULL before the wire, and the "nothing went out" checks would hold with the gate removed. --- src/internal.c | 9 +++ src/ssh.c | 20 ++++++- tests/regress.c | 136 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 4 ++ wolfssh/ssh.h | 3 + 5 files changed, 170 insertions(+), 2 deletions(-) diff --git a/src/internal.c b/src/internal.c index 31e8c6ff..e0a73d79 100644 --- a/src/internal.c +++ b/src/internal.c @@ -8174,6 +8174,10 @@ static int DoDisconnect(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) WOLFSSH_UNUSED(reasonStr); + /* RFC 4253 section 11.1, the peer is gone whether or not the rest of + * the message decodes. */ + ssh->disconnected = 1; + ret = GetUint32(&reason, buf, len, &begin); if (ret == WS_SUCCESS) { /* Skip the description text. */ @@ -16782,6 +16786,11 @@ int SendDisconnect(WOLFSSH* ssh, word32 reason) if (ssh == NULL) ret = WS_BAD_ARGUMENT; + /* Mark the session over before the send. A partial or failed send + * still ends it. */ + if (ret == WS_SUCCESS) + ssh->disconnected = 1; + if (ret == WS_SUCCESS) ret = PreparePacket(ssh, MSG_ID_SZ + UINT32_SZ + (LENGTH_SZ * 2)); diff --git a/src/ssh.c b/src/ssh.c index 38842fa1..86a1710c 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1228,7 +1228,15 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_read()"); - if (ssh == NULL || buf == NULL || bufSz == 0 || ssh->channelList == NULL) + if (ssh == NULL || buf == NULL || bufSz == 0) + return WS_BAD_ARGUMENT; + + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + + if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; if (ssh->channelList->eofRxd) { @@ -1307,7 +1315,15 @@ int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Entering wolfSSH_stream_send()"); - if (ssh == NULL || buf == NULL || ssh->channelList == NULL) + if (ssh == NULL || buf == NULL) + return WS_BAD_ARGUMENT; + + if (ssh->disconnected) { + ssh->error = WS_DISCONNECT; + return WS_FATAL_ERROR; + } + + if (ssh->channelList == NULL) return WS_BAD_ARGUMENT; if (ssh->isKeying) { diff --git a/tests/regress.c b/tests/regress.c index 86b73d55..48d0ee88 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -88,6 +88,7 @@ static void ResetSession(WOLFSSH* ssh) ssh->connectState = CONNECT_BEGIN; ssh->acceptState = ACCEPT_BEGIN; ssh->error = 0; + ssh->disconnected = 0; } @@ -2850,6 +2851,7 @@ static void TestDisconnectSetsDisconnectError(void) MemIo io; byte in[128]; byte out[32]; + byte data[8]; word32 inSz; int ret; @@ -2873,6 +2875,138 @@ static void TestDisconnectSetsDisconnectError(void) AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); AssertIntEQ(io.inOff, io.inSz); + /* The disconnect is terminal, not just this call's error. Later stream + * calls must report it rather than clearing the error and reading or + * writing more. */ + AssertTrue(ssh->disconnected); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Append a bare session channel so the stream calls have a channel to work + * on, the state a disconnect actually arrives in. */ +static void AddSessionChannel(WOLFSSH* ssh) +{ + WOLFSSH_CHANNEL* ch; + + ch = ChannelNew(ssh, ID_CHANTYPE_SESSION, 1024, 1024); + AssertNotNull(ch); + AssertIntEQ(ChannelAppend(ssh, ch), WS_SUCCESS); + ch->openConfirmed = 1; + /* Credit the peer's window too. Left at 0, SendChannelData() bails with + * WS_WINDOW_FULL before the wire, and the "nothing went out" checks + * would hold with the gates removed. */ + ch->peerWindowSz = 1024; + ch->peerMaxPacketSz = 1024; +} + + +/* The same received disconnect on an established session. Without a channel + * the stream calls bail out on the NULL channel list before they reach + * anything, so this is the case that shows the gate doing work. */ +static void TestDisconnectTerminalWithChannel(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[128]; + byte data[8]; + word32 inSz; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Past userauth, or the message filter blocks the sends on its own. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = DoReceive(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertTrue(ssh->disconnected); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* Nothing may go out on the channel either. */ + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Sending SSH_MSG_DISCONNECT ends the session the same way receiving one + * does: RFC 4253 section 11.1 says the connection is over once the message + * goes out, so the stream calls must refuse afterwards. */ +static void TestSendDisconnectIsTerminal(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte out[128]; + byte data[8]; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Past userauth, or the message filter blocks the sends on its own. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + ret = wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION); + AssertIntEQ(ret, WS_SUCCESS); + AssertTrue(ssh->disconnected); + AssertTrue(io.outSz > 0); + + WMEMSET(data, 0, sizeof(data)); + ret = wolfSSH_stream_send(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + ret = wolfSSH_stream_read(ssh, data, sizeof(data)); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); } @@ -6603,6 +6737,8 @@ int main(int argc, char** argv) TestDoNewKeys(); #endif TestDisconnectSetsDisconnectError(); + TestDisconnectTerminalWithChannel(); + TestSendDisconnectIsTerminal(); #if !(defined(WOLFSSH_NO_RSA) && defined(WOLFSSH_NO_ECDSA_SHA2_NISTP256)) TestClientBuffersIdempotent(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index f50a28c8..898eafca 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1102,6 +1102,10 @@ struct WOLFSSH { #endif byte connReset; byte isClosed; + /* Set when a DISCONNECT is sent or received. Only wolfSSH_stream_read() + * and wolfSSH_stream_send() are gated on it; the channel-id calls and + * wolfSSH_worker() are not, since the shutdown paths still pump them. */ + byte disconnected; byte clientOpenSSH; byte kexId; diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index dfb45ab6..842de304 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -558,6 +558,9 @@ WOLFSSH_API int wolfSSH_CTX_SetWindowPacketSize(WOLFSSH_CTX* ctx, WOLFSSH_API int wolfSSH_accept(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); +/* A disconnect, sent or received, ends the session: wolfSSH_stream_read() + * and wolfSSH_stream_send() report WS_DISCONNECT from then on, and channel + * data that arrived before it but was never drained is dropped. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz); WOLFSSH_API int wolfSSH_stream_send(WOLFSSH* ssh, byte* buf, word32 bufSz);