Skip to content

ssh: drop unexpected message types in channel.handlePacket#355

Open
adilburaksen wants to merge 1 commit into
golang:masterfrom
adilburaksen:ssh-channel-default-arm-drop
Open

ssh: drop unexpected message types in channel.handlePacket#355
adilburaksen wants to merge 1 commit into
golang:masterfrom
adilburaksen:ssh-channel-default-arm-drop

Conversation

@adilburaksen

@adilburaksen adilburaksen commented Jun 9, 2026

Copy link
Copy Markdown

(*channel).handlePacket handles the channel response and request messages
explicitly and falls back to ch.msg <- msg in the default arm for every
other decoded type. Nothing ever reads those types from ch.msg: it is only
consumed by SendRequest (channelRequestSuccess/Failure) and the channel-open
wait (channelOpenConfirm/Failure), all handled in explicit cases above.

A peer can route any decode()-able message (for example a service-request or
ext-info packet) to an existing channel id; it falls through to the default
arm and is queued to the bounded ch.msg. On an open, idle channel nothing
drains ch.msg, so after chanSize such messages the blocking send in the
default arm stalls the single mux read loop for the whole connection (the
mux/readLoop goroutines leak; closing the connection does not release them).
When a SendRequest is in flight, the queued messages instead corrupt its
response handling.

This is the same primitive that was addressed for the unexpected-response
case (the fix for the deadlock on unexpected channel responses), which moved
channelRequestSuccess/Failure out of the default arm into a gated,
non-blocking case but left the default arm unchanged. Since no consumer
awaits any default-arm type, ignore them instead of queuing to ch.msg.

Adds a regression test (TestChannelUnexpectedDefaultMessagesDiscarded) that
floods a channel with default-arm messages and confirms the channel and mux
loop remain usable; it fails without the fix. go test ./ssh/... passes.

Updates golang/go#79564

@gopherbot

Copy link
Copy Markdown
Contributor

This PR (HEAD: 2716b0f) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/crypto/+/788740.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

(*channel).handlePacket handles the channel response and request messages
explicitly and falls back to `ch.msg <- msg` in the default arm for every
other decoded type. Nothing ever reads those types from ch.msg: it is only
consumed by SendRequest (channelRequestSuccess/Failure) and the channel-open
wait (channelOpenConfirm/Failure), all handled in explicit cases above.

A peer can route any decode()-able message (for example a service-request or
ext-info packet) to an existing channel id; it falls through to the default
arm and is queued to the bounded ch.msg. On an open, idle channel nothing
drains ch.msg, so after chanSize such messages the blocking send in the
default arm stalls the single mux read loop for the whole connection (the
mux/readLoop goroutines leak; closing the connection does not release them).
When a SendRequest is in flight, the queued messages instead corrupt its
response handling.

This is the same primitive that was addressed for the unexpected-response
case (the fix for the deadlock on unexpected channel responses), which moved
channelRequestSuccess/Failure out of the default arm into a gated,
non-blocking case but left the default arm unchanged. Since no consumer
awaits any default-arm type, ignore them instead of queuing to ch.msg.

Adds a regression test (TestChannelUnexpectedDefaultMessagesDiscarded) that
floods a channel with default-arm messages and confirms the channel and mux
loop remain usable; it fails without the fix. go test ./ssh/... passes.

Updates golang/go#79564
@adilburaksen adilburaksen force-pushed the ssh-channel-default-arm-drop branch from 2716b0f to df564be Compare June 9, 2026 14:35
@gopherbot

Copy link
Copy Markdown
Contributor

Message from Gopher Robot:

Patch Set 1:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/788740.
After addressing review feedback, remember to publish your drafts!

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Adil Burak ŞEN:

Patch Set 1:

(1 comment)

Done. Addressed both points in the updated PR: the commit message body is now wrapped at ~76 characters, and added Updates golang/go#79564 — this change completes the fix for that issue (CVE-2026-39830), which moved channelRequestSuccess/Failure out of the default arm but left the default arm's unconditional send in place. GerritBot should re-import the revised commit message on its next sync.


Please don’t reply on this GitHub thread. Visit golang.org/cl/788740.
After addressing review feedback, remember to publish your drafts!

@gopherbot

Copy link
Copy Markdown
Contributor

This PR (HEAD: df564be) has been imported to Gerrit for code review.

Please visit Gerrit at https://go-review.googlesource.com/c/crypto/+/788740.

Important tips:

  • Don't comment on this PR. All discussion takes place in Gerrit.
  • You need a Gmail or other Google account to log in to Gerrit.
  • To change your code in response to feedback:
    • Push a new commit to the branch used by your GitHub PR.
    • A new "patch set" will then appear in Gerrit.
    • Respond to each comment by marking as Done in Gerrit if implemented as suggested. You can alternatively write a reply.
    • Critical: you must click the blue Reply button near the top to publish your Gerrit responses.
    • Multiple commits in the PR will be squashed by GerritBot.
  • The title and description of the GitHub PR are used to construct the final commit message.
    • Edit these as needed via the GitHub web interface (not via Gerrit or git).
    • You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
  • See the Sending a change via GitHub and Reviews sections of the Contribution Guide as well as the FAQ for details.

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Nicola Murino:

Patch Set 2:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/788740.
After addressing review feedback, remember to publish your drafts!

@gopherbot

Copy link
Copy Markdown
Contributor

Message from Adil Burak ŞEN:

Patch Set 2:

Thanks for confirming. Happy to leave the regression test (TestChannelUnexpectedDefaultMessagesDiscarded) here if it's useful for your fix; otherwise feel free to close this CL in favour of yours. Glad the default-arm sibling is being addressed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/788740.
After addressing review feedback, remember to publish your drafts!

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.

2 participants