Repository navigation
internal: cap open channels per session - #1369
ejohnstown wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The review found an issue that should be addressed before approval.
2 open issues
What changed in this PR
Adds a configurable per-session limit for peer-initiated SSH channel opens.
Changes:
- Refuses opens beyond the limit with
OPEN_RESOURCE_SHORTAGE. - Adds regression coverage for reaching the limit and reopening a channel.
- Simplifies the port-zero forwarding refusal path.
| File | Description |
|---|---|
wolfssh/internal.h |
Defines and validates the channel limit. |
src/internal.c |
Enforces the limit and adjusts forwarding refusal handling. |
tests/regress.c |
Tests the limit and adapts multi-channel tests. |
Review effort: Balanced
DoChannelOpen() refuses a peer CHANNEL_OPEN with OPEN_RESOURCE_SHORTAGE once WOLFSSH_MAX_CHANNELS channels are open, before allocating anything. Each channel holds a window-sized buffer, so uncapped forward or agent opens could exhaust memory. The session stays up after a refusal. - WOLFSSH_MAX_CHANNELS defaults to 64 and is overridable at build time - regress test opens channels to the cap, checks the next is refused and that a close makes room again - regress tests that open a second channel build only with a cap of at least 2; below that, TestSecondSessionChannelRejected expects OPEN_RESOURCE_SHORTAGE Issue: F-12620
DoGlobalRequestFwd() sets WS_RESOURCE_E for a port-0 tcpip-forward the callback gave no port for, whether or not a callback is set. Only the cleanup call stays behind the fwdCb check. The guard is always true on that path today, so behavior is unchanged; the existing port-0 regress tests cover it. Issue: F-10557
390e5d5 to
2f2263a
Compare
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1369
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 3 in-scope changed file(s) opened by the reviewer; not opened: wolfssh/internal.h
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It adds a limit on peer-reachable input in the SSH channel-open handler, and the two Copilot threads on the new check were resolved by the author, so a human look would still be worthwhile.
What was reviewed:
- DoChannelOpen: the cap check runs after the fixed header is parsed and before anything is allocated. A rejection takes the existing failure path, which sends CHANNEL_OPEN_FAILURE with the peer's ID and OPEN_RESOURCE_SHORTAGE and keeps the session up. The cap covers only peer-initiated opens; channels the local side opens are not counted against it.
- DoGlobalRequestFwd: moving
ret = WS_RESOURCE_Eout changes nothing. That branch only runs when ret is WS_SUCCESS, and ret can only be WS_SUCCESS there if fwdCb is set. - tests/regress.c: the
#if/#endifnesting in main balances. The newWOLFSSH_MAX_CHANNELS >= 2guard closes before the existing agent#endifand the outer client#endif.
Extended reasoning...
The change touches src/internal.c (a new cap on peer CHANNEL_OPEN in DoChannelOpen, plus a ret assignment moved in DoGlobalRequestFwd), wolfssh/internal.h (a new WOLFSSH_MAX_CHANNELS macro that defaults to 64) and tests/regress.c (a new cap test and build guards on tests that open two channels). The code is small and consistent with the existing failure paths. However, it is DoS-hardening logic in network-facing SSH protocol handling, and the default limit is a design choice. Two inline Copilot comments on the new check were resolved by the PR author, and their content can't be verified here, so this was deferred rather than approved.
DoChannelOpen() refuses a peer CHANNEL_OPEN with OPEN_RESOURCE_SHORTAGE
once WOLFSSH_MAX_CHANNELS (default 64) are open, before allocating
anything. The session stays up.
check; no behavior change
Issue: F-12620, F-10557