ssh/agent: drain channel stderr in agent forwarders

ForwardToAgent and ForwardToRemote only read the main stream of the
auth-agent@openssh.com channels they accept. If a peer sends data on
the channel's extended (stderr) stream the bytes accumulate in the
client-side extPending buffer and the receive window is never
replenished, because the window is only adjusted as a side effect of
ReadExtended. That can pin up to channelWindowSize (2 MiB) of memory
per channel and silently stalls any stderr traffic once the window is
exhausted.

The auth-agent protocol does not use stderr, so a well-behaved peer
never sends anything on it. To stay tolerant of misbehaving peers
without leaving the channel half-stuck, drain the stderr stream into
io.Discard, mirroring the existing DiscardRequests pattern. The
goroutine exits when the channel is closed because Stderr().Read
returns io.EOF.

Add a regression test that opens an agent-forwarding channel and
writes more than the default window on the stderr stream from the
server side. Without the fix the write blocks once the remote window
is exhausted; with the fix the bytes are drained and the agent stream
remains usable.

Change-Id: Iadf8ea6ca726c058421bbc39f92e0100579fda17
Reviewed-on: https://go-review.googlesource.com/c/crypto/+/783720
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>

ssh/agent/forward.go

@@ -41,6 +41,7 @@ func ForwardToAgent(client *ssh.Client, keyring Agent) error {
 				continue
 			}
 			go ssh.DiscardRequests(reqs)
+			go io.Copy(io.Discard, channel.Stderr())
 			go func() {
 				ServeAgent(keyring, channel)
 				channel.Close()
@@ -72,6 +73,7 @@ func ForwardToRemote(client *ssh.Client, addr string) error {
 				continue
 			}
 			go ssh.DiscardRequests(reqs)
+			go io.Copy(io.Discard, channel.Stderr())
 			go forwardUnixSocket(channel, addr)
 		}
 	}()

ssh/agent/server_test.go

@@ -13,6 +13,7 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	"golang.org/x/crypto/ssh"
 )
@@ -88,6 +89,96 @@ func TestSetupForwardAgent(t *testing.T) {
 	conn.Close()
 }
 
+func TestForwardAgentDiscardsStderr(t *testing.T) {
+	a, b, err := netPipe()
+	if err != nil {
+		t.Fatalf("netPipe: %v", err)
+	}
+	defer a.Close()
+	defer b.Close()
+
+	serverConf := ssh.ServerConfig{
+		NoClientAuth: true,
+	}
+	serverConf.AddHostKey(testSigners["rsa"])
+	incoming := make(chan *ssh.ServerConn, 1)
+	go func() {
+		conn, _, _, err := ssh.NewServerConn(a, &serverConf)
+		incoming <- conn
+		if err != nil {
+			t.Errorf("NewServerConn error: %v", err)
+			return
+		}
+	}()
+
+	conf := ssh.ClientConfig{
+		HostKeyCallback: ssh.InsecureIgnoreHostKey(),
+	}
+	conn, chans, reqs, err := ssh.NewClientConn(b, "", &conf)
+	if err != nil {
+		t.Fatalf("NewClientConn: %v", err)
+	}
+	client := ssh.NewClient(conn, chans, reqs)
+	defer client.Close()
+
+	if err := ForwardToAgent(client, NewKeyring()); err != nil {
+		t.Fatalf("ForwardToAgent: %v", err)
+	}
+
+	server := <-incoming
+	if server == nil {
+		t.Fatal("Unable to get server")
+	}
+	defer server.Close()
+
+	ch, chanReqs, err := server.OpenChannel(channelType, nil)
+	if err != nil {
+		t.Fatalf("OpenChannel(%q): %v", channelType, err)
+	}
+	go ssh.DiscardRequests(chanReqs)
+	defer ch.Close()
+
+	// Write more than the default channel receive window (2 MiB) on the
+	// stderr stream. If the forwarder did not drain it the remote window
+	// would be exhausted and this write would block forever, because no
+	// window-adjust messages are emitted for an extended stream that is
+	// never read.
+	const payload = 4 * 1024 * 1024
+	done := make(chan error, 1)
+	go func() {
+		buf := make([]byte, 32*1024)
+		remaining := payload
+		for remaining > 0 {
+			n := len(buf)
+			if remaining < n {
+				n = remaining
+			}
+			w, err := ch.Stderr().Write(buf[:n])
+			if err != nil {
+				done <- err
+				return
+			}
+			remaining -= w
+		}
+		done <- nil
+	}()
+
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("write to stderr: %v", err)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("write to stderr blocked: ForwardToAgent is not draining channel.Stderr()")
+	}
+
+	// The agent must still be reachable on the main stream after the
+	// stderr traffic.
+	if _, err := NewClient(ch).List(); err != nil {
+		t.Fatalf("List after stderr flood: %v", err)
+	}
+}
+
 func TestV1ProtocolMessages(t *testing.T) {
 	c1, c2, err := netPipe()
 	if err != nil {