Sitelet https://github.com/golang/crypto/commit/7d695da948bfa44ed6eedcebc8f43bcb50e94a57
Skip to content

Commit 7d695da

Browse files
drakkandrakkan
committed
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>
1 parent 5b7f841 commit 7d695da

2 files changed

Lines changed: 93 additions & 0 deletions

File tree

‎ssh/agent/forward.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ func ForwardToAgent(client *ssh.Client, keyring Agent) error {
4141
continue
4242
}
4343
go ssh.DiscardRequests(reqs)
44+
go io.Copy(io.Discard, channel.Stderr())
4445
go func() {
4546
ServeAgent(keyring, channel)
4647
channel.Close()
@@ -72,6 +73,7 @@ func ForwardToRemote(client *ssh.Client, addr string) error {
7273
continue
7374
}
7475
go ssh.DiscardRequests(reqs)
76+
go io.Copy(io.Discard, channel.Stderr())
7577
go forwardUnixSocket(channel, addr)
7678
}
7779
}()

‎ssh/agent/server_test.go‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"reflect"
1414
"strings"
1515
"testing"
16+
"time"
1617

1718
"golang.org/x/crypto/ssh"
1819
)
@@ -88,6 +89,96 @@ func TestSetupForwardAgent(t *testing.T) {
8889
conn.Close()
8990
}
9091

92+
func TestForwardAgentDiscardsStderr(t *testing.T) {
93+
a, b, err := netPipe()
94+
if err != nil {
95+
t.Fatalf("netPipe: %v", err)
96+
}
97+
defer a.Close()
98+
defer b.Close()
99+
100+
serverConf := ssh.ServerConfig{
101+
NoClientAuth: true,
102+
}
103+
serverConf.AddHostKey(testSigners["rsa"])
104+
incoming := make(chan *ssh.ServerConn, 1)
105+
go func() {
106+
conn, _, _, err := ssh.NewServerConn(a, &serverConf)
107+
incoming <- conn
108+
if err != nil {
109+
t.Errorf("NewServerConn error: %v", err)
110+
return
111+
}
112+
}()
113+
114+
conf := ssh.ClientConfig{
115+
HostKeyCallback: ssh.InsecureIgnoreHostKey(),
116+
}
117+
conn, chans, reqs, err := ssh.NewClientConn(b, "", &conf)
118+
if err != nil {
119+
t.Fatalf("NewClientConn: %v", err)
120+
}
121+
client := ssh.NewClient(conn, chans, reqs)
122+
defer client.Close()
123+
124+
if err := ForwardToAgent(client, NewKeyring()); err != nil {
125+
t.Fatalf("ForwardToAgent: %v", err)
126+
}
127+
128+
server := <-incoming
129+
if server == nil {
130+
t.Fatal("Unable to get server")
131+
}
132+
defer server.Close()
133+
134+
ch, chanReqs, err := server.OpenChannel(channelType, nil)
135+
if err != nil {
136+
t.Fatalf("OpenChannel(%q): %v", channelType, err)
137+
}
138+
go ssh.DiscardRequests(chanReqs)
139+
defer ch.Close()
140+
141+
// Write more than the default channel receive window (2 MiB) on the
142+
// stderr stream. If the forwarder did not drain it the remote window
143+
// would be exhausted and this write would block forever, because no
144+
// window-adjust messages are emitted for an extended stream that is
145+
// never read.
146+
const payload = 4 * 1024 * 1024
147+
done := make(chan error, 1)
148+
go func() {
149+
buf := make([]byte, 32*1024)
150+
remaining := payload
151+
for remaining > 0 {
152+
n := len(buf)
153+
if remaining < n {
154+
n = remaining
155+
}
156+
w, err := ch.Stderr().Write(buf[:n])
157+
if err != nil {
158+
done <- err
159+
return
160+
}
161+
remaining -= w
162+
}
163+
done <- nil
164+
}()
165+
166+
select {
167+
case err := <-done:
168+
if err != nil {
169+
t.Fatalf("write to stderr: %v", err)
170+
}
171+
case <-time.After(10 * time.Second):
172+
t.Fatal("write to stderr blocked: ForwardToAgent is not draining channel.Stderr()")
173+
}
174+
175+
// The agent must still be reachable on the main stream after the
176+
// stderr traffic.
177+
if _, err := NewClient(ch).List(); err != nil {
178+
t.Fatalf("List after stderr flood: %v", err)
179+
}
180+
}
181+
91182
func TestV1ProtocolMessages(t *testing.T) {
92183
c1, c2, err := netPipe()
93184
if err != nil {

0 commit comments

Comments
 (0)