From 279210d13fedf5be6d476bad5df6a015042bb905 Mon Sep 17 00:00:00 2001 From: Erick Tryzelaar Date: Tue, 2 Jun 2020 17:54:51 +0000 Subject: [PATCH] ssh: don't err out on channel request msgs to unknown channels 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 TryBot-Result: Gobot Gobot Reviewed-by: Filippo Valsorda --- ssh/mux.go | 23 +++++- ssh/mux_test.go | 214 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 236 insertions(+), 1 deletion(-) diff --git a/ssh/mux.go b/ssh/mux.go index f190162..9654c01 100644 --- a/ssh/mux.go +++ b/ssh/mux.go @@ -240,7 +240,7 @@ func (m *mux) onePacket() error { id := binary.BigEndian.Uint32(packet[1:]) ch := m.chanList.getChan(id) if ch == nil { - return fmt.Errorf("ssh: invalid channel %d", id) + return m.handleUnknownChannelPacket(id, packet) } return ch.handlePacket(packet) @@ -328,3 +328,24 @@ func (m *mux) openChannel(chanType string, extra []byte) (*channel, error) { return nil, fmt.Errorf("ssh: unexpected packet in response to channel open: %T", msg) } } + +func (m *mux) handleUnknownChannelPacket(id uint32, packet []byte) error { + msg, err := decode(packet) + if err != nil { + return err + } + + switch msg := msg.(type) { + // RFC 4254 section 5.4 says unrecognized channel requests should + // receive a failure response. + case *channelRequestMsg: + if msg.WantReply { + return m.sendMessage(channelRequestFailureMsg{ + PeersID: msg.PeersID, + }) + } + return nil + default: + return fmt.Errorf("ssh: invalid channel %d", id) + } +} diff --git a/ssh/mux_test.go b/ssh/mux_test.go index 94596ec..0b6b74d 100644 --- a/ssh/mux_test.go +++ b/ssh/mux_test.go @@ -9,6 +9,7 @@ import ( "io/ioutil" "sync" "testing" + "time" ) func muxPair() (*mux, *mux) { @@ -288,6 +289,219 @@ func TestMuxChannelRequest(t *testing.T) { } } +func TestMuxUnknownChannelRequests(t *testing.T) { + clientPipe, serverPipe := memPipe() + client := newMux(clientPipe) + defer serverPipe.Close() + defer client.Close() + + kDone := make(chan struct{}) + go func() { + // Ignore unknown channel messages that don't want a reply. + err := serverPipe.writePacket(Marshal(channelRequestMsg{ + PeersID: 1, + Request: "keepalive@openssh.com", + WantReply: false, + RequestSpecificData: []byte{}, + })) + if err != nil { + t.Fatalf("send: %v", err) + } + + // Send a keepalive, which should get a channel failure message + // in response. + err = serverPipe.writePacket(Marshal(channelRequestMsg{ + PeersID: 2, + Request: "keepalive@openssh.com", + WantReply: true, + RequestSpecificData: []byte{}, + })) + if err != nil { + t.Fatalf("send: %v", err) + } + + packet, err := serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + decoded, err := decode(packet) + if err != nil { + t.Fatalf("decode failed: %v", err) + } + + switch msg := decoded.(type) { + case *channelRequestFailureMsg: + if msg.PeersID != 2 { + t.Fatalf("received response to wrong message: %v", msg) + } + default: + t.Fatalf("unexpected channel message: %v", msg) + } + + kDone <- struct{}{} + + // Receive and respond to the keepalive to confirm the mux is + // still processing requests. + packet, err = serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + if packet[0] != msgGlobalRequest { + t.Fatalf("expected global request") + } + + err = serverPipe.writePacket(Marshal(globalRequestFailureMsg{ + Data: []byte{}, + })) + if err != nil { + t.Fatalf("failed to send failure msg: %v", err) + } + + close(kDone) + }() + + // Wait for the server to send the keepalive message and receive back a + // response. + select { + case <-kDone: + case <-time.After(10 * time.Second): + t.Fatalf("server never received ack") + } + + // Confirm client hasn't closed. + if _, _, err := client.SendRequest("keepalive@golang.org", true, nil); err != nil { + t.Fatalf("failed to send keepalive: %v", err) + } + + select { + case <-kDone: + case <-time.After(10 * time.Second): + t.Fatalf("server never shut down") + } +} + +func TestMuxClosedChannel(t *testing.T) { + clientPipe, serverPipe := memPipe() + client := newMux(clientPipe) + defer serverPipe.Close() + defer client.Close() + + kDone := make(chan struct{}) + go func() { + // Open the channel. + packet, err := serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + if packet[0] != msgChannelOpen { + t.Fatalf("expected chan open") + } + + var openMsg channelOpenMsg + if err := Unmarshal(packet, &openMsg); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + // Send back the opened channel confirmation. + err = serverPipe.writePacket(Marshal(channelOpenConfirmMsg{ + PeersID: openMsg.PeersID, + MyID: 0, + MyWindow: 0, + MaxPacketSize: channelMaxPacket, + })) + if err != nil { + t.Fatalf("send: %v", err) + } + + // Close the channel. + err = serverPipe.writePacket(Marshal(channelCloseMsg{ + PeersID: openMsg.PeersID, + })) + if err != nil { + t.Fatalf("send: %v", err) + } + + // Send a keepalive message on the channel we just closed. + err = serverPipe.writePacket(Marshal(channelRequestMsg{ + PeersID: openMsg.PeersID, + Request: "keepalive@openssh.com", + WantReply: true, + RequestSpecificData: []byte{}, + })) + if err != nil { + t.Fatalf("send: %v", err) + } + + // Receive the channel closed response. + packet, err = serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + if packet[0] != msgChannelClose { + t.Fatalf("expected channel close") + } + + // Receive the keepalive response failure. + packet, err = serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + if packet[0] != msgChannelFailure { + t.Fatalf("expected channel close") + } + kDone <- struct{}{} + + // Receive and respond to the keepalive to confirm the mux is + // still processing requests. + packet, err = serverPipe.readPacket() + if err != nil { + t.Fatalf("read packet: %v", err) + } + if packet[0] != msgGlobalRequest { + t.Fatalf("expected global request") + } + + err = serverPipe.writePacket(Marshal(globalRequestFailureMsg{ + Data: []byte{}, + })) + if err != nil { + t.Fatalf("failed to send failure msg: %v", err) + } + + close(kDone) + }() + + // Open a channel. + ch, err := client.openChannel("chan", nil) + if err != nil { + t.Fatalf("OpenChannel: %v", err) + } + defer ch.Close() + + // Wait for the server to close the channel and send the keepalive. + select { + case <-kDone: + case <-time.After(10 * time.Second): + t.Fatalf("server never received ack") + } + + // Make sure the channel closed. + if _, ok := <-ch.incomingRequests; ok { + t.Fatalf("channel not closed") + } + + // Confirm client hasn't closed + if _, _, err := client.SendRequest("keepalive@golang.org", true, nil); err != nil { + t.Fatalf("failed to send keepalive: %v", err) + } + + select { + case <-kDone: + case <-time.After(10 * time.Second): + t.Fatalf("server never shut down") + } +} + func TestMuxGlobalRequest(t *testing.T) { clientMux, serverMux := muxPair() defer serverMux.Close()