Extended (stderr) data: buffering and flow control

[ejohnstown]

• Ignore unknown extended data types: consume, log, and window-credit them per RFC 4254/4251 instead of rejecting with WS_INVALID_EXTDATA.
• Per-channel buffering: move extDataBuffer to WOLFSSH_CHANNEL; accumulate by appending so unread data survives across packets.
• Window flow control (RFC 4254 5.2): charge on receipt, credit on drain, reject data larger than the advertised window.
• Rekey-safe crediting (RFC 4253 7.1): ChannelCreditWindow() parks credit that can't go out mid-KEX or after a failed send; flushed when keying completes. Reads are not blocked during rekey.
• Multi-channel: stream_/extended_data_ read serves only the first channel; other channels use new API wolfSSH_Channel{,Id}{Read,Send}Ext.
• SCP client: drain stderr in a loop, not one chunk.
• Document the mandatory-drain contract in the public header.

Issues: F-864, F-2078

[Copilot]

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.

[aidangarske]

🐺 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.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[aidangarske]

Skoll Multi-Scan Review

Modes: review + review-security + bugsOverall recommendation: REQUEST_CHANGES
Findings: 5 total — 5 posted, 0 skipped
4 finding(s) posted as inline comments (see file-level comments below)
1 finding(s) not tied to a diff line (full detail below)

Posted findings

• [High] [review+review-security+bugs] wolfSSH_worker masks extended-data events during rekey — src/ssh.c:2860-2867
• [High] [review+review-security] Parked receive-window credit is counted as advertised before the peer is credited — src/ssh.c:3196-3204
• [Medium] [bugs] Ignored extended-data credit errors are reported as success — src/internal.c:11336-11340
• [Low] stream_read returns WS_ERROR for non-head extended data but leaves ssh-error == WS_EXTDATA — src/ssh.c:1257-1266

Findings not tied to a diff line

wolfSSH_extended_data_read now rejects outSz==0 with WS_BAD_ARGUMENT (behavior change)

File: src/ssh.c:1456-1470 (via _ChannelReadExt guard at src/ssh.c:3176)
Function: wolfSSH_extended_data_read / _ChannelReadExt
Severity: Low

The rewritten wolfSSH_extended_data_read() delegates to _ChannelReadExt(), whose first guard is if (channel == NULL || buf == NULL || bufSz == 0) return WS_BAD_ARGUMENT;. The previous implementation computed bufSz = min(outSz, length - idx) and, for outSz == 0, copied 0 bytes and returned 0. So a caller that passes outSz == 0 now gets a negative error code (WS_BAD_ARGUMENT) instead of 0. A drain loop written as while ((n = wolfSSH_extended_data_read(ssh, buf, room)) > 0) where room can legitimately reach 0 (e.g. a full application buffer) would previously terminate cleanly on the 0 return and now terminates on a negative value that reads as a failure. The in-tree callers (apps/wolfssh/wolfssh.c, examples/client/client.c, src/wolfscp.c) all pass SIZE-1 > 0, so they are unaffected, and the change is asserted intentionally by test_ChannelReadExtBadArgs. Flagging so the behavior change is a conscious, documented decision for external API consumers.

Recommendation: Confirm the outSz==0 -> WS_BAD_ARGUMENT change is intended and document it for API consumers, or special-case outSz==0 to return 0 to preserve prior behavior.

Referenced code: src/ssh.c:1456-1470 (via _ChannelReadExt guard at src/ssh.c:3176) (2 lines)

---

Review generated by Skoll

Items addressed

[wolfSSL-Fenrir-bot]

Fenrir Automated Review — PR #1054

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 2
2 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.