From 280aa188d8e019bad9b7768251d9b968f452615b Mon Sep 17 00:00:00 2001 From: Viktor Liu <17948409+lixmal@users.noreply.github.com> Date: Mon, 26 Oct 2026 19:35:02 +0900 Subject: [PATCH] [client] Validate and bound SSH session requests and channels (#8344) --- .../ssh/server/command_execution_windows.go | 23 +- client/ssh/server/server.go | 21 +- client/ssh/server/session_hardening_test.go | 340 ++++++++++++++++++ client/ssh/server/session_limits.go | 246 +++++++++++++ client/ssh/server/session_limits_test.go | 82 +++++ go.mod | 2 +- go.sum | 4 +- 7 files changed, 709 insertions(+), 9 deletions(-) create mode 100644 client/ssh/server/session_hardening_test.go create mode 100644 client/ssh/server/session_limits.go create mode 100644 client/ssh/server/session_limits_test.go diff --git a/client/ssh/server/command_execution_windows.go b/client/ssh/server/command_execution_windows.go index feb8daa26..54981276a 100644 --- a/client/ssh/server/command_execution_windows.go +++ b/client/ssh/server/command_execution_windows.go @@ -267,7 +267,7 @@ func (s *Server) prepareCommandEnv(logger *log.Entry, localUser *user.User, sess return env } -func (s *Server) handlePtyLogin(logger *log.Entry, session ssh.Session, privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, _ <-chan ssh.Window) bool { +func (s *Server) handlePtyLogin(logger *log.Entry, session ssh.Session, privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, winCh <-chan ssh.Window) bool { if privilegeResult.User == nil { logger.Errorf("no user in privilege result") return false @@ -276,7 +276,7 @@ func (s *Server) handlePtyLogin(logger *log.Entry, session ssh.Session, privileg shell := getUserShell(privilegeResult.User.Uid) logger.Infof("starting interactive shell: %s", shell) - s.executeCommandWithPty(logger, session, nil, privilegeResult, ptyReq, nil) + s.executeCommandWithPty(logger, session, nil, privilegeResult, ptyReq, winCh) return true } @@ -376,7 +376,9 @@ func (s *Server) detectUtilLinuxLogin(context.Context) bool { } // executeCommandWithPty executes a command with PTY allocation on Windows using ConPty -func (s *Server) executeCommandWithPty(logger *log.Entry, session ssh.Session, _ *exec.Cmd, privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, _ <-chan ssh.Window) bool { +func (s *Server) executeCommandWithPty(logger *log.Entry, session ssh.Session, _ *exec.Cmd, privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, winCh <-chan ssh.Window) bool { + discardWindowChanges(winCh) + localUser := privilegeResult.User if localUser == nil { logger.Errorf("no user in privilege result") @@ -406,3 +408,18 @@ func (s *Server) executeCommandWithPty(logger *log.Entry, session ssh.Session, _ logger.Debug("ConPty execution completed") return true } + +// discardWindowChanges drains the session's window-change channel. The ConPty is sized +// once at start, and an undrained channel blocks the session's request loop on the +// next resize. +func discardWindowChanges(winCh <-chan ssh.Window) { + if winCh == nil { + return + } + go func() { + for range winCh { + // The ConPty keeps the size it was started with. + continue + } + }() +} diff --git a/client/ssh/server/server.go b/client/ssh/server/server.go index a33784329..b850c8e70 100644 --- a/client/ssh/server/server.go +++ b/client/ssh/server/server.go @@ -58,6 +58,10 @@ const ( // the forwarded destination before rejecting the SSH channel. const directTCPIPDialTimeout = 30 * time.Second +// defaultHandshakeTimeout bounds how long a connection may take to complete the SSH +// handshake and authentication before it is dropped. +const defaultHandshakeTimeout = 60 * time.Second + var ( ErrPrivilegedUserDisabled = errors.New(msgPrivilegedUserDisabled) ErrUserNotFound = errors.New("user not found") @@ -165,6 +169,7 @@ type Server struct { allowRemotePortForwarding bool allowRootLogin bool allowSFTP bool + handshakeTimeout time.Duration jwtEnabled bool netstackNet *wgbackend.NetstackNet @@ -227,6 +232,7 @@ func New(config *Config) *Server { jwtEnabled: config.JWT != nil, jwtConfig: config.JWT, authorizer: sshauth.NewAuthorizer(), + handshakeTimeout: defaultHandshakeTimeout, } if config.Auth != nil { @@ -884,15 +890,16 @@ func (s *Server) createSSHServer(addr net.Addr) (*ssh.Server, error) { }, HostSigners: []ssh.Signer{}, ChannelHandlers: map[string]ssh.ChannelHandler{ - "session": ssh.DefaultSessionHandler, + "session": s.sessionChannelHandler, "direct-tcpip": s.directTCPIPHandler, }, RequestHandlers: map[string]ssh.RequestHandler{ "tcpip-forward": s.tcpipForwardHandler, "cancel-tcpip-forward": s.cancelTcpipForwardHandler, }, - ConnCallback: s.connectionValidator, - Version: serverVersion, + ConnCallback: s.connectionValidator, + HandshakeTimeout: s.handshakeTimeout, + Version: serverVersion, } if s.jwtEnabled { @@ -934,6 +941,14 @@ func (s *Server) removeRemoteForwardListener(key forwardKey) bool { func (s *Server) directTCPIPHandler(srv *ssh.Server, conn *cryptossh.ServerConn, newChan cryptossh.NewChannel, ctx ssh.Context) { logger := s.getRequestLogger(ctx) + release, ok := acquireConnSlot(ctx, directTCPIPCountKey{}, maxDirectTCPIPPerConn) + if !ok { + logger.Warnf("local port forwarding denied: too many forwarded connections") + _ = newChan.Reject(cryptossh.ResourceShortage, "too many forwarded connections") + return + } + defer release() + var payload struct { Host string Port uint32 diff --git a/client/ssh/server/session_hardening_test.go b/client/ssh/server/session_hardening_test.go new file mode 100644 index 000000000..ed107a32a --- /dev/null +++ b/client/ssh/server/session_hardening_test.go @@ -0,0 +1,340 @@ +//go:build !js + +package server + +import ( + "bytes" + "encoding/binary" + "errors" + "io" + "net" + "os/user" + "runtime" + "strings" + "testing" + "time" + + cryptossh "golang.org/x/crypto/ssh" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + nbssh "github.com/netbirdio/netbird/client/ssh" +) + +func dialNoAuthServer(t *testing.T, addr string, hostKey []byte) *cryptossh.Client { + t.Helper() + + clientKey, err := nbssh.GeneratePrivateKey(nbssh.ED25519) + require.NoError(t, err) + signer, err := cryptossh.ParsePrivateKey(clientKey) + require.NoError(t, err) + hostSigner, err := cryptossh.ParsePrivateKey(hostKey) + require.NoError(t, err) + current, err := user.Current() + require.NoError(t, err) + + client, err := cryptossh.Dial("tcp", addr, &cryptossh.ClientConfig{ + User: current.Username, + Auth: []cryptossh.AuthMethod{cryptossh.PublicKeys(signer)}, + HostKeyCallback: cryptossh.FixedHostKey(hostSigner.PublicKey()), + Timeout: 3 * time.Second, + }) + require.NoError(t, err) + t.Cleanup(func() { _ = client.Close() }) + return client +} + +func startNoAuthServer(t *testing.T) (string, []byte) { + t.Helper() + + hostKey, err := nbssh.GeneratePrivateKey(nbssh.ED25519) + require.NoError(t, err) + srv := New(&Config{HostKeyPEM: hostKey}) + srv.SetAllowRootLogin(true) + addr := StartTestServer(t, srv) + t.Cleanup(func() { require.NoError(t, srv.Stop()) }) + return addr, hostKey +} + +// ptyReqPayload builds a pty-req payload whose term string claims the given length. +func ptyReqPayload(termLen uint32, term string) []byte { + buf := make([]byte, 4, 64) + binary.BigEndian.PutUint32(buf, termLen) + buf = append(buf, term...) + buf = append(buf, make([]byte, 16)...) + return buf +} + +// A pty-req with a term length close to the uint32 maximum must be refused and must +// not take the daemon down. +func TestSession_MalformedPtyReqDoesNotCrashServer(t *testing.T) { + addr, hostKey := startNoAuthServer(t) + client := dialNoAuthServer(t, addr, hostKey) + + session, err := client.NewSession() + require.NoError(t, err) + + ok, err := session.SendRequest("pty-req", true, ptyReqPayload(0xFFFFFFFF, "xterm")) + require.NoError(t, err) + assert.False(t, ok, "malformed pty-req should be rejected") + _ = session.Close() + + // The server must still serve a fresh connection. + again := dialNoAuthServer(t, addr, hostKey) + s2, err := again.NewSession() + require.NoError(t, err) + require.NoError(t, s2.Close()) +} + +// Env requests past the per-session cap are refused in order, and the ones before it +// are still accepted. +func TestSession_EnvRequestsAreCapped(t *testing.T) { + addr, hostKey := startNoAuthServer(t) + client := dialNoAuthServer(t, addr, hostKey) + + session, err := client.NewSession() + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + payload := cryptossh.Marshal(struct{ Key, Value string }{"FOO", "bar"}) + accepted := 0 + for i := 0; i < maxEnvRequests+50; i++ { + ok, err := session.SendRequest("env", true, payload) + require.NoError(t, err) + if ok { + accepted++ + } + } + assert.Equal(t, maxEnvRequests, accepted, "only the first maxEnvRequests env requests should be accepted") + + oversized := cryptossh.Marshal(struct{ Key, Value string }{"BIG", string(make([]byte, maxEnvPayload))}) + fresh, err := client.NewSession() + require.NoError(t, err) + t.Cleanup(func() { _ = fresh.Close() }) + ok, err := fresh.SendRequest("env", true, oversized) + require.NoError(t, err) + assert.False(t, ok, "an env request over the payload limit should be refused") +} + +// A flood of window-change requests before the command starts must not stall the +// session's request loop: the next request still gets its reply. +func TestSession_WindowChangeFloodDoesNotStallRequests(t *testing.T) { + addr, hostKey := startNoAuthServer(t) + client := dialNoAuthServer(t, addr, hostKey) + + session, err := client.NewSession() + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + require.NoError(t, session.RequestPty("xterm", 24, 80, cryptossh.TerminalModes{})) + + winch := cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{100, 40, 0, 0}) + for i := 0; i < 500; i++ { + _, err := session.SendRequest("window-change", false, winch) + require.NoError(t, err) + } + + done := make(chan bool, 1) + go func() { + ok, err := session.SendRequest("env", true, cryptossh.Marshal(struct{ Key, Value string }{"A", "b"})) + done <- err == nil && ok + }() + select { + case ok := <-done: + assert.True(t, ok, "env request after the flood should be accepted") + case <-time.After(5 * time.Second): + t.Fatal("session request loop stalled behind window-change requests") + } +} + +func TestSession_PerConnectionSessionLimit(t *testing.T) { + addr, hostKey := startNoAuthServer(t) + client := dialNoAuthServer(t, addr, hostKey) + + for i := 0; i < maxSessionsPerConn; i++ { + s, err := client.NewSession() + require.NoError(t, err, "session %d should open", i) + t.Cleanup(func() { _ = s.Close() }) + } + + _, err := client.NewSession() + assert.Error(t, err, "session beyond the limit should be refused") + + // The limit is per connection: another connection is unaffected. + other := dialNoAuthServer(t, addr, hostKey) + s, err := other.NewSession() + require.NoError(t, err, "a second connection should still open sessions") + require.NoError(t, s.Close()) +} + +func TestValidPtyReqPayload(t *testing.T) { + assert.True(t, validPtyReqPayload(ptyReqPayload(5, "xterm"))) + assert.False(t, validPtyReqPayload(nil), "empty payload") + assert.False(t, validPtyReqPayload([]byte{0, 0, 0}), "short length prefix") + assert.False(t, validPtyReqPayload(ptyReqPayload(0xFFFFFFFF, "xterm")), "length that wraps uint32") + assert.False(t, validPtyReqPayload(ptyReqPayload(1000, "xterm")), "length beyond the payload") +} + +// A resize sent while the command runs reaches the pty. +func TestSession_ResizeWhileRunningIsApplied(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("stty is not available") + } + addr, hostKey := startNoAuthServer(t) + client := dialNoAuthServer(t, addr, hostKey) + + session, err := client.NewSession() + require.NoError(t, err) + t.Cleanup(func() { _ = session.Close() }) + + var out bytes.Buffer + session.Stdout = &out + require.NoError(t, session.RequestPty("xterm", 24, 80, cryptossh.TerminalModes{})) + require.NoError(t, session.Start("sleep 1; stty size")) + + // Let the handler read the initial window before resizing. + time.Sleep(300 * time.Millisecond) + require.NoError(t, session.WindowChange(50, 132)) + require.NoError(t, session.Wait()) + // Only the first line is the resize report. Coverage-instrumented binaries can append + // runtime warnings to the same output. + firstLine := strings.TrimSpace(strings.SplitN(out.String(), "\n", 2)[0]) + assert.Equal(t, "50 132", firstLine, "pty should have the resized dimensions") +} + +// Forwarded connections beyond the per-connection cap are refused, and a closed one +// frees its slot. +func TestDirectTCPIP_PerConnectionLimit(t *testing.T) { + hostKey, err := nbssh.GeneratePrivateKey(nbssh.ED25519) + require.NoError(t, err) + srv := New(&Config{HostKeyPEM: hostKey}) + srv.SetAllowRootLogin(true) + srv.SetAllowLocalPortForwarding(true) + addr := StartTestServer(t, srv) + t.Cleanup(func() { require.NoError(t, srv.Stop()) }) + + // Target that holds each connection open until the peer half-closes, then closes it + // like a regular server. + target, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = target.Close() }) + go func() { + for { + c, err := target.Accept() + if err != nil { + return + } + go func() { + _, _ = io.Copy(io.Discard, c) + _ = c.Close() + }() + } + }() + + client := dialNoAuthServer(t, addr, hostKey) + + conns := make([]net.Conn, 0, maxDirectTCPIPPerConn) + for i := 0; i < maxDirectTCPIPPerConn; i++ { + c, err := client.Dial("tcp", target.Addr().String()) + require.NoError(t, err, "forward %d should open", i) + conns = append(conns, c) + } + + _, err = client.Dial("tcp", target.Addr().String()) + assert.Error(t, err, "forward beyond the limit should be refused") + + // The limit is per connection: another connection is unaffected. + other := dialNoAuthServer(t, addr, hostKey) + oc, err := other.Dial("tcp", target.Addr().String()) + require.NoError(t, err, "a second connection should still open forwards") + require.NoError(t, oc.Close()) + + require.NoError(t, conns[0].Close()) + assert.Eventually(t, func() bool { + c, err := client.Dial("tcp", target.Addr().String()) + if err != nil { + return false + } + _ = c.Close() + return true + }, 3*time.Second, 50*time.Millisecond, "a closed forward should free its slot") + + for _, c := range conns[1:] { + _ = c.Close() + } +} + +// A connection that never completes the SSH handshake is dropped, and one that does +// complete it is not subject to the handshake deadline afterwards. +func TestHandshakeTimeout(t *testing.T) { + hostKey, err := nbssh.GeneratePrivateKey(nbssh.ED25519) + require.NoError(t, err) + srv := New(&Config{HostKeyPEM: hostKey}) + srv.handshakeTimeout = 500 * time.Millisecond + addr := StartTestServer(t, srv) + t.Cleanup(func() { require.NoError(t, srv.Stop()) }) + + stalled, err := net.Dial("tcp", addr) + require.NoError(t, err) + t.Cleanup(func() { _ = stalled.Close() }) + + // The server sends its version line first; after that it waits for ours. + require.NoError(t, stalled.SetReadDeadline(time.Now().Add(5*time.Second))) + _, err = io.Copy(io.Discard, stalled) + var nerr net.Error + assert.False(t, errors.As(err, &nerr) && nerr.Timeout(), "server should close the stalled connection, not leave it open") + + client := dialNoAuthServer(t, addr, hostKey) + time.Sleep(time.Second) + session, err := client.NewSession() + require.NoError(t, err, "an established connection must outlive the handshake deadline") + require.NoError(t, session.Close()) +} + +// Refused requests are forwarded to the session loop under a type it answers with a +// failure, so every reply is sent by the loop in request order. +func TestFilterSessionRequests_RefusedRequestsStayInOrder(t *testing.T) { + in := make(chan *cryptossh.Request) + out := filterSessionRequests(nil, in) + + validWinch := cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{100, 40, 0, 0}) + reqs := []*cryptossh.Request{ + {Type: "pty-req", Payload: ptyReqPayload(0xFFFFFFFF, "xterm")}, + {Type: "pty-req", Payload: ptyReqPayload(5, "xterm")}, + {Type: "window-change", Payload: validWinch}, + {Type: "shell"}, + {Type: "window-change", Payload: validWinch}, + {Type: "window-change", Payload: validWinch}, + } + want := []string{ + refusedRequestType, // malformed pty-req + "pty-req", + refusedRequestType, // resize before the command starts + "shell", + refusedRequestType, // older of two queued resizes + "window-change", + } + + // Hold all requests in the filter's queue before the loop consumes any, so the + // coalescing of the two trailing resizes is exercised. + for _, r := range reqs { + in <- r + } + for i, w := range want { + select { + case got := <-out: + assert.Equal(t, w, got.Type, "request %d", i) + case <-time.After(2 * time.Second): + t.Fatalf("request %d not delivered", i) + } + } + + close(in) + select { + case _, ok := <-out: + assert.False(t, ok, "filter should close its output when the input closes") + case <-time.After(2 * time.Second): + t.Fatal("filter did not close its output") + } +} diff --git a/client/ssh/server/session_limits.go b/client/ssh/server/session_limits.go new file mode 100644 index 000000000..68850e4bd --- /dev/null +++ b/client/ssh/server/session_limits.go @@ -0,0 +1,246 @@ +package server + +import ( + "encoding/binary" + "math" + "sync/atomic" + + "github.com/gliderlabs/ssh" + log "github.com/sirupsen/logrus" + cryptossh "golang.org/x/crypto/ssh" +) + +const ( + // maxSessionsPerConn bounds concurrent session channels on one SSH connection. + maxSessionsPerConn = 64 + + // maxDirectTCPIPPerConn bounds concurrent direct-tcpip channels, dialing or + // relaying, on one SSH connection. + maxDirectTCPIPPerConn = 512 + + // maxEnvRequests and maxEnvPayload bound the environment a single session can + // accumulate before its command starts. + maxEnvRequests = 128 + maxEnvPayload = 8 * 1024 + + // maxQueuedRequests bounds the requests held for a session whose request loop is + // not consuming them. A client that exceeds it has its channel closed. + maxQueuedRequests = 256 +) + +type ( + sessionCountKey struct{} + directTCPIPCountKey struct{} +) + +// sessionChannelHandler serves "session" channels like ssh.DefaultSessionHandler, with +// the per-connection session count capped, the request stream bounded, and any panic +// from the session request loop contained to its channel. +func (s *Server) sessionChannelHandler(srv *ssh.Server, conn *cryptossh.ServerConn, newChan cryptossh.NewChannel, ctx ssh.Context) { + release, ok := acquireConnSlot(ctx, sessionCountKey{}, maxSessionsPerConn) + if !ok { + if err := newChan.Reject(cryptossh.ResourceShortage, "too many sessions"); err != nil { + log.Debugf("reject session channel: %v", err) + } + return + } + defer release() + + guarded := &guardedSessionChannel{NewChannel: newChan} + defer func() { + r := recover() + if r == nil { + return + } + log.Errorf("SSH session request loop panicked from %s: %v", ctx.RemoteAddr(), r) + guarded.closeChannel() + }() + + ssh.DefaultSessionHandler(srv, conn, guarded, ctx) +} + +// acquireConnSlot takes one of limit slots counted under key for the connection that +// ctx belongs to. The returned func releases the slot. +func acquireConnSlot(ctx ssh.Context, key any, limit int32) (func(), bool) { + counter := connCounter(ctx, key) + if counter.Add(1) > limit { + counter.Add(-1) + return nil, false + } + return func() { counter.Add(-1) }, true +} + +func connCounter(ctx ssh.Context, key any) *atomic.Int32 { + ctx.Lock() + defer ctx.Unlock() + + if c, ok := ctx.Value(key).(*atomic.Int32); ok { + return c + } + c := new(atomic.Int32) + ctx.SetValue(key, c) + return c +} + +// guardedSessionChannel hands the session loop a filtered request stream. +type guardedSessionChannel struct { + cryptossh.NewChannel + ch atomic.Pointer[cryptossh.Channel] +} + +// Accept accepts the channel and returns its requests after filtering. +func (g *guardedSessionChannel) Accept() (cryptossh.Channel, <-chan *cryptossh.Request, error) { + ch, reqs, err := g.NewChannel.Accept() + if err != nil { + return nil, nil, err + } + g.ch.Store(&ch) + return ch, filterSessionRequests(ch, reqs), nil +} + +func (g *guardedSessionChannel) closeChannel() { + if ch := g.ch.Load(); ch != nil { + _ = (*ch).Close() + } +} + +// refusedRequestType is the type given to a request the filter refuses. The session +// loop answers unknown request types with a failure reply, so a refused request is +// answered by the loop itself and in its position among the replies, which the +// protocol requires since replies carry no request identifier. +const refusedRequestType = "refused@netbird.io" + +// requestFilter passes session channel requests to the session loop while keeping +// what the loop can accumulate or block on bounded: +// - a malformed pty-req is refused instead of reaching the loop's parser +// - env requests beyond maxEnvRequests, or with an oversized payload, are refused +// - window-change requests are refused until the command starts and coalesced +// after, as the loop blocks sending on a one-slot channel that only the command +// handler drains +// - the queue behind a stalled loop is capped, after which the channel is closed +type requestFilter struct { + ch cryptossh.Channel + queue []*cryptossh.Request + lastWinch *cryptossh.Request + started bool + envCount int +} + +// filterSessionRequests returns the requests of in as the session loop should see them. +func filterSessionRequests(ch cryptossh.Channel, in <-chan *cryptossh.Request) <-chan *cryptossh.Request { + out := make(chan *cryptossh.Request) + f := &requestFilter{ch: ch} + go func() { + defer close(out) + f.run(in, out) + }() + return out +} + +func (f *requestFilter) run(in <-chan *cryptossh.Request, out chan<- *cryptossh.Request) { + for { + var ( + sendCh chan<- *cryptossh.Request + next *cryptossh.Request + ) + if len(f.queue) > 0 { + sendCh = out + next = f.queue[0] + } + + select { + case req, ok := <-in: + if !ok { + return + } + if !f.enqueue(req) { + return + } + case sendCh <- next: + f.dropHead() + } + } +} + +func (f *requestFilter) dropHead() { + if f.queue[0] == f.lastWinch { + f.lastWinch = nil + } + f.queue = f.queue[1:] +} + +// enqueue classifies req and queues it. It returns false after closing the channel +// because the queue is full. +func (f *requestFilter) enqueue(req *cryptossh.Request) bool { + if len(f.queue) >= maxQueuedRequests { + log.Warnf("SSH session request queue full, closing channel") + _ = f.ch.Close() + return false + } + + switch req.Type { + case "pty-req": + if !validPtyReqPayload(req.Payload) { + refuseRequest(req) + } + case "env": + f.envCount++ + if f.envCount > maxEnvRequests || len(req.Payload) > maxEnvPayload { + refuseRequest(req) + } + case "shell", "exec", "subsystem": + f.started = true + case "window-change": + f.coalesceWindowChange(req) + } + f.queue = append(f.queue, req) + return true +} + +func (f *requestFilter) coalesceWindowChange(req *cryptossh.Request) { + if !f.started || !validWinchPayload(req.Payload) { + refuseRequest(req) + return + } + if f.lastWinch != nil { + refuseRequest(f.lastWinch) + } + f.lastWinch = req +} + +// validWinchPayload reports whether p parses as a window-change payload whose columns and +// rows are non-zero and fit the uint16 the PTY takes. A malformed or unusable replacement +// must not displace a valid queued resize. +func validWinchPayload(p []byte) bool { + var win struct{ Columns, Rows, Width, Height uint32 } + if err := cryptossh.Unmarshal(p, &win); err != nil { + return false + } + return usablePtyDimension(win.Columns) && usablePtyDimension(win.Rows) +} + +func usablePtyDimension(v uint32) bool { + return v > 0 && v <= math.MaxUint16 +} + +func refuseRequest(req *cryptossh.Request) { + req.Type = refusedRequestType + req.Payload = nil +} + +// validPtyReqPayload reports whether p holds a term string followed by the two +// uint32 window dimensions, without overflowing on a hostile string length. The +// dimensions must fit the Windows int16 ConPty coordinate, which is narrower than the +// Unix uint16 winsize; zero is allowed and defaulted by the Unix path. +func validPtyReqPayload(p []byte) bool { + if len(p) < 4 { + return false + } + termLen := uint64(binary.BigEndian.Uint32(p)) + if uint64(len(p)) < 4+termLen+8 { + return false + } + dims := p[4+termLen:] + return binary.BigEndian.Uint32(dims) <= math.MaxInt16 && + binary.BigEndian.Uint32(dims[4:]) <= math.MaxInt16 +} diff --git a/client/ssh/server/session_limits_test.go b/client/ssh/server/session_limits_test.go new file mode 100644 index 000000000..8c2a35d37 --- /dev/null +++ b/client/ssh/server/session_limits_test.go @@ -0,0 +1,82 @@ +package server + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + cryptossh "golang.org/x/crypto/ssh" +) + +// A malformed window-change arriving after a valid one must not cost the valid resize: +// the valid request stays queued for the PTY, and only the malformed one is refused. +func TestRequestFilter_MalformedWinchKeepsQueuedValidResize(t *testing.T) { + f := &requestFilter{} + require.True(t, f.enqueue(&cryptossh.Request{Type: "shell"})) + + valid := &cryptossh.Request{Type: "window-change", Payload: cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{100, 40, 0, 0})} + require.True(t, f.enqueue(valid)) + + malformed := &cryptossh.Request{Type: "window-change", Payload: []byte{1, 2, 3}} + require.True(t, f.enqueue(malformed)) + + assert.Equal(t, "window-change", valid.Type, "valid resize must still reach the PTY handler") + assert.Equal(t, refusedRequestType, malformed.Type, "malformed resize must be refused") +} + +// A zero-row or zero-column resize parses but would size the PTY to nothing, so it must +// not displace a valid queued resize either. +func TestRequestFilter_ZeroSizeWinchKeepsQueuedValidResize(t *testing.T) { + f := &requestFilter{} + require.True(t, f.enqueue(&cryptossh.Request{Type: "shell"})) + + valid := &cryptossh.Request{Type: "window-change", Payload: cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{100, 40, 0, 0})} + require.True(t, f.enqueue(valid)) + + zeroRows := &cryptossh.Request{Type: "window-change", Payload: cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{100, 0, 0, 0})} + require.True(t, f.enqueue(zeroRows)) + + assert.Equal(t, "window-change", valid.Type, "valid resize must still reach the PTY handler") + assert.Equal(t, refusedRequestType, zeroRows.Type, "zero-row resize must be refused") +} + +// The initial pty-req size must fit both the Unix uint16 winsize and the Windows int16 +// ConPty coordinate, so larger values are refused instead of wrapping. Zero stays +// accepted because the Unix path defaults it to 80x24. +func TestValidPtyReqPayload_BoundsInitialSize(t *testing.T) { + ptyReq := func(columns, rows uint32) []byte { + return cryptossh.Marshal(struct { + Term string + Columns, Rows, Width, Height uint32 + Modes string + }{"xterm", columns, rows, 0, 0, ""}) + } + + assert.True(t, validPtyReqPayload(ptyReq(80, 24)), "ordinary size") + assert.True(t, validPtyReqPayload(ptyReq(0, 0)), "zero size is defaulted later") + assert.True(t, validPtyReqPayload(ptyReq(32767, 32767)), "largest size both platforms take") + assert.False(t, validPtyReqPayload(ptyReq(32768, 24)), "columns past the Windows int16 range") + assert.False(t, validPtyReqPayload(ptyReq(80, 32768)), "rows past the Windows int16 range") + assert.False(t, validPtyReqPayload(ptyReq(65536, 24)), "columns that wrap to zero as uint16") +} + +// Sizes that are zero or do not fit the PTY's uint16 dimensions are refused, so they +// cannot replace a valid queued resize. +func TestValidWinchPayload_RejectsUnusableSizes(t *testing.T) { + tests := []struct { + name string + columns, rows uint32 + }{ + {name: "zero columns", columns: 0, rows: 40}, + {name: "zero rows", columns: 100, rows: 0}, + {name: "columns wrap to zero as uint16", columns: 65536, rows: 40}, + {name: "rows wrap to zero as uint16", columns: 100, rows: 65536}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + payload := cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{tc.columns, tc.rows, 0, 0}) + assert.False(t, validWinchPayload(payload)) + }) + } + assert.True(t, validWinchPayload(cryptossh.Marshal(struct{ Columns, Rows, Width, Height uint32 }{65535, 65535, 0, 0})), "largest uint16 size is usable") +} diff --git a/go.mod b/go.mod index 0d1492731..699a553af 100644 --- a/go.mod +++ b/go.mod @@ -54,7 +54,7 @@ require ( github.com/eko/gocache/store/go_cache/v4 v4.2.2 github.com/eko/gocache/store/redis/v4 v4.2.2 github.com/fsnotify/fsnotify v1.9.0 - github.com/gliderlabs/ssh v0.3.8 + github.com/gliderlabs/ssh v0.3.9-0.20250127223818-909fa952d4e4 github.com/go-jose/go-jose/v4 v4.1.4 github.com/go-ole/go-ole v1.3.0 github.com/gobwas/ws v1.4.0 diff --git a/go.sum b/go.sum index a6d3dedca..de7b77649 100644 --- a/go.sum +++ b/go.sum @@ -200,8 +200,8 @@ github.com/fxamacker/cbor/v2 v2.9.1 h1:2rWm8B193Ll4VdjsJY28jxs70IdDsHRWgQYAI80+r github.com/fxamacker/cbor/v2 v2.9.1/go.mod h1:vM4b+DJCtHn+zz7h3FFp/hDAI9WNWCsZj23V5ytsSxQ= github.com/gin-contrib/sse v0.1.0/go.mod h1:RHrZQHXnP2xjPF+u1gW/2HnVO7nvIa9PG3Gm+fLHvGI= github.com/gin-gonic/gin v1.5.0/go.mod h1:Nd6IXA8m5kNZdNEHMBd93KT+mdY3+bewLgRvmCsR2Do= -github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= -github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= +github.com/gliderlabs/ssh v0.3.9-0.20250127223818-909fa952d4e4 h1:iLDHIkpEUuk277c53BF7v9yCAzgQHOcaY8tOu5ZX9Ws= +github.com/gliderlabs/ssh v0.3.9-0.20250127223818-909fa952d4e4/go.mod h1:1nh+SYql7iCMwTrbe9K8l/lah6xcgJTH7W4ZlVoq22c= github.com/go-asn1-ber/asn1-ber v1.5.8-0.20250403174932-29230038a667 h1:BP4M0CvQ4S3TGls2FvczZtj5Re/2ZzkV9VwqPHH/3Bo= github.com/go-asn1-ber/asn1-ber v1.5.8-0.20250403174932-29230038a667/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= github.com/go-git/gcfg v1.5.1-0.20230307220236-3a3c6141e376 h1:+zs/tPmkDkHx3U66DAb0lQFJrpS6731Oaa12ikc+DiI=