diff --git a/client/cmd/vnc_agent.go b/client/cmd/vnc_agent.go index e7dcc3b4b..109e516e1 100644 --- a/client/cmd/vnc_agent.go +++ b/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//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 +} diff --git a/client/vnc/server/agent_darwin.go b/client/vnc/server/agent_darwin.go index b61eb63f8..ebb87893b 100644 --- a/client/vnc/server/agent_darwin.go +++ b/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) diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index 4d3566958..2c6c18aeb 100644 --- a/client/vnc/server/agent_ipc.go +++ b/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. diff --git a/client/vnc/server/capture_darwin.go b/client/vnc/server/capture_darwin.go index 8032b2b33..9a5c931ab 100644 --- a/client/vnc/server/capture_darwin.go +++ b/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. diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 59868869f..c5fcae8ea 100644 --- a/client/vnc/server/server.go +++ b/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 diff --git a/client/vnc/server/server_darwin.go b/client/vnc/server/server_darwin.go index 2bb85bf23..b8184e15b 100644 --- a/client/vnc/server/server_darwin.go +++ b/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 } diff --git a/client/vnc/server/server_windows.go b/client/vnc/server/server_windows.go index 2c26a3ee4..89ba26925 100644 --- a/client/vnc/server/server_windows.go +++ b/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 } diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index 262d3f79b..c3bd8dc33 100644 --- a/client/vnc/server/session.go +++ b/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 diff --git a/client/vnc/server/session_clipboard.go b/client/vnc/server/session_clipboard.go index 0b80d1248..e07495870 100644 --- a/client/vnc/server/session_clipboard.go +++ b/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 } diff --git a/client/vnc/server/session_encode.go b/client/vnc/server/session_encode.go index fff86cc13..1ae5a32ab 100644 --- a/client/vnc/server/session_encode.go +++ b/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 {