ssh: return window credit for discarded extended data

handleData subtracts the length of every data packet from the receive
window, but extended data with a type code other than
SSH2_EXTENDED_DATA_STDERR is then silently discarded and, since no API
exists to read it, its window credit was never returned: adjustWindow
only runs as a side effect of reading a stream. A peer sending
extended data with an unknown type code could therefore permanently
shrink the channel window, down to a complete stall of the channel.

Credit the discarded bytes back through adjustWindow. adjustWindow
returns io.EOF if the local side has already sent a channel close;
ignore it like ReadExtended does, since an error returned here would
terminate the mux read loop and tear down the whole connection. For
comparison, OpenSSH rejects extended data with unexpected type codes
outright and never buffers it, so the dropped bytes are simply never
credited back to the misbehaving peer.

Fixes golang/go#80333

Change-Id: Ia311ce3ad008e7c89ce9752f1937c91374a4e74f
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/802901
Reviewed-by: Mark Freeman <markfreeman@google.com>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>

ssh/channel.go

@@ -344,7 +344,18 @@ func (ch *channel) handleData(packet []byte) error {
 	if extended == 1 {
 		ch.extPending.write(data)
 	} else if extended > 0 {
-		// discard other extended data.
+		// RFC 4254, Section 5.2 defines no extended data types other
+		// than stderr (type 1, handled above) and this package provides
+		// no API to read them, so the data is discarded. Credit its
+		// window back immediately: it can never be read, so the
+		// deduction above would otherwise shrink the window permanently.
+		// adjustWindow returns io.EOF if the local side has already
+		// sent a channel close; ignore it like ReadExtended does, since
+		// an error returned here would terminate the mux read loop and
+		// tear down the whole connection.
+		if err := ch.adjustWindow(length); err != nil && err != io.EOF {
+			return err
+		}
 	} else {
 		ch.pending.write(data)
 	}

ssh/channel_test.go

@@ -5,6 +5,8 @@
 package ssh
 
 import (
+	"encoding/binary"
+	"io"
 	"math/bits"
 	"testing"
 	"time"
@@ -138,3 +140,111 @@ func TestWriteExtendedNoInfiniteLoopOnLargeWrite(t *testing.T) {
 		t.Fatal("WriteExtended did not return after closing the window")
 	}
 }
+
+func TestDiscardedExtendedDataReturnsWindowCredit(t *testing.T) {
+	client, _, serverChans := forwardingPair(t)
+
+	clientCh, reqs, err := client.OpenChannel("test", nil)
+	if err != nil {
+		t.Fatalf("OpenChannel: %v", err)
+	}
+	go DiscardRequests(reqs)
+	defer clientCh.Close()
+
+	serverCh := (<-serverChans).(*channel)
+	defer serverCh.Close()
+
+	// Write more than the default channel receive window (2 MiB) of
+	// extended data with a type code other than stderr. The peer
+	// discards such data: if it did not return the window credit the
+	// write would block forever once the window is exhausted.
+	const payload = 4 * 1024 * 1024
+	done := make(chan error, 1)
+	go func() {
+		buf := make([]byte, 32*1024)
+		remaining := payload
+		for remaining > 0 {
+			n := min(len(buf), remaining)
+			nw, err := serverCh.WriteExtended(buf[:n], 2)
+			if err != nil {
+				done <- err
+				return
+			}
+			remaining -= nw
+		}
+		done <- nil
+	}()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("write of discarded extended data: %v", err)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("write of discarded extended data blocked: window credit is not returned")
+	}
+
+	// The main stream must still be usable.
+	want := []byte("main stream after extended data")
+	if _, err := serverCh.Write(want); err != nil {
+		t.Fatalf("write to main stream: %v", err)
+	}
+	got := make([]byte, len(want))
+	if _, err := io.ReadFull(clientCh, got); err != nil {
+		t.Fatalf("read from main stream: %v", err)
+	}
+	if string(got) != string(want) {
+		t.Fatalf("read %q from main stream, want %q", got, want)
+	}
+
+	// Reading the main stream payload above orders the test after all
+	// preceding packets: everything received has now been either read or
+	// discarded, so the full window credit must have been accounted.
+	// Anything less indicates a partial-credit leak that the liveness
+	// check above would not detect.
+	cc := clientCh.(*channel)
+	cc.windowMu.Lock()
+	window := cc.myWindow + cc.myConsumed
+	cc.windowMu.Unlock()
+	if window != channelWindowSize {
+		t.Errorf("myWindow+myConsumed = %d, want %d", window, channelWindowSize)
+	}
+}
+
+func TestDiscardedExtendedDataAfterClose(t *testing.T) {
+	client, _, serverChans := forwardingPair(t)
+
+	clientCh, reqs, err := client.OpenChannel("test", nil)
+	if err != nil {
+		t.Fatalf("OpenChannel: %v", err)
+	}
+	go DiscardRequests(reqs)
+	serverCh := <-serverChans
+	defer serverCh.Close()
+
+	ch := clientCh.(*channel)
+	// Close the channel locally: sentClose is now set, but the channel
+	// stays in the mux chanList until the peer's close arrives, so
+	// in-flight data packets are still dispatched to handleData.
+	if err := ch.Close(); err != nil {
+		t.Fatalf("Close: %v", err)
+	}
+
+	// Deliver extended data packets with an unknown type code, enough to
+	// cross the adjustWindow send threshold of 3*maxIncomingPayload.
+	// The window adjust message cannot be sent after the close and
+	// adjustWindow fails with io.EOF: handleData must swallow that
+	// error, since any error it returns terminates the mux read loop
+	// and tears down every channel on the connection.
+	const packetLen = 32 * 1024
+	packet := make([]byte, 13+packetLen)
+	packet[0] = msgChannelExtendedData
+	binary.BigEndian.PutUint32(packet[1:], ch.localId)
+	binary.BigEndian.PutUint32(packet[5:], 2)
+	binary.BigEndian.PutUint32(packet[9:], packetLen)
+	for range 5 {
+		if err := ch.handleData(packet); err != nil {
+			t.Fatalf("handleData after local close: %v", err)
+		}
+	}
+}