Skip to content

Extended (stderr) data: accumulation and window flow control #1054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ejohnstown wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Extended (stderr) data: accumulation and window flow control #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d612c49
Accumulate extended data, ignore unknown types
ejohnstown Jun 23, 2026
cbf4c37
Window flow control for extended data
ejohnstown Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 147 additions & 26 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6480,6 +6480,56 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
}


/* Flush any receive-window credit that was deferred while a rekey was in
* progress. DoChannelExtendedData() parks the credit for discarded extended
* data on the channel (pendingWindowAdjust) instead of sending a WINDOW_ADJUST,
* because RFC 4253 section 7.1 forbids connection-layer messages between KEXINIT
* and NEWKEYS and IsMessageAllowed() does not gate connection-layer messages on
* isKeying. This is called when keying completes. */
static int SendPendingChannelWindowAdjust(WOLFSSH* ssh)
{
WOLFSSH_CHANNEL* cur;
int savedError;

if (ssh == NULL)
return WS_BAD_ARGUMENT;

/* Still mid-KEX: leave the credit parked until both sides finish. */
if (ssh->isKeying)
return WS_SUCCESS;

/* Flushing parked credit is incidental to the keying flow that triggered
* it, so do not disturb ssh->error: SendChannelWindowAdjust ->
* wolfSSH_SendPacket sets it to WS_WANT_WRITE when the bundled adjust
* cannot flush in non-blocking mode, and the bundled bytes are sent on the
* next flush regardless. Mirror wolfSSH_extended_data_read(). */
savedError = ssh->error;

for (cur = ssh->channelList; cur != NULL; cur = cur->next) {
word32 amount = cur->pendingWindowAdjust;
int adjustResult;

if (amount == 0)
continue;

/* Clear before sending: the credit is owed regardless of the send
* result. On WS_WANT_WRITE the adjust is bundled into outputBuffer and
* reaches the peer on a later flush; on a hard failure the channel is
* already failing, so dropping the deferred credit is moot. */
cur->pendingWindowAdjust = 0;
adjustResult = SendChannelWindowAdjust(ssh, cur->channel, amount);
if (adjustResult != WS_SUCCESS && adjustResult != WS_WANT_WRITE)
WLOG(WS_LOG_ERROR,
"SendPendingChannelWindowAdjust: window adjust send failed "
"(%d) for channel %u; deferred credit dropped",
adjustResult, cur->channel);
}

ssh->error = savedError;
return WS_SUCCESS;
}


