ssh: stop driving a session after a disconnect - #1211
Conversation
wolfSSH_accept() and wolfSSH_connect() drive the handshake only while the session is live. Both gate on SendAfterDisconnect() ahead of the pending-send block, which would otherwise flush a queued disconnect and count it as the next handshake message; the shutdown paths own that flush. - The prototype sits ahead of both drivers, since either can be the only one built. - TestDisconnectGatesAccept() and TestDisconnectGatesConnect() cover a local disconnect, one from the peer, and a queued short send. One test per endpoint, so a single-sided build keeps the coverage that applies to it. - A received disconnect used to reach the error-state test in wolfSSH_accept() and report WS_INVALID_STATE_E; the gate answers WS_FATAL_ERROR first, and both tests pin that. - The contract comments in ssh.h and internal.h drop the ungated note.
There was a problem hiding this comment.
Pull request overview
This PR aligns session behavior with RFC 4253 §11.1 by ensuring that once a DISCONNECT is sent or received, the library stops driving handshake progress and stops emitting any protocol replies triggered by later inbound messages.
Changes:
- Gate
wolfSSH_accept()andwolfSSH_connect()onSendAfterDisconnect()to prevent handshake driving (and pending-send flushing) after a disconnect. - Update
DoPacket()to skip dispatching all post-disconnect inbound messages exceptMSGID_DISCONNECT, preventing post-termination replies and dropping late inbound traffic while keeping framing in sync. - Adjust
wolfSSH_worker()to stop reportingWS_REKEYINGafter a disconnect so callers don’t keep pumping a dead session.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
wolfssh/ssh.h |
Updates public API documentation to reflect post-disconnect gating and inbound-drop behavior. |
wolfssh/internal.h |
Clarifies internal state contract for disconnected and the fact that inbound dispatch is skipped post-disconnect. |
src/ssh.c |
Adds disconnect gating to accept/connect drivers and prevents WS_REKEYING reporting after disconnect. |
src/internal.c |
Skips inbound message dispatch after disconnect (except DISCONNECT) while still advancing the input frame safely. |
tests/regress.c |
Adds regression coverage for the accept/connect gates and for silencing/dropping post-disconnect inbound handling. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1211
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. Reported findings require changes before merge.
| * buffered stderr and its window credit. A disconnect outranks the | ||
| * rekey, the way the head-of-list reads have it: the NEWKEYS that | ||
| * clears isKeying is skipped from here on, so the flag latches. */ | ||
| if (ssh->isKeying && !ssh->disconnected && ret != WS_EXTDATA) { |
There was a problem hiding this comment.
New !ssh-disconnected rekey gate in wolfSSH_worker() has no test coverage · Missing edge-case coverage on a function the PR also changed
The PR adds !ssh->disconnected to the WS_REKEYING gate in wolfSSH_worker(), but none of the five new tests — nor any existing one — calls wolfSSH_worker() with both isKeying and disconnected set. Reverting the condition leaves the suite green.
Fix: Add a test that sets isKeying, takes a disconnect, then asserts wolfSSH_worker() no longer returns WS_REKEYING, mirroring TestDisconnectOutranksRekey().
| * buffered stderr and its window credit. A disconnect outranks the | ||
| * rekey, the way the head-of-list reads have it: the NEWKEYS that | ||
| * clears isKeying is skipped from here on, so the flag latches. */ | ||
| if (ssh->isKeying && !ssh->disconnected && ret != WS_EXTDATA) { |
There was a problem hiding this comment.
wolfSSH_worker() reports success indefinitely on a disconnected session · SSH protocol violations
DoPacket() now returns WS_SUCCESS for every post-disconnect message, so DoReceive() sets ssh->error = WS_SUCCESS; adding !ssh->disconnected removes the last non-success return. After a locally-sent disconnect wolfSSH_worker() returns WS_SUCCESS with wolfSSH_get_error() 0 for unbounded peer traffic, so a worker-driven caller never tears the session down.
Fix: Have wolfSSH_worker() set ssh->error = WS_DISCONNECT and return WS_FATAL_ERROR once ssh->disconnected is set, as wolfSSH_accept() and wolfSSH_connect() now do.
DoPacket() skips the whole message dispatch once ssh->disconnected is set, for every message but a DISCONNECT. The handlers that answer must not -- a close draws an EOF and a close of ours, a request a success or failure, an open a confirmation, an unknown message an UNIMPLEMENTED -- and what the rest would record is of no use to a caller that can no longer send. RFC 4253 section 11.1. - Inbound data from here on is dropped rather than buffered, so the read path hands back only what arrived before the disconnect. ssh.h and internal.h say so, beside the calls and beside the flag. - A DISCONNECT still reaches DoDisconnect(), which sends nothing and is what latches WS_DISCONNECT; SendDisconnect() sets the flag too, so ours can be the one that raised it. - The frame advance steps over the whole packet, so the stream stays in step with no payload bookkeeping of its own. - wolfSSH_worker() gates on SendAfterDisconnect() the way wolfSSH_accept() and wolfSSH_connect() do. With the dispatch skipped there is no non-success left to return, so it would answer a healthy session for as long as the peer kept talking and the SFTP and SCP drive loops would keep pumping a dead one. The rekey test below the gate needs no disconnect of its own: the gate returns first, and a DISCONNECT arriving mid-pass leaves ret fatal. wolfSSH_shutdown() drops the channel on a disconnect before its own pump, so the gate does not cost it the read it does there. - tests/regress.c pins all four: no reply goes out, the stream stays in step with a disconnect queued behind a skipped close, late channel data is dropped rather than queued, and the worker reports the disconnect on the pass that takes it and on every pass behind it.
f323531 to
e95c975
Compare
RFC 4253 section 11.1 ends the session at the disconnect, but two paths still ran afterwards.
wolfSSH_accept()andwolfSSH_connect()drove the handshake, and their pending-send block would flush a disconnect of ours left queued by a short send, counting it as the next handshake message.DoPacket()still dispatched every inbound message, so the handlers that answer -- a close draws an EOF and a close of ours, a request a success or failure, an open a confirmation, an unknown message an UNIMPLEMENTED -- put traffic on a session that is over.SendAfterDisconnect()ahead of the pending-send block; the shutdown paths own that flushDoPacket()skips the whole dispatch oncessh->disconnectedis set, for every message but a DISCONNECT, which still reachesDoDisconnect()and is what latchesWS_DISCONNECT. The frame advance steps over the packet using the lengthDoReceive()already validated, so the stream stays in step with no payload bookkeeping of its ownwolfSSH_worker()stops reportingWS_REKEYINGonce the session is over. The NEWKEYS that clearsisKeyingis skipped from here on, so the flag would latch for the rest of the session and the SFTP and SCP drive loops would keep pumping a dead onessh.handinternal.hrecord that inbound traffic is dropped rather than buffered, and drop the note that the drivers are ungatedA received disconnect used to reach the error-state test in
wolfSSH_accept()and reportWS_INVALID_STATE_E; the gate answersWS_FATAL_ERRORfirst. Both driver tests pin that, alongside the local disconnect and the queued short send, one test per endpoint so a single-sided build keeps the coverage that applies to it.This is the first of three branches re-cut from #1195, which had grown to 25 commits across three unrelated concerns. This strand owns all of the
tests/regress.cchurn and reads nothing the EOF work adds; its lineage is the merged "Block every send after a disconnect" series, not the EOF PR.Merge order: #1211, #1212, #1195, then #1148. Nothing here conflicts with master's 14 commits since the fork point -- they touch no file this branch touches.