Extended (stderr) data: accumulation and window flow control#1054
Extended (stderr) data: accumulation and window flow control#1054ejohnstown wants to merge 2 commits into
Conversation
- PutBuffer appends instead of resetting, preserving unread stderr across multiple packets. - Ignore unknown EXTENDED_DATA type codes per RFC instead of returning WS_INVALID_EXTDATA. Issues: F-864, F-2078
There was a problem hiding this comment.
Pull request overview
This PR updates wolfSSH’s handling of SSH_MSG_CHANNEL_EXTENDED_DATA (stderr) to (1) accumulate unread extended data instead of overwriting it, (2) ignore unknown extended-data type codes per RFC behavior, and (3) apply channel window back-pressure semantics for stderr by charging the receive window on receipt and replenishing it only when the application drains the buffered stderr.
Changes:
- Accumulate buffered stderr across multiple packets and preserve unread data ordering.
- Implement receive-window accounting for stderr (charge on receipt; replenish on
wolfSSH_extended_data_read()), including a documented single-session limitation for multi-channel stderr buffering. - Add unit test coverage for accumulation, window back-pressure, overflow rejection, unknown-type ignore, and cross-channel fail-safe behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/ssh.h | Documents the public contract for draining stderr and the single-channel buffering limitation. |
| wolfssh/internal.h | Adds internal tracking (extDataChannelId) and documents shared-buffer/window semantics. |
| wolfssh/error.h | Clarifies WS_INVALID_EXTDATA is retained for compatibility but no longer produced. |
| src/internal.c | Changes stderr buffering to append; adds window charging and unknown-type ignore behavior. |
| src/ssh.c | Updates wolfSSH_extended_data_read() to replenish window on drain and gate sends during KEX. |
| tests/unit.c | Adds a comprehensive unit test for extended-data accumulation + window flow control. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Charge the channel window on receipt; replenish only when the application consumes bytes via wolfSSH_extended_data_read. - Track extDataChannelId so the read path adjusts the right window. - Reject extended data larger than the advertised window. - stderr now shares the channel window with normal data (RFC 4254 section 5.2 back-pressure). - Gate window adjusts on isKeying both ways: IsMessageAllowed does not gate connection-layer messages, so they must not go out between KEXINIT and NEWKEYS (RFC 4253 section 7.1). The read path returns WS_REKEYING; the receive path parks owed credit in pendingWindowAdjust, flushed by SendPendingChannelWindowAdjust once keying completes. - Fail safe when a second channel sends stderr while another still has unread data: drop it and replenish its window instead of leaking the displaced channel's window and over-crediting the survivor. - Document the mandatory-drain contract in the public header. Issue: F-864
aidangarske
left a comment
aidangarske
left a comment
There was a problem hiding this comment.
🐺 Skoll Code Review
Overall recommendation: REQUEST_CHANGES
Findings: 2 total — 2 posted, 0 skipped
Posted findings
- [High] Deferred discarded extended data is not charged against the channel receive window during rekey —
src/internal.c:10296-10334 - [Low] Extended-data read hides nonblocking WINDOW_ADJUST back-pressure —
src/ssh.c:1471-1475
Review generated by Skoll.
| "has unread stderr buffered (only one channel's stderr " | ||
| "can be buffered at a time)", | ||
| channel->channel, ssh->extDataChannelId); | ||
| if (ssh->isKeying) { |
There was a problem hiding this comment.
🟠 [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.
| * 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, |
There was a problem hiding this comment.
🔵 [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.
Issues: F-864, F-2078