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 <filippo@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Filippo Valsorda <filippo@golang.org>
2020-06-02 18:02:16 +00:00
parent 06a226fb4e
commit 279210d13f
2 changed files with 236 additions and 1 deletions
@@ -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)
+	}
+}
@@ -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()