static int DoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
{
int ret = WS_SUCCESS;
Expand Down Expand Up @@ -6565,6 +6615,9 @@ static int DoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
ssh->isKeying &= ~WOLFSSH_PEER_IS_KEYING;
HandshakeInfoFree(ssh->handshake, ssh->ctx->heap);
ssh->handshake = NULL;
/* If this was the last keying flag to clear, flush any window credit
* that was deferred during the rekey (RFC 4253 section 7.1). */
SendPendingChannelWindowAdjust(ssh);
WLOG(WS_LOG_DEBUG, "Keying completed");
if (ssh->ctx->keyingCompletionCb)
ssh->ctx->keyingCompletionCb(ssh->keyingCompletionCtx);
Expand Down Expand Up @@ -10165,25 +10218,25 @@ static int DoChannelData(WOLFSSH* ssh,
}


/* deletes current buffer and updates it
* return WS_SUCCESS on success */
/* Appends data to the buffer, preserving any bytes not yet read. This is an
* accumulating buffer: unread data is kept, already-read data (everything
* below idx) is compacted away, and the new data is appended after the unread
* region. Returns WS_SUCCESS on success. */
static int PutBuffer(WOLFSSH_BUFFER* buf, byte* data, word32 dataSz)
{
int ret;

/* reset "used" section of buffer back to 0 */
buf->length = 0;
buf->idx = 0;

if (dataSz > buf->bufferSz) {
if ((ret = GrowBuffer(buf, dataSz)) != WS_SUCCESS) {
return ret;
}
/* GrowBuffer compacts the consumed prefix (bytes below idx) down to the
* front and guarantees room for dataSz more bytes after the unread data,
* reallocating only when needed. On return idx is 0 and length is the
* count of unread bytes. */
ret = GrowBuffer(buf, dataSz);
if (ret == WS_SUCCESS) {
WMEMCPY(buf->buffer + buf->length, data, dataSz);
buf->length += dataSz;
}
WMEMCPY(buf->buffer, data, dataSz);
buf->length = dataSz;

return WS_SUCCESS;
return ret;
}


Expand All @@ -10202,9 +10255,6 @@ static int DoChannelExtendedData(WOLFSSH* ssh,
ret = GetUint32(&channelId, buf, len, &begin);
if (ret == WS_SUCCESS)
ret = GetUint32(&dataTypeCode, buf, len, &begin);
if (ret == WS_SUCCESS)
ret = (dataTypeCode == CHANNEL_EXTENDED_DATA_STDERR) ?
WS_SUCCESS : WS_INVALID_EXTDATA;
if (ret == WS_SUCCESS)
ret = GetSize(&dataSz, buf, len, &begin);

Expand All @@ -10214,23 +10264,85 @@ static int DoChannelExtendedData(WOLFSSH* ssh,
ret = WS_INVALID_CHANID;
else if (dataSz > channel->maxPacketSz)
ret = WS_RECV_OVERFLOW_E;
else if (dataSz > channel->windowSz)
/* Peer sent more than the window we advertised. */
ret = WS_RECV_OVERFLOW_E;
else if (dataTypeCode == CHANNEL_EXTENDED_DATA_STDERR) {
/* Buffer the stderr data for the application to read. The
* extended data buffer accumulates unread data instead of
* overwriting it, so a blob arriving before the application
* reads the previous one is no longer silently lost.
*
* Extended data consumes channel window the same as ordinary
* data (RFC 4254 section 5.2). Charge the window now and only
* replenish it once the application drains the data in
* wolfSSH_extended_data_read(). That gives stderr real
* back-pressure instead of replenishing on receipt, which
* previously let the peer keep clobbering the buffer. */
int hadPending = ssh->extDataBuffer.idx < ssh->extDataBuffer.length;

if (hadPending && ssh->extDataChannelId != channel->channel) {
/* Buffer and extDataChannelId are shared (single-session
* limitation, see WOLFSSH.extDataBuffer in internal.h), so only
* one channel's stderr buffers at a time. Buffering a second
* would desync windows (displaced channel never credited, this
* one over-credited). Fail safe: drop it and replenish its
* window (now, or deferred past a rekey) so nothing leaks. */
WLOG(WS_LOG_ERROR,
"Extended data from channel %u dropped; channel %u still "
"has unread stderr buffered (only one channel's stderr "
"can be buffered at a time)",
channel->channel, ssh->extDataChannelId);
if (ssh->isKeying) {

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 [High] Deferred discarded extended data is not charged against the channel receive window during rekey
🚫 BLOCK

The new rekey deferral path accepts discarded extended data while ssh->isKeying by adding peer-controlled dataSz to channel->pendingWindowAdjust, but it leaves channel->windowSz unchanged. The only validation is per-packet (dataSz <= channel->maxPacketSz and dataSz <= channel->windowSz), so it never accounts for cumulative bytes received while window adjustments are deferred. A malicious authenticated peer can send repeated unknown extended-data packets, or repeated second-channel stderr packets that are dropped, each individually within the unchanged window, and this code accepts more bytes than were advertised. When keying completes, SendPendingChannelWindowAdjust() flushes the accumulated credit, potentially over-crediting the peer and wrapping the unchecked word32 pendingWindowAdjust += dataSz. The buffered stderr path correctly subtracts from windowSz, but the discarded-data rekey branches introduced by this PR do not. Data flow: encrypted peer packet -> DoPacket() -> DoChannelExtendedData() -> GetSize() -> per-packet dataSz checks -> uncharged pendingWindowAdjust accumulation. Impact is connection-level resource / flow-control DoS, not memory corruption. Severity views differ across modes: the review mode rated this High/BLOCK (cumulative window bypass + credit wrap); the security mode rated it Low (DoS only). The stricter High assessment is retained.

Recommendation: Charge the receive window whenever payload bytes are accepted from the peer, even when discarded, and guard the deferred-credit addition against overflow, e.g. if (dataSz > UINT32_MAX - channel->pendingWindowAdjust) ret = WS_OVERFLOW_E; else { channel->windowSz -= dataSz; channel->pendingWindowAdjust += dataSz; ret = WS_SUCCESS; }. In SendPendingChannelWindowAdjust(), restore windowSz by amount (with overflow check) after SendChannelWindowAdjust() returns WS_SUCCESS/WS_WANT_WRITE. Extend test_DoChannelExtendedData_keying() to cover cumulative deferred data and pendingWindowAdjust overflow boundaries.

/* RFC 4253 section 7.1: no WINDOW_ADJUST between KEXINIT
* and NEWKEYS. Park the credit; flushed when keying ends. */
channel->pendingWindowAdjust += dataSz;
ssh->lastRxId = channelId;
ret = WS_SUCCESS;
}
else {
ret = SendChannelWindowAdjust(ssh, channel->channel,
dataSz);
if (ret == WS_SUCCESS)
ssh->lastRxId = channelId;
}
}
else {
ret = PutBuffer(&ssh->extDataBuffer, buf + begin, dataSz);
#ifdef DEBUG_WOLFSSH
DumpOctetString(buf + begin, dataSz);
#endif
if (ret == WS_SUCCESS) {
channel->windowSz -= dataSz;
ssh->extDataChannelId = channel->channel;
ssh->lastRxId = channelId;
ret = WS_EXTDATA;
}
}
}
else {
ret = PutBuffer(&ssh->extDataBuffer, buf + begin, dataSz);
#ifdef DEBUG_WOLFSSH
DumpOctetString(buf + begin, dataSz);
#endif
if (ret == WS_SUCCESS) {
/* Unknown extended data type. RFC 4254/4251 call for unknown
* values to be ignored rather than rejected, so consume and
* discard the payload. Nothing is buffered for the application,
* so replenish the window (now, or deferred past a rekey). */
WLOG(WS_LOG_INFO, "Ignoring unknown extended data type %u",
dataTypeCode);
if (ssh->isKeying) {
/* RFC 4253 section 7.1: no WINDOW_ADJUST between KEXINIT and
* NEWKEYS. Park the credit; flushed when keying ends. */
channel->pendingWindowAdjust += dataSz;
ssh->lastRxId = channelId;
Comment thread
ejohnstown marked this conversation as resolved.
ret = WS_SUCCESS;
}
else {
ret = SendChannelWindowAdjust(ssh, channel->channel, dataSz);
if (ret == WS_SUCCESS)
ssh->lastRxId = channelId;
}
}
*idx = begin + dataSz;
}

if (ret == WS_SUCCESS) {
ssh->lastRxId = channelId;
ret = WS_EXTDATA;
}

WLOG(WS_LOG_DEBUG, "Leaving DoChannelExtendedData(), ret = %d", ret);
return ret;
}
Expand Down Expand Up @@ -13724,6 +13836,10 @@ int SendNewKeys(WOLFSSH* ssh)

/* Clear self is keying flag */
ssh->isKeying &= ~WOLFSSH_SELF_IS_KEYING;

/* If this was the last keying flag to clear, flush any window credit
* that was deferred during the rekey (RFC 4253 section 7.1). */
SendPendingChannelWindowAdjust(ssh);
}

WLOG(WS_LOG_DEBUG, "Leaving SendNewKeys(), ret = %d", ret);
Expand Down Expand Up @@ -18502,6 +18618,11 @@ int wolfSSH_TestDoChannelExtendedData(WOLFSSH* ssh, byte* buf, word32 len,
return DoChannelExtendedData(ssh, buf, len, idx);
}

int wolfSSH_TestSendPendingChannelWindowAdjust(WOLFSSH* ssh)
{
return SendPendingChannelWindowAdjust(ssh);
}

int wolfSSH_TestDoUserAuthRequest(WOLFSSH* ssh, byte* buf, word32 len,
word32* idx)
{
Expand Down
61 changes: 58 additions & 3 deletions src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1404,9 +1404,14 @@ int wolfSSH_extended_data_send(WOLFSSH* ssh, byte* buf, word32 bufSz)
}


/* Reads pending data from extended data buffer. Currently can be used to get
* STDERR information sent across the channel.
* Returns the number of bytes read on success */
/* Reads pending STDERR data from the extended data buffer. Returns bytes read
* (>= 0) or a negative WS_ error. See wolfssh/ssh.h for the public contract
* (callers MUST drain stderr; WS_REKEYING during key exchange).
*
* A successful read drains the buffer and sends a window adjust for the bytes
* consumed, releasing back-pressure. The read still succeeds if that send
* cannot complete: WS_WANT_WRITE is absorbed (adjust stays queued) and a hard
* failure is logged, not returned. ssh->error is saved/restored across it. */
int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
{
byte* buf;
Expand All @@ -1416,6 +1421,14 @@ int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
return WS_BAD_ARGUMENT;
}

/* A read sends a window adjust, which must not go out between KEXINIT and
* NEWKEYS. IsMessageAllowed() gates only transport messages on isKeying,
* not connection-layer ones, so gate here like wolfSSH_stream_read(). */
if (ssh->isKeying) {
ssh->error = WS_REKEYING;
return WS_REKEYING;
}

/* sanity check to make sure idx is not in a bad state */
if (ssh->extDataBuffer.idx > ssh->extDataBuffer.length) {
WLOG(WS_LOG_ERROR, "Bad internal state for buffer index");
Expand All @@ -1425,6 +1438,48 @@ int wolfSSH_extended_data_read(WOLFSSH* ssh, byte* out, word32 outSz)
buf = ssh->extDataBuffer.buffer + ssh->extDataBuffer.idx;
WMEMCPY(out, buf, bufSz);
ssh->extDataBuffer.idx += bufSz;

/* Replenish the channel window for the bytes just consumed. The window
* was charged when the extended data arrived (DoChannelExtendedData), so
* draining it here is what releases back-pressure to the peer. If the
* channel is gone the data is still returned; there is just no window to
* adjust. */
if (bufSz > 0) {
WOLFSSH_CHANNEL* channel;

channel = ChannelFind(ssh, ssh->extDataChannelId, WS_CHANNEL_ID_SELF);
if (channel != NULL) {
int adjustResult;
int savedError = ssh->error;

/* Credit the window locally regardless of send result, mirroring
* _UpdateChannelWindow(). On WS_WANT_WRITE the adjust is already
* bundled into outputBuffer and the peer is credited on a later
* flush; on a hard failure the channel is already failing, so a
* transient local over-advertisement is moot and crediting avoids
* permanently leaking the window for the bytes just consumed.
*
* The bytes are already in out, so this is a completed read: it
* reports the byte count and must not disturb ssh->error. Stomping
* ssh->error here would be invisible to callers that only check the
* return value and surprising to callers that inspect
* wolfSSH_get_error() after a successful read. SendChannelWindow-
* Adjust -> wolfSSH_SendPacket sets ssh->error = WS_WANT_WRITE when
* the bundled adjust cannot be flushed in non-blocking mode, so
* save ssh->error across the send and restore it; a hard send
* failure is logged, not returned. */
adjustResult = SendChannelWindowAdjust(ssh, channel->channel,

@aidangarske aidangarske Jul 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 [Low] Extended-data read hides nonblocking WINDOW_ADJUST back-pressure
💡 SUGGEST

The PR moves stderr receive-window replenishment from receipt time to wolfSSH_extended_data_read(), but then treats WS_WANT_WRITE from SendChannelWindowAdjust() as an invisible success. In nonblocking mode the adjust may be bundled in outputBuffer but not flushed; the function restores ssh->error and returns a byte count, so callers are not told to wait for writability or call a flushing path. Read-only loops then keep waiting for inbound data while the peer never receives the window credit it needs to send more, creating a remote-triggerable channel stall. Data flow: peer sends stderr -> DoChannelExtendedData() buffers it and decrements windowSz -> application calls wolfSSH_extended_data_read() -> SendChannelWindowAdjust() returns WS_WANT_WRITE -> error restored and byte count returned -> pending adjust may remain unsent. Impact is session/channel DoS.

Recommendation: Do not silently hide WS_WANT_WRITE for the window-adjust path. Mirror wolfSSH_stream_read() semantics: only advance extDataBuffer.idx and credit channel->windowSz after the adjust is queued/flushed successfully, and return WS_WANT_WRITE with ssh->error preserved when the caller must service the writable socket. Alternatively, flush pending output before blocking and preserve a visible writable-wakeup signal for callers.

bufSz);
channel->windowSz += bufSz;
ssh->error = savedError;
if (adjustResult != WS_SUCCESS && adjustResult != WS_WANT_WRITE)
WLOG(WS_LOG_ERROR,
"wolfSSH_extended_data_read: window adjust send failed "
"(%d); window credited locally, read still succeeded",
adjustResult);
}
}

return bufSz;
}

Expand Down
Loading
Loading