Address review findings on the VNC server, session auth and capture decoder

client/status/status.go

@@ -448,7 +448,7 @@ func (o *OutputOverview) YAML() (string, error) {
 }
 
 // GeneralSummary returns a general summary of the status overview.
-func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameServers bool, showSSHSessions bool) string {
+func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameServers bool, showSessions bool) string {
 	var managementConnString string
 	if o.ManagementState.Connected {
 		managementConnString = "Connected"
@@ -573,7 +573,7 @@ func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameS
 			sshServerStatus = "Enabled"
 		}
 
-		if showSSHSessions && sessionCount > 0 {
+		if showSessions && sessionCount > 0 {
 			for _, session := range o.SSHServerState.Sessions {
 				var sessionDisplay string
 				if session.JWTUsername != "" {
@@ -611,7 +611,7 @@ func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameS
 			vncServerStatus = "Enabled"
 		}
 
-		if showSSHSessions && vncSessionCount > 0 {
+		if showSessions && vncSessionCount > 0 {
 			for _, sess := range o.VNCServerState.Sessions {
 				vncServerStatus += "\n  " + formatVNCSessionLine(sess)
 			}

client/vnc/server/agent_windows.go

@@ -800,17 +800,16 @@ func relogAgentOutput(pipe windows.Handle) {
 // each call site, while still capturing diagnostic info when the OS reports
 // a failure.
 func logCleanupCall(name string, proc *windows.LazyProc) {
-	r, _, err := proc.Call()
+	logCleanupCallArgs(name, proc)
-	if r == 0 && err != nil && err != windows.NTE_OP_OK {
-		log.Tracef("%s: %v", name, err)
-	}
 }
 
-// logCleanupCallArgs is logCleanupCall with one argument; common pattern for
+// logCleanupCallArgs is logCleanupCall with arguments; common pattern for
 // release-by-handle syscalls.
 func logCleanupCallArgs(name string, proc *windows.LazyProc, args ...uintptr) {
 	r, _, err := proc.Call(args...)
-	if r == 0 && err != nil && err != windows.NTE_OP_OK {
+	// LazyProc.Call always returns a non-nil error carrying the thread's last
+	// error code, so a zero code is what "the call did not fail" looks like.
+	if r == 0 && !errors.Is(err, windows.ERROR_SUCCESS) {
 		log.Tracef("%s: %v", name, err)
 	}
 }

client/vnc/server/capture_x11_shm_linux.go

@@ -31,25 +31,27 @@ func (c *X11Capturer) initSHM() error {
 		return fmt.Errorf("shmat: %w", err)
 	}
 
-	if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil {
-		log.Debugf("shmctl IPC_RMID: %v", err)
-	}
-
 	seg, err := shm.NewSegId(c.conn)
 	if err != nil {
-		if detachErr := unix.SysvShmDetach(addr); detachErr != nil {
+		releaseShmSegment(id, addr)
-			log.Debugf("shmdt on new-seg failure: %v", detachErr)
-		}
 		return fmt.Errorf("new SHM seg: %w", err)
 	}
 
+	// The X server attaches before the segment is marked for deletion: since
+	// Linux 3.10 a shmat() against an IPC_RMID'd segment fails with EIDRM, so
+	// marking it first would push us onto the slow non-SHM path.
 	if err := shm.AttachChecked(c.conn, seg, uint32(id), false).Check(); err != nil {
-		if detachErr := unix.SysvShmDetach(addr); detachErr != nil {
+		releaseShmSegment(id, addr)
-			log.Debugf("shmdt on attach-checked failure: %v", detachErr)
-		}
 		return fmt.Errorf("SHM attach to X: %w", err)
 	}
 
+	// Both ends hold the segment at this point, so marking it for deletion
+	// frees it as soon as the last of them detaches, even if this process
+	// dies without cleaning up.
+	if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil {
+		log.Debugf("shmctl IPC_RMID: %v", err)
+	}
+
 	c.shmID = id
 	c.shmAddr = addr
 	c.shmSeg = uint32(seg)
@@ -57,6 +59,18 @@ func (c *X11Capturer) initSHM() error {
 	return nil
 }
 
+// releaseShmSegment gives a segment back on a setup path that failed after
+// attaching it: detach this process, then mark it for deletion so it does not
+// linger in the kernel's IPC table for the life of the host.
+func releaseShmSegment(id int, addr []byte) {
+	if err := unix.SysvShmDetach(addr); err != nil {
+		log.Debugf("shmdt on setup failure: %v", err)
+	}
+	if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil {
+		log.Debugf("shmctl IPC_RMID on setup failure: %v", err)
+	}
+}
+
 func (c *X11Capturer) captureSHM() (*image.RGBA, error) {
 	if err := c.fillSHM(); err != nil {
 		return nil, err

client/vnc/server/cursor_windows.go

@@ -120,28 +120,15 @@ func (s *cursorSampler) sample() (*cursorSnapshot, error) {
 		// cursor and stay armed for the next handle change rather than
 		// treating this as a hard failure that would latch us off for
 		// the session.
-		if s.lastHandle == hiddenHandle {
+		if s.lastHandle == hiddenHandle && s.snapshot != nil {
-			s.snapshot.posX = int(ci.PtPos.X)
+			return s.publish(*s.snapshot, ci), nil
-			s.snapshot.posY = int(ci.PtPos.Y)
-			s.snapshot.hasPos = true
-			return s.snapshot, nil
 		}
 		s.lastHandle = hiddenHandle
 		s.serial++
-		s.snapshot = &cursorSnapshot{
+		return s.publish(cursorSnapshot{img: transparentCursorImage(), serial: s.serial}, ci), nil
-			img:    transparentCursorImage(),
-			posX:   int(ci.PtPos.X),
-			posY:   int(ci.PtPos.Y),
-			hasPos: true,
-			serial: s.serial,
-		}
-		return s.snapshot, nil
 	}
 	if ci.Cursor == s.lastHandle && s.snapshot != nil {
-		s.snapshot.posX = int(ci.PtPos.X)
+		return s.publish(*s.snapshot, ci), nil
-		s.snapshot.posY = int(ci.PtPos.Y)
-		s.snapshot.hasPos = true
-		return s.snapshot, nil
 	}
 	img, hotX, hotY, err := decodeCursor(ci.Cursor)
 	if err != nil {
@@ -149,16 +136,21 @@ func (s *cursorSampler) sample() (*cursorSnapshot, error) {
 	}
 	s.lastHandle = ci.Cursor
 	s.serial++
-	s.snapshot = &cursorSnapshot{
+	return s.publish(cursorSnapshot{img: img, hotX: hotX, hotY: hotY, serial: s.serial}, ci), nil
-		img:    img,
+}
-		hotX:   hotX,
+
-		hotY:   hotY,
+// publish stamps the cursor's current position onto snap and stores it as the
-		posX:   int(ci.PtPos.X),
+// sampler's latest snapshot. A fresh value every time, never an update in
-		posY:   int(ci.PtPos.Y),
+// place: the session encoder reads the snapshot the sampler last handed out,
-		hasPos: true,
+// and must not see a position that is halfway between two samples. The sprite
-		serial: s.serial,
+// fields are carried over by value, so a snapshot the encoder still holds keeps
-	}
+// pointing at the same immutable image.
-	return s.snapshot, nil
+func (s *cursorSampler) publish(snap cursorSnapshot, ci winCursorInfo) *cursorSnapshot {
+	snap.posX = int(ci.PtPos.X)
+	snap.posY = int(ci.PtPos.Y)
+	snap.hasPos = true
+	s.snapshot = &snap
+	return s.snapshot
 }
 
 // decodeCursor extracts the sprite at hCur as RGBA along with the hotspot.

client/vnc/server/extclipboard_test.go

@@ -3,6 +3,8 @@
 package server
 
 import (
+	"bytes"
+	"compress/zlib"
 	"encoding/binary"
 	"strings"
 	"testing"
@@ -93,10 +95,43 @@ func TestExtClipProvideRoundTripLarge(t *testing.T) {
 	assert.Equal(t, original, text)
 }
 
-func TestParseExtClipProvideTextRejectsOversized(t *testing.T) {
+func TestParseExtClipProvideTextRejectsMalformedStream(t *testing.T) {
 	var fakePayload [4]byte
 	// 4 bytes of zlib-compressed garbage won't decode; we want to ensure we
 	// don't panic, not that we accept it.
 	_, err := parseExtClipProvideText(extClipActionProvide|extClipFormatText, fakePayload[:])
 	assert.Error(t, err)
 }
+
+// The size caps are what keep a peer from making us allocate a record of its
+// choosing, on either side of the wire.
+func TestParseExtClipProvideTextRejectsOversizedRecord(t *testing.T) {
+	// A well-formed stream whose declared record size is past the cap: the
+	// bytes behind it are never read, so the guard is the only thing that
+	// stops the allocation.
+	var body bytes.Buffer
+	var lenBuf [4]byte
+	binary.BigEndian.PutUint32(lenBuf[:], uint32(extClipMaxText)+1)
+	body.Write(lenBuf[:])
+
+	var compressed bytes.Buffer
+	zw := zlib.NewWriter(&compressed)
+	_, err := zw.Write(body.Bytes())
+	require.NoError(t, err)
+	require.NoError(t, zw.Close())
+
+	_, err = parseExtClipProvideText(extClipActionProvide|extClipFormatText, compressed.Bytes())
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "record too large")
+}
+
+func TestBuildExtClipProvideTextRejectsOversizedText(t *testing.T) {
+	// extClipMaxText itself is already one over: the builder appends a NUL
+	// terminator, and the length it writes counts it.
+	_, err := buildExtClipProvideText(strings.Repeat("a", extClipMaxText))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "exceeds extClipMaxText")
+
+	_, err = buildExtClipProvideText(strings.Repeat("a", extClipMaxText-1))
+	require.NoError(t, err, "one byte under the cap must still build")
+}

client/vnc/server/input_x11.go

@@ -32,9 +32,11 @@ type X11InputInjector struct {
 // NewX11InputInjector connects to the X11 display and initializes XTest.
 // Empty cookieHex/authFile fall back to XAUTHORITY env lookup.
 func NewX11InputInjector(display, cookieHex, authFile string) (*X11InputInjector, error) {
-	detectX11Display()
+	// Only probe for a display when the caller named none: detection writes
-
+	// DISPLAY and XAUTHORITY into this process's environment, which has no
+	// business changing when the caller already knows which display to use.
 	if display == "" {
+		detectX11Display()
 		display = os.Getenv(envDisplay)
 	}
 	if display == "" {

client/vnc/server/server.go

@@ -459,6 +459,16 @@ func (s *Server) trackConn(c net.Conn) {
 	s.sessionsMu.Unlock()
 }
 
+// retrackConn replaces a tracked raw connection with the wrapper its handler
+// will actually hold, so shutdown and the handler's own untrackConn agree on
+// which object is registered.
+func (s *Server) retrackConn(raw, wrapped net.Conn) {
+	s.sessionsMu.Lock()
+	delete(s.acceptedConns, raw)
+	s.acceptedConns[wrapped] = struct{}{}
+	s.sessionsMu.Unlock()
+}
+
 // untrackConn forgets a connection once its handler is returning.
 func (s *Server) untrackConn(c net.Conn) {
 	s.sessionsMu.Lock()
@@ -1046,9 +1056,19 @@ func (s *Server) acquireVirtualSession(conn net.Conn, header *connectionHeader,
 		return nil, nil, nil, false
 	}
 	vs.ClientConnect()
+	// GetOrCreate checks the session is alive, but nothing stops it being torn
+	// down between that check and here, and a nil capturer would only surface
+	// as a panic in the encoder.
+	capturer, injector := vs.Capturer(), vs.Injector()
+	if capturer == nil {
+		vs.ClientDisconnect()
+		rejectConnection(conn, codeMessage(RejectCodeSessionError, "virtual session stopped"))
+		(*connLog).Warnf("virtual session for %s stopped before the client attached", header.username)
+		return nil, nil, nil, false
+	}
 	*connLog = (*connLog).WithField("vnc_user", header.username)
 	(*connLog).Infof("session mode: user=%s display=%s", header.username, vs.Display())
-	return vs.Capturer(), vs.Injector(), vs.ClientDisconnect, true
+	return capturer, injector, vs.ClientDisconnect, true
 }
 
 // acquireAttachSession bumps the shared capturer's per-session refcount

client/vnc/server/server_darwin.go

@@ -49,19 +49,24 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 			continue
 		}
 
+		// Track before any early-reject path so a concurrent Stop's
+		// closeActiveSessions snapshot can never miss a just-accepted
+		// socket and let it survive shutdown.
+		s.trackConn(conn)
 		if !s.tryAcquireConnSlot() {
+			s.untrackConn(conn)
 			s.log.Warnf("rejecting VNC connection from %s: %d concurrent connections in flight", conn.RemoteAddr(), maxConcurrentVNCConns)
 			_ = conn.Close()
 			continue
 		}
 		enableTCPKeepAlive(conn, s.log)
-		conn = newMetricsConn(conn, s.sessionRecorder)
+		metered := newMetricsConn(conn, s.sessionRecorder)
-		s.trackConn(conn)
+		s.retrackConn(conn, metered)
 		go func(c net.Conn) {
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)
 			s.handleServiceConnection(c, mgr)
-		}(conn)
+		}(metered)
 	}
 }
 

client/vnc/server/server_windows.go

@@ -336,19 +336,24 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 			continue
 		}
 
+		// Track before any early-reject path so a concurrent Stop's
+		// closeActiveSessions snapshot can never miss a just-accepted
+		// socket and let it survive shutdown.
+		s.trackConn(conn)
 		if !s.tryAcquireConnSlot() {
+			s.untrackConn(conn)
 			s.log.Warnf("rejecting VNC connection from %s: %d concurrent connections in flight", conn.RemoteAddr(), maxConcurrentVNCConns)
 			_ = conn.Close()
 			continue
 		}
 		enableTCPKeepAlive(conn, s.log)
-		conn = newMetricsConn(conn, s.sessionRecorder)
+		metered := newMetricsConn(conn, s.sessionRecorder)
-		s.trackConn(conn)
+		s.retrackConn(conn, metered)
 		go func(c net.Conn) {
 			defer s.releaseConnSlot()
 			defer s.untrackConn(c)
 			s.handleServiceConnection(c, sm)
-		}(conn)
+		}(metered)
 	}
 }
 

client/vnc/server/session.go

@@ -4,6 +4,7 @@ package server
 
 import (
 	"encoding/binary"
+	"errors"
 	"fmt"
 	"image"
 	"io"
@@ -201,7 +202,9 @@ func (s *session) serve() {
 		<-encoderDone
 	}()
 
-	if err := s.messageLoop(); err != nil && err != io.EOF {
+	// messageLoop only ever returns an error, so the interesting question is
+	// which one: a clean client disconnect is io.EOF and not worth a warning.
+	if err := s.messageLoop(); !errors.Is(err, io.EOF) {
 		s.log.Warnf("client %s disconnected: %v", s.addr(), err)
 	} else {
 		s.log.Infof("client disconnected: %s", s.addr())

client/vnc/server/session_clipboard.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"time"
+	"unicode/utf8"
 )
 
 // clipboardPoll periodically checks the server-side clipboard and sends
@@ -192,8 +193,10 @@ func (s *session) handleExtClipProvide(flags uint32, payload []byte) {
 // host clipboard contents, capped to extClipMaxText.
 func (s *session) sendExtClipProvideText() error {
 	text := s.injector.GetClipboard()
-	if len(text) > extClipMaxText {
+	// One byte short of the cap: buildExtClipProvideText appends a NUL
-		text = text[:extClipMaxText]
+	// terminator, which counts against extClipMaxText.
+	if len(text) > extClipMaxText-1 {
+		text = trimPartialRune(text[:extClipMaxText-1])
 	}
 	payload, err := buildExtClipProvideText(text)
 	if err != nil {
@@ -202,6 +205,20 @@ func (s *session) sendExtClipProvideText() error {
 	return s.writeExtClipMessage(payload)
 }
 
+// trimPartialRune drops the trailing bytes a byte-length cut left halfway
+// through a UTF-8 rune, so the client is never handed invalid UTF-8. A real
+// U+FFFD in the text decodes as three bytes and is kept.
+func trimPartialRune(s string) string {
+	for s != "" {
+		r, size := utf8.DecodeLastRuneInString(s)
+		if r != utf8.RuneError || size > 1 {
+			return s
+		}
+		s = s[:len(s)-1]
+	}
+	return s
+}
+
 // writeExtClipMessage frames an ExtendedClipboard payload as a ServerCutText
 // message with a negative length, then writes it under writeMu.
 func (s *session) writeExtClipMessage(payload []byte) error {

client/vnc/server/virtual_x11.go

@@ -231,13 +231,23 @@ func (vs *VirtualSession) isAlive() bool {
 	return true
 }
 
-// Capturer returns the screen capturer for this virtual session.
+// Capturer returns the screen capturer for this virtual session, or nil once
+// Stop has torn it down. Read under vs.mu, which is what Stop writes it under.
 func (vs *VirtualSession) Capturer() ScreenCapturer {
+	vs.mu.Lock()
+	defer vs.mu.Unlock()
+
+	if vs.poller == nil {
+		return nil
+	}
 	return vs.poller
 }
 
 // Injector returns the input injector for this virtual session.
 func (vs *VirtualSession) Injector() InputInjector {
+	vs.mu.Lock()
+	defer vs.mu.Unlock()
+
 	return vs.injector
 }
 

client/vnc/server/xauth_x11.go

@@ -81,14 +81,17 @@ func writeXAuthFile(path, hostname, display string, cookie []byte, uid, gid uint
 }
 
 // ensureTraversable walks up from dir to configs.RuntimeDir (inclusive) and
-// sets mode 0711 on each component. Stops once it leaves the runtime dir so
+// sets mode 0711 on each component. A dir outside the runtime dir is refused
-// it never touches /var/run or /run.
+// before anything is chmodded, so it never touches /var/run or /run.
 func ensureTraversable(dir string) error {
 	root := filepath.Clean(configs.RuntimeDir)
 	if root == "" {
 		return nil
 	}
 	cur := filepath.Clean(dir)
+	if cur != root && !strings.HasPrefix(cur, root+string(os.PathSeparator)) {
+		return fmt.Errorf("xauth dir %s is outside the runtime dir %s", cur, root)
+	}
 	for {
 		if err := os.Chmod(cur, 0711); err != nil {
 			return fmt.Errorf("chmod %s: %w", cur, err)
@@ -97,7 +100,7 @@ func ensureTraversable(dir string) error {
 			return nil
 		}
 		parent := filepath.Dir(cur)
-		if parent == cur || !strings.HasPrefix(cur, root+string(os.PathSeparator)) {
+		if parent == cur {
 			return nil
 		}
 		cur = parent

management/server/http/handlers/peers/temporary_access_permission_test.go

@@ -8,8 +8,8 @@ import (
 	"net/http/httptest"
 	"testing"
 
-	"github.com/golang/mock/gomock"
 	"github.com/gorilla/mux"
+	"go.uber.org/mock/gomock"
 
 	nbcontext "github.com/netbirdio/netbird/management/server/context"
 	"github.com/netbirdio/netbird/management/server/permissions"

management/server/types/legacynmap/networkmap_components.go

@@ -10,10 +10,10 @@ import (
 	"sync"
 	"time"
 
-	auth "github.com/netbirdio/netbird/shared/sessionauth"
 	nbdns "github.com/netbirdio/netbird/dns"
 	"github.com/netbirdio/netbird/route"
 	"github.com/netbirdio/netbird/shared/management/domain"
+	auth "github.com/netbirdio/netbird/shared/sessionauth"
 )
 
 type NetworkMapComponents struct {

management/server/types/legacynmap/proto_legacy.go

@@ -7,7 +7,6 @@ import (
 	"net/url"
 	"strings"
 
-	auth "github.com/netbirdio/netbird/shared/sessionauth"
 	nbconfig "github.com/netbirdio/netbird/management/internals/server/config"
 	nbpeer "github.com/netbirdio/netbird/management/server/peer"
 	"github.com/netbirdio/netbird/management/server/types"
@@ -15,6 +14,7 @@ import (
 	"github.com/netbirdio/netbird/shared/management/networkmap"
 	"github.com/netbirdio/netbird/shared/management/proto"
 	"github.com/netbirdio/netbird/shared/netiputil"
+	auth "github.com/netbirdio/netbird/shared/sessionauth"
 )
 
 func ToProtocolRoutes(routes []*nbroute.Route) []*proto.Route {

shared/sessionauth/auth.go

@@ -104,7 +104,7 @@ func (a *Authorizer) Update(config *Config) {
 		a.machineUsers = make(map[string][]uint32)
 		a.sessionPubKeys = make(map[[sessionPubKeyLen]byte]sshuserhash.UserIDHash)
 		a.sessionDisplayNames = make(map[[sessionPubKeyLen]byte]string)
-		log.Info("SSH authorization cleared")
+		log.Info("session authorization cleared")
 		return
 	}
 
@@ -139,7 +139,7 @@ func (a *Authorizer) Update(config *Config) {
 			continue
 		}
 		if existing, ok := sessionPubKeys[key]; ok && existing != e.UserIDHash {
-			log.Warnf("SSH auth: session pubkey bound to conflicting user hashes; dropping binding")
+			log.Warn("session auth: session pubkey bound to conflicting user hashes; dropping binding")
 			delete(sessionPubKeys, key)
 			delete(sessionDisplayNames, key)
 			conflicted[key] = struct{}{}
@@ -153,7 +153,7 @@ func (a *Authorizer) Update(config *Config) {
 	a.sessionPubKeys = sessionPubKeys
 	a.sessionDisplayNames = sessionDisplayNames
 
-	log.Debugf("SSH auth: updated with %d authorized users, %d machine user mappings, %d session pubkeys",
+	log.Debugf("session auth: updated with %d authorized users, %d machine user mappings, %d session pubkeys",
 		len(config.AuthorizedUsers), len(machineUsers), len(sessionPubKeys))
 }
 

util/capture/text.go

@@ -538,11 +538,13 @@ func matchRFBSecurityFailure(p []byte) (string, bool) {
 	if len(p) < 5 || p[0] != 0 {
 		return "", false
 	}
-	reasonLen := int(p[1])<<24 | int(p[2])<<16 | int(p[3])<<8 | int(p[4])
+	// Kept as uint32: converting to int first would wrap to a negative value
-	if reasonLen <= 0 || reasonLen > 4096 || 5+reasonLen != len(p) {
+	// on a 32-bit build and lose the annotation to the length check below.
+	reasonLen := binary.BigEndian.Uint32(p[1:5])
+	if reasonLen == 0 || reasonLen > 4096 || 5+int(reasonLen) != len(p) {
 		return "", false
 	}
-	return string(p[5 : 5+reasonLen]), true
+	return string(p[5 : 5+int(reasonLen)]), true
 }
 
 // vncRejectCodes mirrors the RejectCode* constants in