From 66627641389f2b2b5d75998729f551fae6680a8e Mon Sep 17 00:00:00 2001 From: James Tucker Date: Sun, 25 Oct 2026 20:28:17 +0000 Subject: [PATCH] session: don't park request loop on window-change, bound env churn An authenticated client could exhaust memory in the server process with per-session state that persisted after disconnect, in two ways: 1. A window-change request did a blocking send on the session's window channel. The channel has capacity for one window size and the initial pty-req already fills it, so if the session handler never drains the channel returned by Pty(), the very first window-change parks the handleRequests goroutine forever. The parked goroutine, the session, and everything it references (such as accumulated environment variables) leak even after the client disconnects, and the request loop stops servicing any further requests for the session. 2. Before a shell or exec request was handled, every env request appended an arbitrarily sized "key=value" string to the session's environment with no cap on count or size, so an env churn loop grew the heap without bound. Window-change sends are now non-blocking and keep-latest: when the consumer is not keeping up, the stale buffered value is replaced with the newest window size, so a slow consumer eventually observes the most recent size and the request loop never blocks. Break requests had the same blocking send on the registered break channel and are now dropped (replied false) when the consumer is not keeping up. Env requests are rejected once a session has maxEnvs (128) variables. While here, fix a data race between handleRequests writing the pty window size and the handler goroutine reading it via Pty(): both now take the session mutex, which previously went unused for pty state. Thanks to the security audit that identified the memory exhaustion condition (approximately 536 MB allocated in 7 seconds of churn, persisting long after disconnect when a parked goroutine retained the session). Updates tailscale/corp#50380 --- session.go | 51 ++++++++++++++- session_test.go | 168 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+), 3 deletions(-) diff --git a/session.go b/session.go index 75026501..e0b726b8 100644 --- a/session.go +++ b/session.go @@ -96,6 +96,12 @@ type Session interface { // when there is no signal channel specified const maxSigBufSize = 128 +// maxEnvs is the maximum number of environment variables a client may +// set on a single session before the session handler runs. Further +// "env" requests are rejected, bounding the memory a client can force +// the server to allocate per session. +const maxEnvs = 128 + func DefaultSessionHandler(srv *Server, conn *gossh.ServerConn, newChan gossh.NewChannel, ctx Context) { ch, reqs, err := newChan.Accept() if err != nil { @@ -136,6 +142,27 @@ type session struct { disablePtyEmulation bool } +// sendWindow delivers a window size to the Pty consumer without ever +// blocking the request handling loop. The channel has capacity for one +// window size; when the consumer is not keeping up, the stale buffered +// value is replaced with the latest one so a slow consumer eventually +// observes the most recent size instead of an old one. +func (sess *session) sendWindow(win Window) { + select { + case sess.winch <- win: + default: + // Buffer full: drop the stale entry and try once more. + select { + case <-sess.winch: + select { + case sess.winch <- win: + default: + } + default: + } + } +} + func (sess *session) DisablePTYEmulation() { sess.disablePtyEmulation = true } @@ -226,6 +253,8 @@ func (sess *session) Subsystem() string { } func (sess *session) Pty() (Pty, <-chan Window, bool) { + sess.Lock() + defer sess.Unlock() if sess.pty != nil { return *sess.pty, sess.winch, true } @@ -320,6 +349,10 @@ func (sess *session) handleRequests(reqs <-chan *gossh.Request) { req.Reply(false, nil) continue } + if len(sess.env) >= maxEnvs { + req.Reply(false, nil) + continue + } var kv struct{ Key, Value string } gossh.Unmarshal(req.Payload, &kv) sess.env = append(sess.env, fmt.Sprintf("%s=%s", kv.Key, kv.Value)) @@ -353,8 +386,10 @@ func (sess *session) handleRequests(reqs <-chan *gossh.Request) { continue } } + sess.Lock() sess.pty = &ptyReq sess.winch = make(chan Window, 1) + sess.Unlock() sess.winch <- ptyReq.Window defer func() { // when reqs is closed @@ -362,14 +397,19 @@ func (sess *session) handleRequests(reqs <-chan *gossh.Request) { }() req.Reply(ok, nil) case "window-change": + sess.Lock() if sess.pty == nil { + sess.Unlock() req.Reply(false, nil) continue } win, _, ok := parseWindow(req.Payload) if ok { sess.pty.Window = win - sess.winch <- win + } + sess.Unlock() + if ok { + sess.sendWindow(win) } req.Reply(ok, nil) case agentRequestType: @@ -380,8 +420,13 @@ func (sess *session) handleRequests(reqs <-chan *gossh.Request) { ok := false sess.Lock() if sess.breakCh != nil { - sess.breakCh <- true - ok = true + select { + case sess.breakCh <- true: + ok = true + default: + // Consumer is not keeping up: drop instead of + // parking the request handling loop. + } } req.Reply(ok, nil) sess.Unlock() diff --git a/session_test.go b/session_test.go index 3291fb5f..5c50dd35 100644 --- a/session_test.go +++ b/session_test.go @@ -7,6 +7,7 @@ import ( "io" "net" "testing" + "time" gossh "golang.org/x/crypto/ssh" ) @@ -466,3 +467,170 @@ func TestBreakWithoutChanRegistered(t *testing.T) { t.Fatalf("expected nil but got %v", err) } } + +// sendRequestWithTimeout sends a global-free session request and waits +// for its reply, failing the test if the server does not answer in +// time. It guards against regressions that park the session request +// loop, which would otherwise hang the test until the global timeout. +func sendRequestWithTimeout(t *testing.T, session *gossh.Session, name string, payload []byte, timeout time.Duration) (bool, error) { + t.Helper() + type result struct { + ok bool + err error + } + ch := make(chan result, 1) + go func() { + ok, err := session.SendRequest(name, true, payload) + ch <- result{ok, err} + }() + select { + case r := <-ch: + return r.ok, r.err + case <-time.After(timeout): + return false, fmt.Errorf("timed out waiting for %q reply", name) + } +} + +// TestWindowChangeWhenConsumerNotDraining verifies that window-change +// requests never park the session request loop, even when the session +// handler never reads the window size channel. Parking the loop would +// leak one goroutine and its session state per session channel, even +// after the client disconnects. +func TestWindowChangeWhenConsumerNotDraining(t *testing.T) { + t.Parallel() + + handlerDone := make(chan bool) + session, _, cleanup := newTestSession(t, &Server{ + Handler: func(s Session) { + s.Pty() // accept the pty, but never drain the window channel + <-handlerDone + }, + }, nil) + defer cleanup() + + if err := session.RequestPty("xterm", 80, 40, gossh.TerminalModes{}); err != nil { + t.Fatalf("expected nil but got %v", err) + } + if err := session.Shell(); err != nil { + t.Fatalf("expected nil but got %v", err) + } + + // The window channel is buffered with capacity one and already + // holds the initial pty window size, so this first request must be + // answered even though no consumer is reading anything. + winchMsg := struct{ W, H, Wpx, Hpx uint32 }{800, 600, 6400, 4800} + ok, err := sendRequestWithTimeout(t, session, "window-change", gossh.Marshal(&winchMsg), 10*time.Second) + if err != nil { + t.Fatalf("expected nil but got %v", err) + } + if !ok { + t.Fatalf("expected true but got %v", ok) + } + + // A burst of window-changes must not wedge the request loop either. + for i := 0; i < 50; i++ { + winchMsg = struct{ W, H, Wpx, Hpx uint32 }{uint32(100 + i), 200, 0, 0} + if _, err := session.SendRequest("window-change", false, gossh.Marshal(&winchMsg)); err != nil { + t.Fatalf("expected nil but got %v", err) + } + } + + // The request loop must still be able to process further requests. + winchMsg = struct{ W, H, Wpx, Hpx uint32 }{900, 700, 7200, 5600} + ok, err = sendRequestWithTimeout(t, session, "window-change", gossh.Marshal(&winchMsg), 10*time.Second) + if err != nil { + t.Fatalf("expected nil but got %v", err) + } + if !ok { + t.Fatalf("expected true but got %v", ok) + } + + close(handlerDone) +} + +// TestBreakWhenConsumerNotDraining verifies that break requests do not +// park the session request loop when the registered break channel is +// never drained; the request is answered with a failure reply instead. +func TestBreakWhenConsumerNotDraining(t *testing.T) { + t.Parallel() + + breakChan := make(chan bool) // unbuffered, never read + readyToRegisterBreak := make(chan bool) + handlerDone := make(chan bool) + + session, _, cleanup := newTestSession(t, &Server{ + Handler: func(s Session) { + s.Break(breakChan) + readyToRegisterBreak <- true + <-handlerDone + }, + }, nil) + defer cleanup() + + var stdout bytes.Buffer + session.Stdout = &stdout + errChan := make(chan error, 1) + go func() { + errChan <- session.Run("") + }() + + <-readyToRegisterBreak + // Break requests must be answered (with ok=false, since the + // consumer is not reading) instead of parking the request loop. + for i := 0; i < 5; i++ { + ok, err := sendRequestWithTimeout(t, session, "break", nil, 10*time.Second) + if err != nil { + t.Fatalf("expected nil but got %v", err) + } + if ok { + t.Fatalf("expected false but got %v", ok) + } + } + + close(handlerDone) + if err := <-errChan; err != nil { + t.Fatalf("expected nil but got %v", err) + } +} + +// TestEnvironBounded verifies that a client cannot force unbounded +// memory growth by sending an endless stream of env requests: only the +// first maxEnvs variables are accepted, the rest are rejected. +func TestEnvironBounded(t *testing.T) { + t.Parallel() + + envCh := make(chan []string, 1) + handlerDone := make(chan bool) + + session, _, cleanup := newTestSession(t, &Server{ + Handler: func(s Session) { + envCh <- s.Environ() + <-handlerDone + }, + }, nil) + defer cleanup() + + for i := 0; i < maxEnvs+10; i++ { + payload := gossh.Marshal(struct{ Key, Value string }{"KEY", fmt.Sprint(i)}) + ok, err := sendRequestWithTimeout(t, session, "env", payload, 10*time.Second) + if err != nil { + t.Fatalf("env %d: expected nil but got %v", i, err) + } + if i < maxEnvs && !ok { + t.Fatalf("env %d: expected true but got %v", i, ok) + } + if i >= maxEnvs && ok { + t.Fatalf("env %d: expected false but got %v", i, ok) + } + } + + if err := session.Shell(); err != nil { + t.Fatalf("expected nil but got %v", err) + } + env := <-envCh + if len(env) != maxEnvs { + t.Fatalf("expected %d environment entries but got %d", maxEnvs, len(env)) + } + + close(handlerDone) +}