Skip to content

fail the session when a packet handler records a fatal error - #12

Open
precla wants to merge 1 commit into
mainfrom
ssh_leading_bytes_dropped
Open

fail the session when a packet handler records a fatal error#12
precla wants to merge 1 commit into
mainfrom
ssh_leading_bytes_dropped

Conversation

@precla

@precla precla commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

libssh's channel_rcv_data frees a whole CHANNEL_DATA payload and returns SSH_PACKET_USED when it exceeds the channel's local window. The session keeps running minus those bytes, so the application sees a stream missing its head, and ssh_event_dopoll cannot report it.

session_event_poll now compares the session error code across the poll and fails the session if a fatal error appeared during it. The snapshot avoids tripping on a stale error.

this narrows the race rather than closing it: dopoll runs the data callbacks inline. Measured 6 of 12 and 7 of 11 blocked, versus none unguarded.

added storm regression test and a harness that forces a real discard

related to #7

this does not fully fix the issue, it's just a minor improvement

you can see in src/test_ssh_hello_storm.sh that the libssh is in one case run patched. which might be the proper fix. but it is questionable if upstream would allow/merge that change.
More details regarding the libssh behavior that causes this issue:

What RFC 4254 says:

Section 5.2, Data Transfer, verbatim:

The window size specifies how many bytes the other party can send before it
must wait for the window to be adjusted.

The maximum amount of data allowed is determined by the maximum packet size
for the channel, and the current window size, whichever is smaller.

Both parties MAY ignore all extra data sent after the allowed window is empty.

So a sender that exceeds the advertised window violates the second sentence.
A receiver that ignores the excess is explicitly permitted by the third.
Ignoring is sanctioned; terminating the connection is also allowed but is
harsher than the RFC contemplates and breaks interop with a peer that overshoots
slightly.

what the libssh patch does and does not fix:

It does not stop data loss. When this branch is reached, bytes beyond the
window are gone either way — the clamp only delivers the portion that was within
the window instead of nothing. The wins are that a partially-overrunning peer no
longer costs a whole packet, and that the session's error state is no longer incorrect.

Note the interaction with the mitigation in session_event_poll: that check
exists solely because the discard is silent. Clamping removes the bogus
SSH_FATAL, so with this patch the guard no longer fires for this branch — and
any residual loss becomes silent again. Keep the guard for the CLOSED_REMOTE
branch and for any future handler that records a fatal without acting on it, but
do not expect the two changes to compose into a guarantee.

The underlying question is still open: why len > local_window occurs in
the field. Whether the peer overshoots the advertised window or libssh's own
accounting drifts decides whether this patch is the remedy or merely makes a
peer bug less damaging. ACTON_SSH_LIBSSH_LOG=warn prints
Data packet too big for our window(N vs M); comparing M against what the
peer should have believed the window to be identifies which side drifted.

libssh's channel_rcv_data frees a whole CHANNEL_DATA payload and returns
SSH_PACKET_USED when it exceeds the channel's local window. The session keeps
running minus those bytes, so the application sees a stream missing its head,
and ssh_event_dopoll cannot report it.

session_event_poll now compares the session error code across the poll and
fails the session if a fatal error appeared during it. The snapshot avoids
tripping on a stale error.

this narrows the race rather than closing it: dopoll runs the data callbacks
inline. Measured 6 of 12 and 7 of 11 blocked, versus none unguarded.

added storm regression test and a harness that forces a real discard

Signed-off-by: Antonio Prcela <antonio.prcela@sartura.hr>
@precla
precla force-pushed the ssh_leading_bytes_dropped branch from 0431850 to 7d18439 Compare August 27, 2026 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant