Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

x/crypto/ssh: mux.onePacket shouldn't err if it receives a message to a closed channel #38908

Closed
erickt opened this issue May 6, 2020 · 2 comments
Closed

x/crypto/ssh: mux.onePacket shouldn't err if it receives a message to a closed channel #38908

erickt opened this issue May 6, 2020 · 2 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@erickt
Copy link

erickt commented May 6, 2020

What version of Go are you using (go version)?

$ go version
go version go1.14.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/etryzelaar/Library/Caches/go-build"
GOENV="/Users/etryzelaar/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/etryzelaar/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/etryzelaar/homebrew/Cellar/go/1.14.2_1/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/etryzelaar/homebrew/Cellar/go/1.14.2_1/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/etryzelaar/fuchsia/third_party/golibs/golang.org/x/crypto/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/8v/nyxy8zwd67324slpw9kqb_nh00j4g8/T/go-build942647229=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

openssh-portable implements the server-sent keepalives by first trying to send a no-op message on some open channel, before falling back to a global request if no channels are open. We have observed that it's possible for the server to emit a keepalive on a channel it has started to close. This can cause x/crypto/ssh/mux.go to err out. Here's what happens:

openssh-portable server          golang.org/x/crypto/ssh client
        |                                   |
 send close channel 1 ---------------> recv close channel 1
        |                                   |
        |                      + ----- send close channel 1
        |                      |            |
        |                      |       mark channel 1 as closed
        |                      |            |
 send keepalive on channel 1 - | ----> recv keepalive on channel 1
        |                      |            |
 recv close channel 1 <--------+       error out, invalid channel 1
        |                                    
 mark channel 1 as closed

This can happen because openssh-portable doesn't remove a channel from the open channel list until it has sent and received the SSH_MSG_CHANNEL_CLOSE message. If things are arranged just right, openssh-portable can send the SSH_MSG_CHANNEL_CLOSE, then the keepalive timer can trigger sending a SSH_MSG_CHANNEL_REQUEST before it has received a SSH_MSG_CHANNEL_CLOSE. From x/crypto/ssh's perspective, it has received and sent the closed messages, so it closes the channel, then receives a message to that closed channel.

While I think it would be nice if openssh-portable removed a partially closed channel from consideration for keepalives, rfc4254 section 5.3 suggests that it only needs to consider it closed if it's sent and received the closed messages. Furthermore, rfc4254 section 5.4 suggests that unrecognized requests should be replied with a SSH_MSG_CHANNEL_FAILURE:

If 'want reply' is FALSE, no response will be sent to the request. Otherwise, the recipient responds with either SSH_MSG_CHANNEL_SUCCESS, SSH_MSG_CHANNEL_FAILURE, or request-specific continuation messages. If the request is not recognized or is not supported for the channel, SSH_MSG_CHANNEL_FAILURE is returned.

What did you expect to see?

mux.go should send back a failure message and continue processing requests and responses.

What did you see instead?

mux.go errs out, which results in ssh.Client closing with an EOF.
@gopherbot gopherbot added this to the Unreleased milestone May 6, 2020
erickt added a commit to erickt/crypto that referenced this issue May 6, 2020
@erickt
ssh: mux.onePacket shouldn't err out on msgs to unknown channels
1db481b 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
@erickt erickt mentioned this issue May 6, 2020
Closed
@gopherbot
Copy link

Change https://golang.org/cl/232659 mentions this issue: ssh: mux.onePacket shouldn't err out on msgs to unknown channels

@toothrot toothrot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 11, 2020
@toothrot
Copy link
Contributor

/cc @hanwen @FiloSottile

erickt added a commit to erickt/crypto that referenced this issue Jun 2, 2020
@erickt
ssh: don't err out on channel request msgs to unknown channels
8a92f87 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: I1931efa6878da7763a84b484cf503a674c8e8d65
@golang golang locked and limited conversation to collaborators Jun 2, 2021
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@erickt @FiloSottile
ssh: don't err out on channel request msgs to unknown channels
c5bfb64 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
GitHub-Last-Rev: 8a92f87dc30697d9e3805af695efdf1b1dc8409b
GitHub-Pull-Request: golang/crypto#136
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/232659
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@erickt @c-expert-zigbee
ssh: don't err out on channel request msgs to unknown channels
8fabd0a 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
GitHub-Last-Rev: 8a92f87dc30697d9e3805af695efdf1b1dc8409b
GitHub-Pull-Request: golang/crypto#136
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/232659
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@erickt @c-expert-zigbee
ssh: don't err out on channel request msgs to unknown channels
0ede3ed 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
GitHub-Last-Rev: 8a92f87dc30697d9e3805af695efdf1b1dc8409b
GitHub-Pull-Request: golang/crypto#136
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/232659
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@erickt @FiloSottile
ssh: don't err out on channel request msgs to unknown channels
5ba6ff9 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
GitHub-Last-Rev: 8a92f87dc30697d9e3805af695efdf1b1dc8409b
GitHub-Pull-Request: golang/crypto#136
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/232659
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@erickt @FiloSottile
ssh: don't err out on channel request msgs to unknown channels
e689a9d 
rfc4254 section 5.4 states that channel request messages sent to an
unrecognized channel should be replied with a `SSH_MSG_CHANNEL_FAILURE`,
rather than erring out and closing the mux. This can occur with servers
like openssh-portable, which can begin to close a channel and also use
that channel for keepalives before it has received a closed response
from the client.

Fixes golang/go#38908

Change-Id: Id68b77e16b2889d3a21d505ed8018f9f457e067a
GitHub-Last-Rev: 8a92f87
GitHub-Pull-Request: golang#136
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/232659
Run-TryBot: Filippo Valsorda <filippo@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

Successfully merging a pull request may close this issue.

ssh: don't err out on channel request msgs to unknown channels erickt/crypto
3 participants
@toothrot @erickt @gopherbot