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

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

Comments

@erickt
Copy link

@erickt 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
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
@gopherbot
Copy link

@gopherbot gopherbot commented May 6, 2020

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

@toothrot
Copy link
Contributor

@toothrot toothrot commented May 11, 2020

erickt added a commit to erickt/crypto that referenced this issue Jun 2, 2020
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.