Hand the macOS agent token over stdin, refuse a pre-existing SAS event, bound session writes, and stop the service agent after handlers drain

client/cmd/vnc_agent.go

@@ -3,10 +3,14 @@
 package cmd
 
 import (
+	"bufio"
+	"errors"
 	"fmt"
+	"io"
 	"net"
 	"net/netip"
 	"os"
+	"strings"
 
 	log "github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -15,13 +19,20 @@ import (
 )
 
 var (
-	vncAgentSocket    string
-	vncAgentTargetUID uint32
+	vncAgentSocket     string
+	vncAgentTargetUID  uint32
+	vncAgentTokenStdin bool
 )
 
+// maxAgentTokenLine bounds the token read from stdin. The token is a short hex
+// string; anything longer is not one.
+const maxAgentTokenLine = 1024
+
 func init() {
 	vncAgentCmd.Flags().StringVar(&vncAgentSocket, "socket", "", "Unix-domain socket path the agent listens on (required)")
 	vncAgentCmd.Flags().Uint32Var(&vncAgentTargetUID, "target-uid", 0, "uid the agent drops privileges to before listening (darwin only; required there, and must not be 0)")
+	// Must match agentTokenStdinFlag in client/vnc/server/agent_ipc.go.
+	vncAgentCmd.Flags().BoolVar(&vncAgentTokenStdin, "token-stdin", false, "read the per-spawn token from stdin instead of the environment")
 	rootCmd.AddCommand(vncAgentCmd)
 }
 
@@ -42,13 +53,9 @@ var vncAgentCmd = &cobra.Command{
 			return fmt.Errorf("--socket is required")
 		}
 
-		token := os.Getenv("NB_VNC_AGENT_TOKEN")
-		if token == "" {
-			return fmt.Errorf("NB_VNC_AGENT_TOKEN not set; agent requires a token from the service")
-		}
-		// Purge the token from env so it doesn't leak via /proc/<pid>/environ.
-		if err := os.Unsetenv("NB_VNC_AGENT_TOKEN"); err != nil {
-			log.Debugf("unset NB_VNC_AGENT_TOKEN: %v", err)
+		token, err := readAgentToken()
+		if err != nil {
+			return err
 		}
 
 		// Drop root privileges to the target console user BEFORE creating
@@ -108,3 +115,31 @@ var vncAgentCmd = &cobra.Command{
 	},
 	SilenceUsage: true,
 }
+
+// readAgentToken returns the per-spawn token the service handed over, from
+// stdin when --token-stdin is set and from the environment otherwise. Missing
+// or empty is an error: the agent must never serve without one.
+func readAgentToken() (string, error) {
+	if vncAgentTokenStdin {
+		line, err := bufio.NewReader(io.LimitReader(os.Stdin, maxAgentTokenLine)).ReadString('\n')
+		if err != nil && !errors.Is(err, io.EOF) {
+			return "", fmt.Errorf("read agent token from stdin: %w", err)
+		}
+		_ = os.Stdin.Close()
+		token := strings.TrimSpace(line)
+		if token == "" {
+			return "", fmt.Errorf("no agent token on stdin; agent requires a token from the service")
+		}
+		return token, nil
+	}
+
+	token := os.Getenv("NB_VNC_AGENT_TOKEN")
+	if token == "" {
+		return "", fmt.Errorf("NB_VNC_AGENT_TOKEN not set; agent requires a token from the service")
+	}
+	// Purge the token from env so later reads in this process do not see it.
+	if err := os.Unsetenv("NB_VNC_AGENT_TOKEN"); err != nil {
+		log.Debugf("unset NB_VNC_AGENT_TOKEN: %v", err)
+	}
+	return token, nil
+}

client/vnc/server/agent_darwin.go

