From 5cf6618c7686e16bc92b1f9e77dfab8f60b7d36b Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 14:04:18 -0700 Subject: [PATCH 1/2] Gate the handshake drivers on a disconnect wolfSSH_accept() and wolfSSH_connect() drive the handshake only while the session is live. Both gate on SendAfterDisconnect() ahead of the pending-send block, which would otherwise flush a queued disconnect and count it as the next handshake message; the shutdown paths own that flush. - The prototype sits ahead of both drivers, since either can be the only one built. - TestDisconnectGatesAccept() and TestDisconnectGatesConnect() cover a local disconnect, one from the peer, and a queued short send. One test per endpoint, so a single-sided build keeps the coverage that applies to it. - A received disconnect used to reach the error-state test in wolfSSH_accept() and report WS_INVALID_STATE_E; the gate answers WS_FATAL_ERROR first, and both tests pin that. - The contract comments in ssh.h and internal.h drop the ungated note. --- src/ssh.c | 14 +++ tests/regress.c | 222 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 5 +- wolfssh/ssh.h | 7 +- 4 files changed, 242 insertions(+), 6 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index c15ccb612..05ea33f9e 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -550,6 +550,10 @@ int wolfSSH_CTX_UseTpmHostKey(WOLFSSH_CTX* ctx, #endif /* WOLFSSH_TPM */ +/* Defined below, ahead of both drivers; either can be the only one built. */ +static int SendAfterDisconnect(WOLFSSH* ssh); + + #ifndef NO_WOLFSSH_SERVER const char acceptError[] = "accept error: %s, %d"; @@ -563,6 +567,11 @@ int wolfSSH_accept(WOLFSSH* ssh) if (ssh == NULL) return WS_BAD_ARGUMENT; + /* No handshake on a session that is over. The pending-send block below + * would flush a queued disconnect as the next handshake message. */ + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; + /* clear want read/writes for retry */ if (ssh->error == WS_WANT_READ || ssh->error == WS_WANT_WRITE || ssh->error == WS_AUTH_PENDING) ssh->error = 0; @@ -826,6 +835,11 @@ int wolfSSH_connect(WOLFSSH* ssh) if (ssh == NULL) return WS_BAD_ARGUMENT; + /* See wolfSSH_accept(). No error-state test here, so the peer's + * disconnect reaches the state machine like a local one. */ + if (SendAfterDisconnect(ssh)) + return WS_FATAL_ERROR; + /* check if data pending to be sent */ if (ssh->outputBuffer.length > 0 && ssh->connectState < CONNECT_SERVER_CHANNEL_REQUEST_DONE) { diff --git a/tests/regress.c b/tests/regress.c index 76ff4618a..49baf3b85 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -4194,6 +4194,222 @@ static void TestDisconnectOutranksRekey(void) } + +#ifndef NO_WOLFSSH_SERVER + +/* wolfSSH_accept() drives the handshake, so a session that is already over + * must stop it the way it stops every other sender. The short-send case is + * the sharp one: the pending-send block at the top would push out a + * disconnect left queued by a short send and then count it as the handshake + * message the state machine was waiting for. RFC 4253 section 11.1. */ +static void TestDisconnectGatesAccept(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[512]; + word32 inSz; + word32 quietSz; + byte state; + int ret; + + /* A local disconnect leaves ssh->error clear, so the "in error state" + * test in wolfSSH_accept() never sees it. */ + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Not one of the states wolfSSH_accept() holds back, so an unwanted + * advance shows up in the assertions below. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(ssh->disconnected); + AssertIntEQ(wolfSSH_get_error(ssh), 0); + quietSz = io.outSz; + state = ssh->acceptState; + + ret = wolfSSH_accept(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + /* No handshake packet, and the state machine did not move. */ + AssertIntEQ(io.outSz, quietSz); + AssertIntEQ(ssh->acceptState, state); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + + /* Our disconnect short-sends, so it is sitting in the output buffer + * with a flush owed. wolfSSH_shutdown() and wolfSSH_SendDisconnect() + * own that flush; wolfSSH_accept() must leave it alone. */ + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + /* Not one of the states wolfSSH_accept() holds back, so an unwanted + * advance shows up in the assertions below. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertTrue(ssh->disconnected); + AssertTrue(ssh->disconnectTxd); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + state = ssh->acceptState; + + ret = wolfSSH_accept(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + /* Still queued, and not mistaken for the awaited handshake message. */ + AssertIntEQ(io.outSz, 0); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(ssh->acceptState, state); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + + /* The peer's disconnect latches WS_DISCONNECT, which the error-state + * test below the gate used to answer with WS_INVALID_STATE_E. */ + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSend); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + + inSz = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + quietSz = io.outSz; + state = ssh->acceptState; + + ret = wolfSSH_accept(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, quietSz); + AssertIntEQ(ssh->acceptState, state); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + +#endif /* !NO_WOLFSSH_SERVER */ + + +#ifndef NO_WOLFSSH_CLIENT + +/* The same gate on the client's driver, which has no error-state test of + * its own. Split from the accept test so a single-sided build keeps the + * coverage that applies to it. */ +static void TestDisconnectGatesConnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[128]; + byte out[512]; + word32 inSz; + byte state; + int ret; + + /* wolfSSH_connect() has no error-state test of its own, so it reaches + * the state machine after a disconnect from either side. */ + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + wolfSSH_SetIORecv(ctx, MemRecv); + wolfSSH_SetIOSend(ctx, MemSendWantWrite); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AddSessionChannel(ssh); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + MemIoInit(&io, NULL, 0, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + MemSendWantWriteCount = 1; + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_WANT_WRITE); + AssertTrue(ssh->disconnected); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(io.outSz, 0); + state = ssh->connectState; + + ret = wolfSSH_connect(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + AssertTrue(wolfSSH_OutputPending(ssh)); + AssertIntEQ(ssh->connectState, state); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + + /* The peer's disconnect reaches the same gate. */ + 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); + 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); + + AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR); + AssertTrue(ssh->disconnected); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + state = ssh->connectState; + + ret = wolfSSH_connect(ssh); + AssertIntEQ(ret, WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, 0); + AssertIntEQ(ssh->connectState, state); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + +#endif /* !NO_WOLFSSH_CLIENT */ + + /* disconnectTxd means "a flush is owed", not "a disconnect was sent". Once * ours has gone out, a teardown call must not push whatever the internal * senders queued behind it. */ @@ -8079,6 +8295,12 @@ int main(int argc, char** argv) TestDisconnectDrainsBufferedData(); TestDisconnectBlocksEverySend(); TestSendDisconnectIsTerminal(); +#ifndef NO_WOLFSSH_SERVER + TestDisconnectGatesAccept(); +#endif +#ifndef NO_WOLFSSH_CLIENT + TestDisconnectGatesConnect(); +#endif TestDisconnectQuietWindowAdjust(); TestDisconnectBlocksChannelAndFwdSends(); TestStreamExitReportsDisconnect(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index c4a3d8951..90ea84af3 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1105,9 +1105,8 @@ struct WOLFSSH { /* Set when a DISCONNECT is sent or received. Gates the public send * calls, so nothing more goes out. Reads still hand back what arrived * before the disconnect; the head-of-list reads report it once their - * buffer runs dry, unless a CHANNEL_EOF arrived first. wolfSSH_worker(), - * wolfSSH_accept() and wolfSSH_connect() are not gated; the shutdown - * paths pump the worker. */ + * buffer runs dry, unless a CHANNEL_EOF arrived first. + * wolfSSH_worker() is not gated; the shutdown paths pump the worker. */ byte disconnected; /* Set once SendDisconnect() has bundled our own DISCONNECT into the * output buffer, and cleared once wolfSSH_SendPacket() drains it, so it diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 9d856d771..7f5c6c0b7 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -563,9 +563,10 @@ WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); /* A disconnect, sent or received, ends the session. Nothing more goes out: * wolfSSH_shutdown() above this comment, and every send call below it, - * report WS_DISCONNECT from then on. wolfSSH_accept() and - * wolfSSH_connect() are not gated; do not drive the handshake after a - * disconnect. Reads are not gated either, so channel data that + * report WS_DISCONNECT from then on, as do wolfSSH_accept() and + * wolfSSH_connect(). Neither driver flushes a disconnect of ours left queued + * by a short send; wolfSSH_shutdown() or another wolfSSH_SendDisconnect() + * owns that. Reads are not gated, so channel data that * arrived before the disconnect can still be drained; wolfSSH_stream_read() * and wolfSSH_stream_peek() report WS_DISCONNECT once their buffer runs * dry. A CHANNEL_EOF already received outranks that drain: both report From e95c975bef8fbb232d4fb9b6b86335bb6625601b Mon Sep 17 00:00:00 2001 From: John Safranek Date: Thu, 27 Aug 2026 21:55:00 -0700 Subject: [PATCH 2/2] Stop answering traffic after a disconnect DoPacket() skips the whole message dispatch once ssh->disconnected is set, for every message but a DISCONNECT. The handlers that answer must not -- a close draws an EOF and a close of ours, a request a success or failure, an open a confirmation, an unknown message an UNIMPLEMENTED -- and what the rest would record is of no use to a caller that can no longer send. RFC 4253 section 11.1. - Inbound data from here on is dropped rather than buffered, so the read path hands back only what arrived before the disconnect. ssh.h and internal.h say so, beside the calls and beside the flag. - A DISCONNECT still reaches DoDisconnect(), which sends nothing and is what latches WS_DISCONNECT; SendDisconnect() sets the flag too, so ours can be the one that raised it. - The frame advance steps over the whole packet, so the stream stays in step with no payload bookkeeping of its own. - wolfSSH_worker() gates on SendAfterDisconnect() the way wolfSSH_accept() and wolfSSH_connect() do. With the dispatch skipped there is no non-success left to return, so it would answer a healthy session for as long as the peer kept talking and the SFTP and SCP drive loops would keep pumping a dead one. The rekey test below the gate needs no disconnect of its own: the gate returns first, and a DISCONNECT arriving mid-pass leaves ret fatal. wolfSSH_shutdown() drops the channel on a disconnect before its own pump, so the gate does not cost it the read it does there. - tests/regress.c pins all four: no reply goes out, the stream stays in step with a disconnect queued behind a skipped close, late channel data is dropped rather than queued, and the worker reports the disconnect on the pass that takes it and on every pass behind it. --- src/internal.c | 12 +- src/ssh.c | 14 ++- tests/regress.c | 283 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 6 +- wolfssh/ssh.h | 19 +-- 5 files changed, 323 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 41168633a..fdaf71639 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12225,7 +12225,17 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) return WS_MSGID_NOT_ALLOWED_E; } - switch (msg) { + /* The session is over, RFC 4253 section 11.1, so skip the whole dispatch: + * the handlers that answer must not, and what the rest would record is of + * no use to a caller that can no longer send. Inbound data from here on is + * dropped rather than buffered. The frame advance at the end steps over + * the packet, so the stream stays in step. A DISCONNECT still dispatches, + * since DoDisconnect() sends nothing and latches the error. */ + if (ssh->disconnected && msg != MSGID_DISCONNECT) { + WLOG(WS_LOG_DEBUG, "Ignoring message ID %u after a disconnect", + (word32)msg); + } + else switch (msg) { case MSGID_DISCONNECT: WLOG(WS_LOG_DEBUG, "Decoding MSGID_DISCONNECT"); diff --git a/src/ssh.c b/src/ssh.c index 05ea33f9e..32c234b2a 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3604,6 +3604,16 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) if (ssh == NULL) ret = WS_BAD_ARGUMENT; + /* Nothing left to drive: no reply may go out and inbound messages are + * skipped, so every pass from here on would answer WS_SUCCESS off a + * dispatch that did nothing and a caller turning the crank would never + * see the session end. What arrived before the disconnect is still the + * caller's, through the read calls. RFC 4253 section 11.1. */ + if (ret == WS_SUCCESS && SendAfterDisconnect(ssh)) { + WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_worker(), session disconnected"); + return WS_FATAL_ERROR; + } + #ifdef WOLFSSH_TEST_BLOCK /* In forced non-blocking test mode, keep legacy ordering (send before * receive) to match the harness expectations and avoid synthetic spins. */ @@ -3653,7 +3663,9 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) } /* WS_EXTDATA is raised once, on arrival; masking it would strand the - * buffered stderr and its window credit. */ + * buffered stderr and its window credit. A disconnect cannot be seen + * here: the gate at the top returns before this, and the DISCONNECT + * that sets the flag mid-pass leaves ret fatal. */ if (ssh->isKeying && ret != WS_EXTDATA) { ssh->error = WS_REKEYING; return WS_REKEYING; diff --git a/tests/regress.c b/tests/regress.c index 49baf3b85..daad3edda 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -265,6 +265,31 @@ static WS_MAYBE_UNUSED word32 BuildExtInfoSigAlgs(byte* buf, word32 bufSz, return AppendString(buf, bufSz, idx, sigAlgs); } +static word32 BuildChannelClosePacket(word32 peerChannelId, byte* out, + word32 outSz) +{ + byte payload[16]; + word32 idx = 0; + + idx = AppendUint32(payload, sizeof(payload), idx, peerChannelId); + + return WrapPacket(MSGID_CHANNEL_CLOSE, payload, idx, out, outSz); +} + + +static word32 BuildChannelDataPacket(word32 peerChannelId, const char* data, + byte* out, word32 outSz) +{ + byte payload[64]; + word32 idx = 0; + + idx = AppendUint32(payload, sizeof(payload), idx, peerChannelId); + idx = AppendString(payload, sizeof(payload), idx, data); + + return WrapPacket(MSGID_CHANNEL_DATA, payload, idx, out, outSz); +} + + #ifdef WOLFSSH_FWD static word32 BuildDirectTcpipExtra(const char* host, word32 hostPort, const char* origin, word32 originPort, byte* out, word32 outSz) @@ -4194,6 +4219,68 @@ static void TestDisconnectOutranksRekey(void) } +/* wolfSSH_worker() is the other drive loop, and the one the SFTP and SCP + * layers turn. Once the session is over it has nothing to drive: the + * dispatch is skipped, so a post-disconnect message would leave + * ssh->error at WS_SUCCESS and the worker would keep reporting a healthy + * session for as long as the peer talks. It also outranks a rekey the peer + * abandoned, which only NEWKEYS could clear. RFC 4253 section 11.1. */ +static void TestWorkerReportsDisconnect(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[256]; + byte out[256]; + word32 inSz; + word32 idx; + + 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); + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* The peer's KEXINIT, the way DoKexInit records it. Nothing clears it + * after the disconnect below, so it latches for the session. */ + ssh->isKeying |= WOLFSSH_PEER_IS_KEYING; + + /* The peer's disconnect, then a message behind it. An IGNORE draws no + * reply of its own, so what goes out can only come from the worker. */ + idx = BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in, sizeof(in)); + inSz = idx + BuildPacket(MSGID_IGNORE, in + idx, sizeof(in) - idx); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_worker(ssh, NULL), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertTrue(ssh->disconnected); + AssertTrue(ssh->isKeying != 0); + io.outSz = 0; + + /* The message behind it is still queued, and every further pass reports + * the disconnect rather than the WS_SUCCESS of a skipped dispatch or the + * WS_REKEYING of a rekey that cannot finish. */ + AssertIntEQ(wolfSSH_worker(ssh, NULL), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(wolfSSH_worker(ssh, NULL), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + /* Nothing went out on any of them. */ + AssertIntEQ(io.outSz, 0); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + #ifndef NO_WOLFSSH_SERVER @@ -4410,6 +4497,198 @@ static void TestDisconnectGatesConnect(void) #endif /* !NO_WOLFSSH_CLIENT */ +/* The public senders sit behind the disconnect gate, but the replies the + * library builds in answer to inbound traffic did not. A channel close + * draws an EOF and a close of ours out of DoChannelClose(), and a channel + * open a confirmation or a failure out of DoChannelOpen(); on a session + * that is already over, none of that may reach the peer. RFC 4253 + * section 11.1. */ +static void TestDisconnectSilencesInboundReplies(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte in[256]; + byte out[512]; + word32 inSz; + word32 quietSz; + word32 channelId; + + 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); + channel = ssh->channelList; + channelId = channel->channel; + /* Past userauth, or the message filter turns the inbound messages away + * on its own and the wire check below proves nothing. */ + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + inSz = BuildChannelClosePacket(channelId, in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Our own disconnect goes out first, and is the last thing that may. */ + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(ssh->disconnected); + quietSz = io.outSz; + AssertTrue(quietSz > 0); + + /* The peer's close arrives anyway: a caller's own wolfSSH_worker() loop + * still reads after a disconnect, and shutdown's pump can find packets + * queued behind the peer's DISCONNECT. A skipped packet + * is not an error; assert that, so the quiet-wire checks below cannot + * pass on a receive that never reached DoPacket(). */ + AssertIntEQ(DoReceive(ssh), WS_SUCCESS); + + /* No EOF and no close went out, and the handler never ran, so the + * channel it would have torn down is still on the list. */ + AssertIntEQ(io.outSz, quietSz); + AssertNotNull(ssh->channelList); + AssertIntEQ(ssh->channelList->channel, channelId); + AssertFalse(channel->eofTxd); + AssertFalse(channel->closeTxd); + + /* A channel open is the other half: it answers with a confirmation or + * a failure, and neither may go out now. */ + inSz = BuildChannelOpenPacket("session", 99, 1024, 1024, NULL, 0, + in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + io.outSz = quietSz; + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(DoReceive(ssh), WS_SUCCESS); + AssertIntEQ(io.outSz, quietSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Skipping the dispatch is all the gate does: the frame advance steps over + * the whole packet on its own, so a packet behind a skipped one is still + * found where it should be. And a DISCONNECT is not skipped -- DoDisconnect() + * sends nothing, and it is what latches WS_DISCONNECT for the caller, so + * swallowing the peer's would report a live session on a dead one. */ +static void TestDisconnectKeepsStreamInStep(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + WOLFSSH_CHANNEL* channel; + MemIo io; + byte in[256]; + byte out[512]; + word32 inSz; + word32 quietSz; + word32 channelId; + + 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); + channel = ssh->channelList; + channelId = channel->channel; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + /* A channel close for the gate to skip, with the peer's disconnect behind + * it in the same read. */ + inSz = BuildChannelClosePacket(channelId, in, sizeof(in)); + inSz += BuildDisconnectPacket(WOLFSSH_DISCONNECT_BY_APPLICATION, + in + inSz, (word32)sizeof(in) - inSz); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + /* Ours goes out first, and is the last thing that may. */ + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(ssh->disconnected); + quietSz = io.outSz; + AssertTrue(quietSz > 0); + + /* The close is skipped, so no reply and the channel stays. */ + AssertIntEQ(DoReceive(ssh), WS_SUCCESS); + AssertIntEQ(io.outSz, quietSz); + AssertNotNull(ssh->channelList); + AssertIntEQ(ssh->channelList->channel, channelId); + AssertFalse(channel->eofTxd); + AssertFalse(channel->closeTxd); + + /* The disconnect behind it decodes and latches, and still answers + * nothing. */ + AssertIntEQ(DoReceive(ssh), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + AssertIntEQ(io.outSz, quietSz); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + + +/* Skipping the dispatch drops what arrives, it does not queue it: the gate + * covers inbound data, not only replies. A caller that can no longer send has + * nothing to do with it. Pinned because the read path still hands back data + * that arrived before the disconnect, and the two are easy to confuse. */ +static void TestDisconnectDropsLateChannelData(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + MemIo io; + byte in[256]; + byte out[512]; + byte buf[32]; + word32 inSz; + word32 channelId; + + 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); + channelId = ssh->channelList->channel; + ssh->connectState = CONNECT_SERVER_USERAUTH_ACCEPT_DONE; + + inSz = BuildChannelDataPacket(channelId, "late", in, sizeof(in)); + MemIoInit(&io, in, inSz, out, sizeof(out)); + wolfSSH_SetIOReadCtx(ssh, &io); + wolfSSH_SetIOWriteCtx(ssh, &io); + + AssertIntEQ(wolfSSH_SendDisconnect(ssh, WOLFSSH_DISCONNECT_BY_APPLICATION), + WS_SUCCESS); + AssertTrue(ssh->disconnected); + + /* Read it, and it is gone: nothing buffered on the channel. */ + AssertIntEQ(DoReceive(ssh), WS_SUCCESS); + AssertNotNull(ssh->channelList); + AssertIntEQ(ssh->channelList->inputBuffer.length + - ssh->channelList->inputBuffer.idx, 0); + + /* So the read reports the dead session rather than the dropped bytes. */ + AssertIntEQ(wolfSSH_stream_read(ssh, buf, sizeof(buf)), WS_FATAL_ERROR); + AssertIntEQ(wolfSSH_get_error(ssh), WS_DISCONNECT); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + /* disconnectTxd means "a flush is owed", not "a disconnect was sent". Once * ours has gone out, a teardown call must not push whatever the internal * senders queued behind it. */ @@ -8301,6 +8580,9 @@ int main(int argc, char** argv) #ifndef NO_WOLFSSH_CLIENT TestDisconnectGatesConnect(); #endif + TestDisconnectSilencesInboundReplies(); + TestDisconnectKeepsStreamInStep(); + TestDisconnectDropsLateChannelData(); TestDisconnectQuietWindowAdjust(); TestDisconnectBlocksChannelAndFwdSends(); TestStreamExitReportsDisconnect(); @@ -8315,6 +8597,7 @@ int main(int argc, char** argv) TestShutdownKeepsFlushWantWrite(); TestDisconnectTxdClearsOnFlush(); TestDisconnectOutranksRekey(); + TestWorkerReportsDisconnect(); #if defined(WOLFSSH_TERM) && !defined(NO_FILESYSTEM) TestTerminalResizeBlockedAfterDisconnect(); #endif diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 90ea84af3..f3c7fd1f4 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1106,7 +1106,11 @@ struct WOLFSSH { * calls, so nothing more goes out. Reads still hand back what arrived * before the disconnect; the head-of-list reads report it once their * buffer runs dry, unless a CHANNEL_EOF arrived first. - * wolfSSH_worker() is not gated; the shutdown paths pump the worker. */ + * wolfSSH_worker() reports it as well, so a drive loop stops turning; + * wolfSSH_shutdown() drops the channel first and so never pumps it. + * DoPacket() skips the inbound dispatch too, for every message but a + * DISCONNECT, so nothing arriving afterward is buffered, answered or + * reported through the channel callbacks. */ byte disconnected; /* Set once SendDisconnect() has bundled our own DISCONNECT into the * output buffer, and cleared once wolfSSH_SendPacket() drains it, so it diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 7f5c6c0b7..da09504f9 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -563,14 +563,17 @@ WOLFSSH_API int wolfSSH_connect(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_shutdown(WOLFSSH* ssh); /* A disconnect, sent or received, ends the session. Nothing more goes out: * wolfSSH_shutdown() above this comment, and every send call below it, - * report WS_DISCONNECT from then on, as do wolfSSH_accept() and - * wolfSSH_connect(). Neither driver flushes a disconnect of ours left queued - * by a short send; wolfSSH_shutdown() or another wolfSSH_SendDisconnect() - * owns that. Reads are not gated, so channel data that - * arrived before the disconnect can still be drained; wolfSSH_stream_read() - * and wolfSSH_stream_peek() report WS_DISCONNECT once their buffer runs - * dry. A CHANNEL_EOF already received outranks that drain: both report - * WS_EOF with data possibly still buffered. RFC 4253 section 11.1. */ + * report WS_DISCONNECT from then on, as do wolfSSH_accept(), + * wolfSSH_connect() and wolfSSH_worker(). None of the three flushes a + * disconnect of ours left queued by a short send; wolfSSH_shutdown() or + * another wolfSSH_SendDisconnect() owns that. Inbound traffic is dropped: + * the receive path skips every message but a DISCONNECT, so late channel + * data is discarded and the channel callbacks stop firing. Reads are not + * gated, so channel data that arrived before the disconnect can still be + * drained; wolfSSH_stream_read() and wolfSSH_stream_peek() report + * WS_DISCONNECT once their buffer runs dry. A CHANNEL_EOF already received + * outranks that drain: both report WS_EOF with data possibly still + * buffered. RFC 4253 section 11.1. */ WOLFSSH_API int wolfSSH_stream_peek(WOLFSSH* ssh, byte* buf, word32 bufSz); /* Returns the bytes read; the next read clears the status. WS_WANT_WRITE * from wolfSSH_get_error() means the adjust is queued; it goes out on the