fix: prevent panic from crashing the whole server - #54
Merged
Conversation
Every connection is served on its own goroutine, and the SSH handshake and session handlers run there. Go has no process-wide panic handler, so a panic on any of those goroutines terminated the entire server process and disconnected every other user along with it. Nothing in the library recovered, and the panic did not even have to come from application code: a fault while decrypting or parsing an incoming packet happens before authentication, so an unauthenticated client could trigger it. Panics are now contained to the connection that caused them, logged with a stack trace, and the session is ended with a non-zero exit status. Connection close callbacks still run and shutdown accounting is still released, so a graceful shutdown does not wait on a connection that already died. Recovery has to be installed on the goroutine that panics, since a recover only sees panics unwinding its own stack. It therefore cannot live in the accept loop or in session middleware, and every goroutine started per connection needs its own.
Port forwarding and agent forwarding proxy data on their own goroutines, which the previous change did not reach. These features are opt-in, so most servers are unaffected, but a panic while proxying a forwarded connection would still take the whole process down. The agent proxy needed a little more care: its copy goroutines signalled completion after copying rather than on the way out, so recovering a panic there would have left the parent waiting forever, leaking the goroutine along with the socket and the channel.
taciturnaxolotl
force-pushed
the
contain-connection-panics
branch
from
July 31, 2026 14:35
c205bd2 to
54c2140
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Panics raised while serving an SSH connection no longer terminate the server process. They are contained to the connection that caused them, logged with a stack trace, and the session ends with a non-zero exit status.
The big bad bug
Every connection is served on its own goroutine, and the handshake and session handlers run there. Go has no process-wide panic handler, so a panic on any of those goroutines killed the entire process and disconnected every other user.
The panic does not have to come from application code. A fault while decrypting or parsing an incoming packet happens before authentication, so an unauthenticated client could take a server down.
So nearly every SSH server built on this library is currently susceptible to a panic crashing their program.
The fix
Recovery has to be installed on the goroutine that panics, because a recover only sees panics unwinding its own stack. That rules out the accept loop, connection callbacks, and session middleware: by the time any of those return, their deferred functions have already run. So every goroutine started per connection gets its own handler.
Compatibility
No exported API changes. All new symbols are unexported and
HandleConn's signature is untouched, so callers who invoke it directly just get containment for free.Behaviour changes worth noting:
slogat error levelexit-status 1Connection close callbacks still fire and shutdown accounting is still released on the panic path, so a graceful shutdown does not hang waiting on a connection that already died. There is a test pinning that.
Found while assessing exploitability for GHSA-v57m-p5gf-9mf5 in soft-serve, where the missing recovery is what turns a handshake panic in
x/crypto/sshinto a full outage. This is not itself an advisory — it isn't independently exploitable, since it needs a separate panic bug to weaponise. It's an impact multiplier, and the pattern is inherited fromgliderlabs/sshupstream, which has the same gap.