From fb3e08b11395901bd6d8070bdd3df6c332c0518f Mon Sep 17 00:00:00 2001 From: John Safranek Date: Wed, 28 Oct 2026 16:05:33 -0700 Subject: [PATCH] ssh: stop reading behind a large output backlog wolfSSH_worker() and wolfSSH_stream_read() now flush before reading once more than WOLFSSH_MAX_OUTPUT_BACKLOG bytes are queued, and return WS_WANT_WRITE without reading if the flush leaves it over. A peer that never reads can no longer grow outputBuffer one reply per packet. - WOLFSSH_MAX_OUTPUT_BACKLOG defaults to two MAX_PACKET_SZ, overridable - A smaller backlog still reads first, keeping the PR 856 order - regress: unknown-id and GLOBAL_REQUEST floods with blocked sends stop at the cap and drain once the peer reads Issue: F-16808 --- src/ssh.c | 53 ++++++++++++++ tests/regress.c | 170 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 12 ++++ 3 files changed, 235 insertions(+) diff --git a/src/ssh.c b/src/ssh.c index 09bf297fe..dac5e98b4 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1226,6 +1226,46 @@ static int FlushQueuedOutput(WOLFSSH* ssh) } +/* Back-pressure on the receive path. Every packet read can queue a reply + * (UNIMPLEMENTED, a GLOBAL_REQUEST or CHANNEL_REQUEST answer, a window + * adjust), and a peer that never reads its socket would otherwise grow the + * output buffer without bound, re-copying it on every append. Once more + * than WOLFSSH_MAX_OUTPUT_BACKLOG bytes are queued, flush, and if that + * leaves the backlog over the cap, take no more input. A smaller backlog + * does not stop the read: two peers that both write before they read must + * not wait on each other, and PR 856 needs the read to fetch the window + * adjust a stalled send is waiting for. + * + * Returns WS_SUCCESS when the caller may read. Otherwise the flush status, + * WS_WANT_WRITE when the transport would block, with ssh->error set to + * match. It reports WS_WANT_WRITE only off a send that just blocked. Call + * only after a NULL check of ssh. */ +static int HoldInputForBacklog(WOLFSSH* ssh) +{ + int ret = WS_SUCCESS; + + if (!ssh->disconnected && ssh->outputBuffer.length > ssh->outputBuffer.idx + && ssh->outputBuffer.length - ssh->outputBuffer.idx + > WOLFSSH_MAX_OUTPUT_BACKLOG) { + ret = wolfSSH_SendPacket(ssh); + if (ret == WS_WANT_WRITE + && ssh->outputBuffer.length - ssh->outputBuffer.idx + <= WOLFSSH_MAX_OUTPUT_BACKLOG) { + /* Drained enough to read again; the caller flushes the rest. */ + ret = WS_SUCCESS; + } + if (ret != WS_SUCCESS) { + WLOG(WS_LOG_DEBUG, "Output backlog of %u over the cap, " + "not reading, ret = %d", + ssh->outputBuffer.length - ssh->outputBuffer.idx, ret); + ssh->error = ret; + } + } + + return ret; +} + + int wolfSSH_shutdown(WOLFSSH* ssh) { int ret = WS_SUCCESS; @@ -1494,6 +1534,9 @@ int wolfSSH_stream_read(WOLFSSH* ssh, byte* buf, word32 bufSz) WLOG(WS_LOG_DEBUG, "Starting to receive data at current index of %u", inputBuffer->idx); + ret = HoldInputForBacklog(ssh); + if (ret != WS_SUCCESS) + break; ret = DoReceive(ssh); /* Off the current head: DoReceive() may have retired the old. */ if (ssh->channelList == NULL @@ -4694,6 +4737,16 @@ int wolfSSH_worker(WOLFSSH* ssh, word32* channelId) return WS_FATAL_ERROR; } + /* Unless too much is queued, which has just been flushed as far as the + * transport takes it. */ + if (ret == WS_SUCCESS) { + ret = HoldInputForBacklog(ssh); + if (ret != WS_SUCCESS) { + WLOG(WS_LOG_DEBUG, "Leaving wolfSSH_worker(), ret = %d", ret); + return ret; + } + } + /* Always service inbound data first so window updates can unblock sends. */ if (ret == WS_SUCCESS) { ret = DoReceive(ssh); diff --git a/tests/regress.c b/tests/regress.c index 658405cfb..018383fdf 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -532,6 +532,18 @@ static byte ReplyToPacket(ChannelOpenHarness* harness, byte* in, harness->io.outSz); } +/* F-16808 back-pressure, shared by the unknown-id and GLOBAL_REQUEST + * tests. Once the library + * stops reading at WOLFSSH_MAX_OUTPUT_BACKLOG queued bytes, the last packet + * it read can add one more reply on top, and no reply outgrows a packet. */ +#define REGRESS_BACKLOG_LIMIT (WOLFSSH_MAX_OUTPUT_BACKLOG + MAX_PACKET_SZ) +#define REGRESS_BACKLOG_PKT_SZ 16U +/* Every reply is at least a 16-byte packet, so if nothing held the input + * this many replies would take the backlog past the limit. */ +#define REGRESS_BACKLOG_PKTS \ + ((REGRESS_BACKLOG_LIMIT / REGRESS_BACKLOG_PKT_SZ) + 1024U) +#define REGRESS_BACKLOG_UNIMPL_MSGID 15 /* unassigned transport id */ + /* The tests below drive a server-side session. With NO_WOLFSSH_SERVER the * message filter has no server branch, so every message on such a session is * refused and those tests cannot run. */ @@ -561,6 +573,57 @@ static void InitChannelOpenHarness(ChannelOpenHarness* harness, } +/* F-16808, the unknown-message-id variant: every unassigned id the server is + * fed is answered with SSH_MSG_UNIMPLEMENTED (RFC 4253 section 11.4). With + * the transport reporting would-block on every send and the application + * calling wolfSSH_worker() again on WS_WANT_WRITE, each pass used to consume + * one packet and append one more UNIMPLEMENTED to the unsent backlog without + * bound. Plaintext harness, server side, past userauth. */ +static void TestOutputBacklogBoundedUnimplemented(void) +{ + ChannelOpenHarness harness; + byte* in; + word32 inSz = 0; + word32 i; + word32 pending; + word32 maxPending = 0; + int ret; + int err; + + in = (byte*)WMALLOC(REGRESS_BACKLOG_PKTS * REGRESS_BACKLOG_PKT_SZ, NULL, + DYNTYPE_BUFFER); + AssertNotNull(in); + for (i = 0; i < REGRESS_BACKLOG_PKTS; i++) { + inSz += BuildPacket(REGRESS_BACKLOG_UNIMPL_MSGID, in + inSz, + REGRESS_BACKLOG_PKT_SZ); + } + + InitChannelOpenHarness(&harness, in, inSz); + harness.io.blockAll = 1; + + for (i = 0; i < REGRESS_BACKLOG_PKTS; i++) { + ret = wolfSSH_worker(harness.ssh, NULL); + err = wolfSSH_get_error(harness.ssh); + + pending = harness.ssh->outputBuffer.length + - harness.ssh->outputBuffer.idx; + if (pending > maxPending) + maxPending = pending; + + AssertFalse(harness.ssh->disconnected); + AssertIntEQ(ret, WS_WANT_WRITE); + AssertIntEQ(err, WS_WANT_WRITE); + } + + /* The cap held, and input stopped being taken to hold it. */ + AssertTrue(maxPending > WOLFSSH_MAX_OUTPUT_BACKLOG); + AssertTrue(maxPending <= REGRESS_BACKLOG_LIMIT); + AssertTrue(harness.io.inOff < harness.io.inSz); + + FreeChannelOpenHarness(&harness); + WFREE(in, NULL, DYNTYPE_BUFFER); +} + #ifdef WOLFSSH_KEYBOARD_INTERACTIVE /* Build a plaintext SSH_MSG_USERAUTH_INFO_RESPONSE. The wire response count is * "responseCount"; "stringCount" "x" response strings are actually appended. @@ -840,6 +903,8 @@ typedef struct DuplexEndpoint { byte sentStrictKexMarker; byte sentExtInfoMarker; byte failSendMsgId; /* nonzero: fail the send of a packet with this id */ + byte blockSend; /* nonzero: every send reports a would-block */ + byte shortSend; /* nonzero: send what fits the peer's queue */ } DuplexEndpoint; typedef struct { @@ -1541,6 +1606,25 @@ static int DuplexSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx) return WS_CBIO_ERR_GENERAL; } + /* A peer that has stopped reading: nothing this side sends leaves. */ + if (endpoint->blockSend) { + return WS_CBIO_ERR_WANT_WRITE; + } + + /* A slow reader: a short send, or a would-block once its queue is full, + * instead of failing the send. */ + if (endpoint->shortSend) { + word32 room = REGRESS_DUPLEX_QUEUE_SZ - endpoint->peer->inbound.len; + + if (room == 0) { + return WS_CBIO_ERR_WANT_WRITE; + } + if (outputSz > room) { + outputSz = room; + sz = room; + } + } + if (endpoint->failSendMsgId != 0 && outputSz > UINT32_SZ + PAD_LENGTH_SZ && output[UINT32_SZ + PAD_LENGTH_SZ] == endpoint->failSendMsgId) { @@ -2294,6 +2378,90 @@ static void TestKexSecretWipedOnServerSendFail(void) FreeKexReplyHarness(&harness); } +/* F-16808: an established session must not keep consuming reply-generating + * input while earlier output is still queued. The client below keeps sending + * GLOBAL_REQUEST want_reply over the real encrypted, authenticated session + * while the server's transport reports would-block on every send, and the + * server application keeps turning wolfSSH_worker() the way a non-blocking + * event loop does on WS_WANT_WRITE. Each pass used to consume one request + * and queue its REQUEST_FAILURE behind the ones that never left, so + * ssh->outputBuffer grew by one reply per request. Once the transport + * drains, the held requests must still be answered. */ +static void TestOutputBacklogBoundedGlobalRequest(void) +{ + static const char reqName[] = "keepalive@openssh.com"; + KexReplyHarness harness; + KexReplyRunResult result; + word32 i; + word32 held = 0; + word32 pending; + word32 maxPending = 0; + int ret; + int err; + + InitKexReplyHarness(&harness, REGRESS_DEFAULT_KEY_ALGO, + REGRESS_DEFAULT_KEY_PATH, 0, NULL); + RunKexReplyHandshake(&harness, &result); + AssertTrue(result.clientSuccess); + AssertTrue(result.serverSuccess); + AssertIntEQ(harness.server->acceptState, + ACCEPT_CLIENT_SESSION_ESTABLISHED); + AssertFalse(wolfSSH_OutputPending(harness.server)); + AssertIntEQ(harness.serverIo.inbound.len, 0); + + /* The client stops reading. */ + harness.serverIo.blockSend = 1; + + for (i = 0; i < REGRESS_BACKLOG_PKTS; i++) { + AssertIntEQ(SendGlobalRequest(harness.client, + (const byte*)reqName, (word32)WSTRLEN(reqName), 1, NULL), + WS_SUCCESS); + + ret = wolfSSH_worker(harness.server, NULL); + err = wolfSSH_get_error(harness.server); + if (harness.serverIo.inbound.len > 0) + held++; + + pending = harness.server->outputBuffer.length + - harness.server->outputBuffer.idx; + if (pending > maxPending) + maxPending = pending; + + /* The session stays up, the would-block is all the app sees. */ + AssertIntEQ(ret, WS_WANT_WRITE); + AssertIntEQ(err, WS_WANT_WRITE); + AssertFalse(harness.server->disconnected); + + /* Stop before the duplex queue to the server fills. */ + if (harness.serverIo.inbound.len > REGRESS_DUPLEX_QUEUE_SZ / 2) + break; + } + + /* The cap held, and input stopped being taken to hold it. */ + AssertTrue(maxPending > WOLFSSH_MAX_OUTPUT_BACKLOG); + AssertTrue(maxPending <= REGRESS_BACKLOG_LIMIT); + AssertTrue(held > 0); + + /* The client reads again: the backlog drains and the held requests are + * answered, with no stall. */ + harness.serverIo.blockSend = 0; + harness.serverIo.shortSend = 1; + for (i = 0; i < REGRESS_BACKLOG_PKTS * 2U; i++) { + if (harness.serverIo.inbound.len == 0 + && !wolfSSH_OutputPending(harness.server) + && harness.clientIo.inbound.len == 0) + break; + (void)wolfSSH_worker(harness.server, NULL); + AssertFalse(harness.server->disconnected); + (void)wolfSSH_worker(harness.client, NULL); + AssertFalse(harness.client->disconnected); + } + AssertIntEQ(harness.serverIo.inbound.len, 0); + AssertFalse(wolfSSH_OutputPending(harness.server)); + + FreeKexReplyHarness(&harness); +} + #ifndef WOLFSSH_NO_RSA_SHA2_256 static void TestKexDhReplyRejectsRsaSha2_256CorruptSig(void) { @@ -21491,6 +21659,7 @@ int main(int argc, char** argv) TestSameUserRetryAllowed(); TestUserAuthBeforeServiceRequestDisconnects(); TestUserAuthPipelinedAfterServiceRequest(); + TestOutputBacklogBoundedUnimplemented(); #ifdef WOLFSSH_KEYBOARD_INTERACTIVE TestKbInfoResponseCountMismatchSendsFailure(); TestKbInfoResponseMismatchKeepsFraming(); @@ -21794,6 +21963,7 @@ int main(int argc, char** argv) TestKexDhReplyRejectsSigNameOverrun(); TestKexSecretWipedPerKex(); TestKexSecretWipedOnServerSendFail(); + TestOutputBacklogBoundedGlobalRequest(); #ifdef REGRESS_TRUNC_KEX_ALGO TestKexDhReplyTruncatedFSendsDisconnect(); TestKexDhInitTruncatedESendsDisconnect(); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index d9c550995..80e51f823 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -870,6 +870,18 @@ WOLFSSH_LOCAL int CheckAlgoList(const char* list, byte type); #error "DEFAULT_MAX_PACKET_SZ too large to frame inside MAX_PACKET_SZ" #endif +#ifndef WOLFSSH_MAX_OUTPUT_BACKLOG + /* Unsent bytes in the output buffer past which the session stops + * taking input until the transport drains. A data send never stacks + * a second packet behind an unsent one, so normal traffic queues at + * most one full packet plus the small replies the receive path adds; + * this leaves room for a full packet of each. CVE-2025-26466 pattern. */ + #define WOLFSSH_MAX_OUTPUT_BACKLOG (MAX_PACKET_SZ * 2) +#endif +#if WOLFSSH_MAX_OUTPUT_BACKLOG < MAX_PACKET_SZ + #error "WOLFSSH_MAX_OUTPUT_BACKLOG must hold at least one MAX_PACKET_SZ" +#endif + typedef struct WOLFSSH_BUFFER { void* heap; /* Heap for allocations */