@@ -11,6 +11,7 @@ import (
 	"os"
 	"os/exec"
 	"strconv"
+	"strings"
 	"sync"
 	"syscall"
 	"time"
@@ -106,6 +107,11 @@ func (m *darwinAgentManager) Resolve(ctx context.Context) (string, string, uint3
 	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
+	// A handler that outlived the server's shutdown must not start an agent:
+	// the manager it holds has been stopped and nothing would reap the child.
+	if err := ctx.Err(); err != nil {
+		return "", "", 0, fmt.Errorf("resolve agent: %w", err)
+	}
 	if m.running && m.uid == consoleUID && vncAgentRunning() {
 		m.users++
 		return m.socketPath, m.authToken, m.uid, nil
@@ -335,8 +341,13 @@ func spawnAgentForUser(uid uint32, socketPath, token string) error {
 		// session. validateAgentPeer on the daemon side also relies on
 		// the agent's effective uid matching consoleUID.
 		"--target-uid", strconv.FormatUint(uint64(uid), 10),
+		agentTokenStdinFlag,
 	)
-	cmd.Env = append(os.Environ(), agentTokenEnvVar+"="+token)
+	// The token goes over stdin, not the environment: the agent drops to the
+	// console user, and a user-owned process's startup environment is
+	// readable by every other process of that user. launchctl asuser execs the
+	// command, so the pipe is inherited as the agent's fd 0.
+	cmd.Stdin = strings.NewReader(token + "\n")
 	stderr, err := cmd.StderrPipe()
 	if err != nil {
 		return fmt.Errorf("agent stderr pipe: %w", err)

client/vnc/server/agent_ipc.go

@@ -135,6 +135,16 @@ const (
 	// such as `ps` or Windows tasklist would expose it.
 	agentTokenEnvVar = "NB_VNC_AGENT_TOKEN" // #nosec G101 -- env var name, not a credential
 
+	// agentTokenStdinFlag tells the agent to read its token from stdin
+	// instead of agentTokenEnvVar. Must match the flag cmd.vncAgentCmd
+	// registers. Used where the agent runs as the console user: there the
+	// environment it was started with stays readable by that user's other
+	// processes for the agent's whole life (macOS KERN_PROCARGS2 keeps the
+	// original strings even after unsetenv), and holding the token lets a
+	// process drive the agent directly, past the daemon's gates and with the
+	// agent's Screen Recording grant.
+	agentTokenStdinFlag = "--token-stdin" // #nosec G101 -- flag name, not a credential
+
 	// vncAgentSubcommand is the CLI subcommand the daemon invokes to start
 	// the per-session agent process. Must match cmd.vncAgentCmd.Use in
 	// client/cmd/vnc_agent.go.

client/vnc/server/capture_darwin.go

@@ -116,9 +116,9 @@ type CGCapturer struct {
 	// into the framebuffer's own pixel grid, which differs from it whenever
 	// the display is Retina.
 	logicalW, logicalH int
-	hashSeed  maphash.Seed
-	lastHash  uint64
-	hasHash   bool
+	hashSeed           maphash.Seed
+	lastHash           uint64
+	hasHash            bool
 	// cursor lazily binds the private CGSCreateCurrentCursorImage symbol
 	// so we can emit the Cursor pseudo-encoding without a per-frame cost
 	// on builds that never query it.

client/vnc/server/server.go

@@ -911,12 +911,6 @@ func (s *Server) Stop() error {
 		s.vmgr.StopAll()
 	}
 
-	s.stopServiceAgent()
-
-	if s.serviceMode {
-		s.platformShutdown()
-	}
-
 	// Let the handlers finish before the capturer and injector go away. Their
 	// sockets are closed above, so each is on its way out, and the last thing a
 	// session does is release the modifiers and buttons the client left held:
@@ -924,6 +918,15 @@ func (s *Server) Stop() error {
 	// stuck Shift or mouse button.
 	s.awaitHandlers(drained)
 
+	// After the drain, not before: a service-mode handler holds the agent
+	// manager for its whole life, and one that reached Resolve after the manager
+	// was stopped would spawn an agent nothing is left to tear down.
+	s.stopServiceAgent()
+
+	if s.serviceMode {
+		s.platformShutdown()
+	}
+
 	if c, ok := s.capturer.(interface{ Close() }); ok {
 		c.Close()
 	}
@@ -943,6 +946,26 @@ func (s *Server) Stop() error {
 	return nil
 }
 
+// retryAccept decides what an accept loop does after ln.Accept fails: false
+// when the loop should exit (server stopping, listener closed, or an error that
+// will not clear), true after pausing when the error is worth another try. The
+// pause is what keeps a persistent error from spinning the loop at full CPU.
+func (s *Server) retryAccept(ln net.Listener, err error) bool {
+	if s.ctx.Err() != nil {
+		return false
+	}
+	if errors.Is(err, net.ErrClosed) {
+		s.log.Debugf("VNC listener closed: %v", err)
+		return false
+	}
+	if !acceptRetryable(err) {
+		s.log.Errorf("VNC listener %s gave up: %v", ln.Addr(), err)
+		return false
+	}
+	s.log.Debugf("accept VNC connection: %v", err)
+	return s.sleepOrDone(acceptRetryPause)
+}
+
 // acceptLoop handles VNC connections directly (user session mode).
 func (s *Server) acceptLoop(ln net.Listener) {
 	if ln == nil {
@@ -951,19 +974,7 @@ func (s *Server) acceptLoop(ln net.Listener) {
 	for {
 		conn, err := ln.Accept()
 		if err != nil {
-			if s.ctx.Err() != nil {
-				return
-			}
-			if errors.Is(err, net.ErrClosed) {
-				s.log.Debugf("VNC listener closed: %v", err)
-				return
-			}
-			if !acceptRetryable(err) {
-				s.log.Errorf("VNC listener %s gave up: %v", ln.Addr(), err)
-				return
-			}
-			s.log.Debugf("accept VNC connection: %v", err)
-			if !s.sleepOrDone(acceptRetryPause) {
+			if !s.retryAccept(ln, err) {
 				return
 			}
 			continue

client/vnc/server/server_darwin.go

@@ -40,12 +40,9 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 	for {
 		conn, err := ln.Accept()
 		if err != nil {
-			select {
-			case <-s.ctx.Done():
+			if !s.retryAccept(ln, err) {
 				return
-			default:
 			}
-			s.log.Debugf("accept VNC connection: %v", err)
 			continue
 		}
 

client/vnc/server/server_windows.go

@@ -230,6 +230,17 @@ func createSASEvent() (windows.Handle, bool) {
 	defer freeSecurityDescriptor(sa)
 
 	ev, err := windows.CreateEvent(sa, 0, 0, namePtr)
+	if errors.Is(err, windows.ERROR_ALREADY_EXISTS) {
+		// CreateEvent opens an existing object instead of creating one, and
+		// then ignores sa: the handle carries whatever DACL the object already
+		// has. A named object only survives while someone holds a handle, so an
+		// existing one belongs to another process, possibly an unprivileged one
+		// that created it permissive so it could signal SendSAS itself. Refuse
+		// it rather than wait on an event this process does not control.
+		_ = windows.CloseHandle(ev)
+		log.Warnf("SAS event %s already exists and is not ours; Ctrl+Alt+Del forwarding disabled", sasEventName)
+		return 0, false
+	}
 	if err != nil {
 		log.Warnf("SAS CreateEvent: %v", err)
 		return 0, false
@@ -388,12 +399,9 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) {
 	for {
 		conn, err := ln.Accept()
 		if err != nil {
-			select {
-			case <-s.ctx.Done():
+			if !s.retryAccept(ln, err) {
 				return
-			default:
 			}
-			s.log.Debugf("accept VNC connection: %v", err)
 			continue
 		}
 

client/vnc/server/session.go

@@ -27,6 +27,11 @@ const (
 // pinning a connSem slot.
 const handshakeDeadline = 10 * time.Second
 
+// writeDeadline bounds every server-to-client write. A peer that stops reading
+// otherwise blocks the writer for as long as it likes, holding its connection
+// slot and the encoder goroutine with it.
+const writeDeadline = 30 * time.Second
+
 const tileSize = 64 // pixels per tile for dirty-rect detection
 
 // fullFramePromoteNum/Den trigger full-frame encoding when the dirty area
@@ -89,8 +94,6 @@ type session struct {
 	// encMu by handleSetEncodings and read by the encoder goroutine.
 	clientSupportsDesktopSize         bool
 	clientSupportsExtendedDesktopSize bool
-	clientSupportsDesktopName         bool
-	clientSupportsLastRect            bool
 	clientSupportsQEMUKey             bool
 	clientSupportsExtClipboard        bool
 	clientSupportsCursor              bool
@@ -182,6 +185,17 @@ type fbRequest struct {
 
 func (s *session) addr() string { return s.conn.RemoteAddr().String() }
 
+// lockWrite takes writeMu and arms the write deadline for the writes that
+// follow, returning the unlock. Every server-to-client write goes through it,
+// so no write can outlive writeDeadline.
+func (s *session) lockWrite() func() {
+	s.writeMu.Lock()
+	if err := s.conn.SetWriteDeadline(time.Now().Add(writeDeadline)); err != nil {
+		s.log.Debugf("set write deadline: %v", err)
+	}
+	return s.writeMu.Unlock
+}
+
 // serve runs the full RFB session lifecycle.
 func (s *session) serve() {
 	defer s.conn.Close()
@@ -337,8 +351,11 @@ func (s *session) sendServerInit() error {
 func (s *session) messageLoop() error {
 	for {
 		var msgType [1]byte
-		if err := s.conn.SetDeadline(time.Now().Add(readDeadline)); err != nil {
-			return fmt.Errorf("set deadline: %w", err)
+		// Read side only. The encoder writes on this connection concurrently,
+		// and a shared deadline would both time its writes out on the read
+		// loop's schedule and, once cleared below, leave them unbounded.
+		if err := s.conn.SetReadDeadline(time.Now().Add(readDeadline)); err != nil {
+			return fmt.Errorf("set read deadline: %w", err)
 		}
 		if _, err := io.ReadFull(s.conn, msgType[:]); err != nil {
 			return err
@@ -369,7 +386,7 @@ func (s *session) messageLoop() error {
 		}
 		// Clear the deadline only after the full message has been read and
 		// processed so payload reads in the handlers stay bounded.
-		_ = s.conn.SetDeadline(time.Time{})
+		_ = s.conn.SetReadDeadline(time.Time{})
 		if err != nil {
 			return err
 		}
@@ -488,11 +505,13 @@ func (s *session) resetEncodingCaps() {
 	s.useHextile = false
 	s.clientSupportsDesktopSize = false
 	s.clientSupportsExtendedDesktopSize = false
-	s.clientSupportsDesktopName = false
-	s.clientSupportsLastRect = false
 	s.clientSupportsQEMUKey = false
 	s.clientSupportsExtClipboard = false
 	s.clientSupportsCursor = false
+	// The client may drop Cursor and ask for it again later, and must then get
+	// the current sprite: left as it is, the marker says the sprite was already
+	// delivered and it is never sent.
+	s.lastCursorSerial = 0
 	s.clientSupportsExtMouseButtons = false
 	s.cursorSourceFailed = false
 	s.cursorSourceFailures = 0
@@ -517,11 +536,13 @@ func (s *session) applyEncoding(enc int32) string {
 	case pseudoEncExtendedDesktopSize:
 		s.clientSupportsExtendedDesktopSize = true
 		return "ext-desktop-size"
-	case pseudoEncDesktopName:
-		s.clientSupportsDesktopName = true
-		return "desktop-name"
-	case pseudoEncLastRect:
-		s.clientSupportsLastRect = true
+	case pseudoEncDesktopName, pseudoEncLastRect:
+		// Recognised for the debug log only. The server never sends either,
+		// so there is no capability to record: a flag here would suggest a
+		// DesktopName or LastRect path that does not exist.
+		if enc == pseudoEncDesktopName {
+			return "desktop-name"
+		}
 		return "last-rect"
 	case pseudoEncQEMUExtendedKeyEvent:
 		s.clientSupportsQEMUKey = true

client/vnc/server/session_clipboard.go

@@ -237,9 +237,9 @@ func (s *session) writeExtClipMessage(payload []byte) error {
 	binary.BigEndian.PutUint32(buf[4:8], uint32(-int32(len(payload))))
 	copy(buf[8:], payload)
 
-	s.writeMu.Lock()
+	unlock := s.lockWrite()
 	_, err := s.conn.Write(buf)
-	s.writeMu.Unlock()
+	unlock()
 	return err
 }
 
@@ -282,8 +282,8 @@ func (s *session) sendServerCutText(text string) error {
 	binary.BigEndian.PutUint32(buf[4:8], uint32(len(data)))
 	copy(buf[8:], data)
 
-	s.writeMu.Lock()
+	unlock := s.lockWrite()
 	_, err := s.conn.Write(buf)
-	s.writeMu.Unlock()
+	unlock()
 	return err
 }

client/vnc/server/session_encode.go

@@ -133,8 +133,8 @@ func (s *session) processIncremental(img *image.RGBA) error {
 	copy(dirty, tiles)
 
 	var moves []copyRectMove
-	if s.useCopyRect && s.copyRectDet != nil {
-		moves, tiles = s.copyRectDet.extractCopyRectTiles(img, tiles)
+	if useCopyRect, det := s.copyRectState(); useCopyRect && det != nil {
+		moves, tiles = det.extractCopyRectTiles(img, tiles)
 	}
 
 	rects := coalesceRects(tiles)
@@ -265,12 +265,12 @@ func (s *session) handleResize() error {
 	// the new dimensions rather than diffing against a stale-sized buffer.
 	s.prevFrame = nil
 	s.curFrame = nil
-	if s.copyRectDet != nil {
+	if _, det := s.copyRectState(); det != nil {
 		// Tile geometry changed; let updateDirty rebuild from scratch on
 		// the next pass instead of reusing stale hashes keyed on old
 		// (cols, rows).
-		s.copyRectDet.prevTiles = nil
-		s.copyRectDet.tileHash = nil
+		det.prevTiles = nil
+		det.tileHash = nil
 	}
 	if err := s.sendDesktopSize(w, h); err != nil {
 		return fmt.Errorf("send desktop size: %w", err)
@@ -294,8 +294,7 @@ func (s *session) sendDesktopSize(w, h int) error {
 	binary.BigEndian.PutUint16(header[2:4], 1)
 
 	body := encodeDesktopSizeBody(w, h)
-	s.writeMu.Lock()
-	defer s.writeMu.Unlock()
+	defer s.lockWrite()()
 	defer s.markFBU(1)()
 	if _, err := s.conn.Write(header); err != nil {
 		return err
@@ -317,8 +316,7 @@ func (s *session) sendExtMouseAck() error {
 	enc := int32(pseudoEncExtendedMouseButtons)
 	binary.BigEndian.PutUint32(rect[8:12], uint32(enc))
 
-	s.writeMu.Lock()
-	defer s.writeMu.Unlock()
+	defer s.lockWrite()()
 	defer s.markFBU(1)()
 	if _, err := s.conn.Write(header); err != nil {
 		return err
@@ -331,20 +329,33 @@ func (s *session) sendExtMouseAck() error {
 // Used after full-frame sends, where we don't have a per-tile dirty list to
 // drive an incremental update.
 func (s *session) refreshCopyRectIndex() {
-	if s.copyRectDet == nil || s.prevFrame == nil {
+	_, det := s.copyRectState()
+	if det == nil || s.prevFrame == nil {
 		return
 	}
-	s.copyRectDet.rebuild(s.prevFrame, s.serverW, s.serverH)
+	det.rebuild(s.prevFrame, s.serverW, s.serverH)
 }
 
 // updateCopyRectIndex incrementally updates the CopyRect detector's hash
 // tables for the tiles that just changed. On first use (or after resize)
 // updateDirty internally falls back to a full rebuild.
 func (s *session) updateCopyRectIndex(dirty [][4]int) {
-	if s.copyRectDet == nil || s.prevFrame == nil {
+	_, det := s.copyRectState()
+	if det == nil || s.prevFrame == nil {
 		return
 	}
-	s.copyRectDet.updateDirty(s.prevFrame, s.serverW, s.serverH, dirty)
+	det.updateDirty(s.prevFrame, s.serverW, s.serverH, dirty)
+}
+
+// copyRectState snapshots whether the client negotiated CopyRect and the
+// detector that serves it. Both are published by SetEncodings on the message
+// loop under encMu, so the encoder reads them under the same lock. The
+// detector's own tables are touched only by the encoder goroutine, so holding
+// the pointer past the unlock is safe.
+func (s *session) copyRectState() (bool, *copyRectDetector) {
+	s.encMu.RLock()
+	defer s.encMu.RUnlock()
+	return s.useCopyRect, s.copyRectDet
 }
 
 // captureFrame returns a session-owned frame for this encode cycle.
@@ -530,8 +541,7 @@ func (s *session) encodeZlibSingle(img *image.RGBA, pf clientPixelFormat, w, h i
 // rects is the rectangle count in that header, reported to the metrics wrapper
 // so it knows where this update begins.
 func (s *session) writeFramed(buf []byte, rects int) error {
-	s.writeMu.Lock()
-	defer s.writeMu.Unlock()
+	defer s.lockWrite()()
 	defer s.markFBU(rects)()
 	if _, err := s.conn.Write(buf); err != nil {
 		return err
@@ -583,8 +593,7 @@ func (s *session) sendDirtyAndMoves(img *image.RGBA, moves []copyRectMove, rects
 	header[0] = serverFramebufferUpdate
 	binary.BigEndian.PutUint16(header[2:4], uint16(total))
 
-	s.writeMu.Lock()
-	defer s.writeMu.Unlock()
+	defer s.lockWrite()()
 
 	defer s.markFBU(total)()
 	if _, err := s.conn.Write(header); err != nil {