diff --git a/client/cmd/vnc_agent.go b/client/cmd/vnc_agent.go index 109e516e1..2506fc38a 100644 --- a/client/cmd/vnc_agent.go +++ b/client/cmd/vnc_agent.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "net" "net/netip" "os" "strings" @@ -29,7 +28,7 @@ var ( const maxAgentTokenLine = 1024 func init() { - vncAgentCmd.Flags().StringVar(&vncAgentSocket, "socket", "", "Unix-domain socket path the agent listens on (required)") + vncAgentCmd.Flags().StringVar(&vncAgentSocket, "socket", "", "socket the agent listens on: a Unix-domain socket path on darwin, a named pipe path on Windows (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") @@ -37,9 +36,9 @@ func init() { } // vncAgentCmd runs a VNC server inside the user's interactive session, -// listening on a Unix-domain socket. The NetBird service spawns it: on -// Windows via CreateProcessAsUser into the console session, on macOS via -// launchctl asuser into the Aqua session. +// listening on a Unix-domain socket (a named pipe on Windows). The NetBird +// service spawns it: on Windows via CreateProcessAsUser into the console +// session, on macOS via launchctl asuser into the Aqua session. var vncAgentCmd = &cobra.Command{ Use: "vnc-agent", Short: "Run VNC capture agent (internal, spawned by service)", @@ -83,16 +82,10 @@ var vncAgentCmd = &cobra.Command{ return err } - if err := os.Remove(vncAgentSocket); err != nil && !os.IsNotExist(err) { - log.Debugf("remove stale socket %s: %v", vncAgentSocket, err) - } - ln, err := net.Listen("unix", vncAgentSocket) + ln, err := vncserver.ListenAgentSocket(vncAgentSocket) if err != nil { return fmt.Errorf("listen on %s: %w", vncAgentSocket, err) } - if err := os.Chmod(vncAgentSocket, 0o600); err != nil { - log.Debugf("chmod %s: %v", vncAgentSocket, err) - } ctx := cmd.Context() diff --git a/client/vnc/server/agent_darwin.go b/client/vnc/server/agent_darwin.go index e7c05c347..58c1d0c92 100644 --- a/client/vnc/server/agent_darwin.go +++ b/client/vnc/server/agent_darwin.go @@ -7,7 +7,6 @@ import ( "context" "errors" "fmt" - "net" "os" "os/exec" "strconv" @@ -375,14 +374,13 @@ func spawnAgentForUser(uid uint32, socketPath, token string) error { // waitForAgent dials the agent's Unix socket until it answers. Used to // gate proxy attempts until the spawned process has finished its Start. func waitForAgent(ctx context.Context, socketPath string, wait time.Duration) error { - var d net.Dialer deadline := time.Now().Add(wait) for time.Now().Before(deadline) { if ctx.Err() != nil { return ctx.Err() } dialCtx, cancel := context.WithTimeout(ctx, 200*time.Millisecond) - c, err := d.DialContext(dialCtx, "unix", socketPath) + c, err := dialAgent(dialCtx, socketPath) cancel() if err == nil { _ = c.Close() diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index f642310b3..25d887f17 100644 --- a/client/vnc/server/agent_ipc.go +++ b/client/vnc/server/agent_ipc.go @@ -91,7 +91,7 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) { authedLog.Info("VNC connection approved by user") } - socketPath, token, peerUID, err := sa.Resolve(s.ctx) + socketPath, token, peerID, err := sa.Resolve(s.ctx) if err != nil { code := RejectCodeCapturerError if errors.Is(err, errNoConsoleUser) { @@ -120,7 +120,7 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) { Reader: io.MultiReader(&headerBuf, conn), Conn: conn, } - if err := proxyToAgent(s.ctx, replayConn, socketPath, token, peerUID, decision.ViewOnly, authedLog); err != nil { + if err := proxyToAgent(s.ctx, replayConn, socketPath, token, peerID, decision.ViewOnly, authedLog); err != nil { rejectConnection(conn, codeMessage(RejectCodeCapturerError, err.Error())) authedLog.Warnf("VNC connection rejected: agent unreachable: %v", err) return @@ -145,8 +145,8 @@ func generateAuthToken() (string, error) { return hex.EncodeToString(b), nil } -// proxyToAgent dials the per-session agent's Unix socket, checks the peer's -// kernel-asserted uid, runs the mutual challenge-response that proves both ends +// proxyToAgent dials the per-session agent's socket (a named pipe on Windows), +// checks the peer's kernel-asserted identity, runs the mutual challenge-response that proves both ends // hold the per-spawn token, then copies bytes both ways until either side // closes. // @@ -160,7 +160,7 @@ func generateAuthToken() (string, error) { // of a bare timeout. authedLog receives one audit line per established session // so an operator can correlate daemon→agent traffic with the remote session // that triggered it. -func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken string, peerUID uint32, viewOnly bool, authedLog *log.Entry) error { +func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken string, peerID uint32, viewOnly bool, authedLog *log.Entry) error { tokenBytes, err := hex.DecodeString(authToken) if err != nil || len(tokenBytes) != agentTokenLen { return fmt.Errorf("invalid auth token (len=%d): %w", len(tokenBytes), err) @@ -171,7 +171,7 @@ func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken st return fmt.Errorf("dial agent at %s: %w", socketPath, err) } - if err := validateAgentPeer(agentConn, peerUID); err != nil { + if err := validateAgentPeer(agentConn, peerID); err != nil { _ = agentConn.Close() return fmt.Errorf("agent peer validation failed: %w", err) } @@ -189,7 +189,7 @@ func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken st tokenFp = tokenFp[:8] } if authedLog != nil { - authedLog.Infof("VNC IPC: agent authenticated socket=%s peer_uid=%d view_only=%v token_fp=%s", socketPath, peerUID, viewOnly, tokenFp) + authedLog.Infof("VNC IPC: agent authenticated socket=%s peer_id=%d view_only=%v token_fp=%s", socketPath, peerID, viewOnly, tokenFp) } defer client.Close() @@ -266,7 +266,6 @@ func relogAgentStream(r io.Reader) { // the final error. Aborts early when ctx is cancelled so a Stop() during // service-mode startup doesn't leave a goroutine sleeping for 10 s. func dialAgentWithRetry(ctx context.Context, addr string) (net.Conn, error) { - var d net.Dialer var lastErr error for range 50 { if err := ctx.Err(); err != nil { @@ -276,7 +275,7 @@ func dialAgentWithRetry(ctx context.Context, addr string) (net.Conn, error) { return nil, lastErr } dialCtx, cancel := context.WithTimeout(ctx, time.Second) - c, err := d.DialContext(dialCtx, "unix", addr) + c, err := dialAgent(dialCtx, addr) cancel() if err == nil { return c, nil diff --git a/client/vnc/server/agent_peercred_windows.go b/client/vnc/server/agent_peercred_windows.go index 07b583704..a903b859e 100644 --- a/client/vnc/server/agent_peercred_windows.go +++ b/client/vnc/server/agent_peercred_windows.go @@ -3,30 +3,34 @@ package server import ( + "errors" + "fmt" "net" + + "golang.org/x/sys/windows" ) -// validateAgentPeer is a documented no-op on Windows. AF_UNIX on Windows -// exposes no SO_PEERCRED equivalent and no supported API to recover the -// peer process from an accepted AF_UNIX connection, so the daemon cannot -// match the connected peer against the agent PID it spawned the way the -// darwin path does via LOCAL_PEERCRED. The Windows trust model therefore -// rests on three other measures, none of which assume the socket path is -// secret: -// -// - the socket lives in a dedicated directory (agentSocketDirPath) under -// %SystemRoot%\SystemTemp, created with a DACL granting only SYSTEM and -// Administrators, so an unprivileged local user cannot create or squat a -// socket there; -// - each spawn uses a cryptographically random socket name, so the path -// is unguessable before the agent binds it; -// - the daemon publishes the path only after confirming the spawned -// agent is listening (see waitForAgentListening), and gates every -// connection on the per-spawn auth-token preamble that follows this -// call. -// -// If a future Windows release exposes peer-PID retrieval for AF_UNIX, -// this function should verify the peer against the spawned agent PID. -func validateAgentPeer(_ net.Conn, _ uint32) error { +// validateAgentPeer checks that the pipe the daemon connected to is served by +// the agent process it spawned. The pipe name sits in the protected namespace, +// so only SYSTEM or an administrator could have created it; the PID check pins +// it to the spawned agent. The daemon holds the agent's process handle for the +// agent's lifetime, so the PID cannot be recycled underneath the check. Fails +// closed when no PID is known or the query fails. +func validateAgentPeer(conn net.Conn, expectedPID uint32) error { + if expectedPID == 0 { + return errors.New("no agent PID to verify the pipe server against") + } + // go-winio's pipe connection embeds *win32File, which exposes Fd(). + fdConn, ok := conn.(interface{ Fd() uintptr }) + if !ok { + return fmt.Errorf("agent connection %T exposes no pipe handle", conn) + } + var pid uint32 + if err := windows.GetNamedPipeServerProcessId(windows.Handle(fdConn.Fd()), &pid); err != nil { + return fmt.Errorf("query pipe server PID: %w", err) + } + if pid != expectedPID { + return fmt.Errorf("pipe served by PID %d, expected agent PID %d", pid, expectedPID) + } return nil } diff --git a/client/vnc/server/agent_transport_darwin.go b/client/vnc/server/agent_transport_darwin.go new file mode 100644 index 000000000..21994202e --- /dev/null +++ b/client/vnc/server/agent_transport_darwin.go @@ -0,0 +1,33 @@ +//go:build darwin && !ios + +package server + +import ( + "context" + "net" + "os" + + log "github.com/sirupsen/logrus" +) + +// ListenAgentSocket binds the Unix-domain socket the vnc-agent serves on, +// replacing a stale socket file left at the path, and restricts it to the +// owning user. +func ListenAgentSocket(path string) (net.Listener, error) { + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Debugf("remove stale socket %s: %v", path, err) + } + ln, err := net.Listen("unix", path) + if err != nil { + return nil, err + } + if err := os.Chmod(path, 0o600); err != nil { + log.Debugf("chmod %s: %v", path, err) + } + return ln, nil +} + +func dialAgent(ctx context.Context, addr string) (net.Conn, error) { + var d net.Dialer + return d.DialContext(ctx, "unix", addr) +} diff --git a/client/vnc/server/agent_transport_windows.go b/client/vnc/server/agent_transport_windows.go new file mode 100644 index 000000000..4e54201e4 --- /dev/null +++ b/client/vnc/server/agent_transport_windows.go @@ -0,0 +1,36 @@ +//go:build windows + +package server + +import ( + "context" + "net" + + "github.com/Microsoft/go-winio" +) + +const ( + // agentPipePrefix places agent pipes in the NPFS namespace where only + // LocalSystem and members of BUILTIN\Administrators may create a pipe, so an + // unprivileged process cannot pre-create the name the daemon hands the agent. + agentPipePrefix = `\\.\pipe\ProtectedPrefix\Administrators\netbird-vnc-` + + // agentPipeSDDL lets only LocalSystem open the pipe. The daemon and the + // agent both run as SYSTEM, and nothing else has a reason to connect. + agentPipeSDDL = "D:P(A;;GA;;;SY)" +) + +// ListenAgentSocket creates the named pipe the vnc-agent serves on. Creation +// fails if a pipe with that name already exists, so the agent never shares a +// name with a process that got there first. +func ListenAgentSocket(path string) (net.Listener, error) { + return listenAgentPipe(path, agentPipeSDDL) +} + +func listenAgentPipe(path, sddl string) (net.Listener, error) { + return winio.ListenPipe(path, &winio.PipeConfig{SecurityDescriptor: sddl}) +} + +func dialAgent(ctx context.Context, addr string) (net.Conn, error) { + return winio.DialPipeContext(ctx, addr) +} diff --git a/client/vnc/server/agent_transport_windows_test.go b/client/vnc/server/agent_transport_windows_test.go new file mode 100644 index 000000000..b44c299dd --- /dev/null +++ b/client/vnc/server/agent_transport_windows_test.go @@ -0,0 +1,104 @@ +//go:build windows + +package server + +import ( + "context" + "errors" + "net" + "os" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/sys/windows" +) + +// testPipeSDDL widens the agent DACL to administrators so a test that does not +// run as SYSTEM can still connect. Creating a pipe in the protected namespace +// needs administrator rights either way. +const testPipeSDDL = "D:P(A;;GA;;;SY)(A;;GA;;;BA)" + +func listenTestPipe(t *testing.T, sddl string) (string, net.Listener) { + t.Helper() + path, err := newAgentPipePath(0) + require.NoError(t, err) + ln, err := listenAgentPipe(path, sddl) + if errors.Is(err, windows.ERROR_ACCESS_DENIED) { + t.Skip("creating a pipe under ProtectedPrefix needs administrator rights") + } + require.NoError(t, err, "create pipe in the protected namespace") + t.Cleanup(func() { _ = ln.Close() }) + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + _ = c.Close() + } + }() + return path, ln +} + +// The test process serves the pipe itself, so the pipe server PID the daemon +// side reads back is our own PID. +func TestAgentPipePeerPID(t *testing.T) { + path, _ := listenTestPipe(t, testPipeSDDL) + self := uint32(os.Getpid()) + + require.NoError(t, waitForAgentListening(path, self, 2*time.Second), + "readiness gate must accept a pipe served by the expected PID") + assert.Error(t, waitForAgentListening(path, self+1, 2*time.Second), + "readiness gate must reject a pipe served by another PID") + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + conn, err := dialAgent(ctx, path) + require.NoError(t, err) + defer conn.Close() + + assert.NoError(t, validateAgentPeer(conn, self)) + assert.Error(t, validateAgentPeer(conn, self+1), "mismatched PID must fail") + assert.Error(t, validateAgentPeer(conn, 0), "unknown PID must fail closed") +} + +// A second listener on the same name must fail, so an agent never serves a +// pipe name some other process created first. +func TestAgentPipeRefusesExistingName(t *testing.T) { + path, _ := listenTestPipe(t, testPipeSDDL) + + second, err := listenAgentPipe(path, testPipeSDDL) + if second != nil { + _ = second.Close() + } + assert.Error(t, err, "creating an existing pipe name must fail") +} + +// The production DACL admits only LocalSystem, so any other caller, an +// administrator included, is refused. +func TestAgentPipeDeniesNonSystem(t *testing.T) { + if isLocalSystem(t) { + t.Skip("running as LocalSystem, which the DACL admits") + } + path, _ := listenTestPipe(t, agentPipeSDDL) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + conn, err := dialAgent(ctx, path) + if conn != nil { + _ = conn.Close() + } + assert.ErrorIs(t, err, windows.ERROR_ACCESS_DENIED) +} + +func isLocalSystem(t *testing.T) bool { + t.Helper() + token := windows.GetCurrentProcessToken() + user, err := token.GetTokenUser() + require.NoError(t, err) + system, err := windows.CreateWellKnownSid(windows.WinLocalSystemSid) + require.NoError(t, err) + return user.User.Sid.Equals(system) +} diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index d402f0ec4..3cfee47f3 100644 --- a/client/vnc/server/agent_windows.go +++ b/client/vnc/server/agent_windows.go @@ -9,9 +9,7 @@ import ( "encoding/hex" "errors" "fmt" - "net" "os" - "path/filepath" "runtime" "sync" "time" @@ -244,10 +242,18 @@ func injectEnvVar(envBlock uintptr, key, value string) []uint16 { return newBlock } -func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHandle windows.Handle) (windows.Handle, error) { +// spawnedAgent is a running agent process: the handle the daemon holds for +// the agent's lifetime and the PID its pipe server must report. +type spawnedAgent struct { + process windows.Handle + pid uint32 +} + +func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHandle windows.Handle) (spawnedAgent, error) { + var none spawnedAgent token, err := getSystemTokenForSession(sessionID) if err != nil { - return 0, fmt.Errorf("get SYSTEM token for session %d: %w", sessionID, err) + return none, fmt.Errorf("get SYSTEM token for session %d: %w", sessionID, err) } defer token.Close() @@ -260,7 +266,7 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand if r == 0 { // Without an environment block we cannot inject NB_VNC_AGENT_TOKEN; // the agent would start unauthenticated. Abort instead of launching. - return 0, fmt.Errorf("CreateEnvironmentBlock: %w", e) + return none, fmt.Errorf("CreateEnvironmentBlock: %w", e) } defer func() { _, _, _ = procDestroyEnvironmentBlock.Call(envBlock) }() @@ -271,13 +277,13 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand exePath, err := os.Executable() if err != nil { - return 0, fmt.Errorf("get executable path: %w", err) + return none, fmt.Errorf("get executable path: %w", err) } - cmdLine := fmt.Sprintf(`"%s" %s --socket %q`, exePath, vncAgentSubcommand, socketPath) + cmdLine := fmt.Sprintf(`%s %s --socket %s`, windows.EscapeArg(exePath), vncAgentSubcommand, windows.EscapeArg(socketPath)) cmdLineW, err := windows.UTF16PtrFromString(cmdLine) if err != nil { - return 0, fmt.Errorf("UTF16 cmdline: %w", err) + return none, fmt.Errorf("UTF16 cmdline: %w", err) } // Create an inheritable pipe for the agent's stderr so we can relog @@ -288,7 +294,7 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand var stderrRead, stderrWrite windows.Handle if err := windows.CreatePipe(&stderrRead, &stderrWrite, &sa, 0); err != nil { - return 0, fmt.Errorf("create stderr pipe: %w", err) + return none, fmt.Errorf("create stderr pipe: %w", err) } // The read end must NOT be inherited by the child. _ = windows.SetHandleInformation(stderrRead, windows.HANDLE_FLAG_INHERIT, 0) @@ -329,7 +335,7 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand _ = windows.CloseHandle(stderrWrite) if err != nil { _ = windows.CloseHandle(stderrRead) - return 0, fmt.Errorf("CreateProcessAsUser: %w", err) + return none, fmt.Errorf("CreateProcessAsUser: %w", err) } if jobHandle != 0 { @@ -350,7 +356,7 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand _ = windows.TerminateProcess(pi.Process, 1) _ = windows.CloseHandle(pi.Process) _ = windows.CloseHandle(stderrRead) - return 0, fmt.Errorf("ResumeThread: %w", err) + return none, fmt.Errorf("ResumeThread: %w", err) } _ = windows.CloseHandle(pi.Thread) @@ -358,17 +364,19 @@ func spawnAgentInSession(sessionID uint32, socketPath, authToken string, jobHand go relogAgentOutput(stderrRead) log.Infof("spawned agent PID=%d in session %d on %s", pi.ProcessId, sessionID, socketPath) - return pi.Process, nil + return spawnedAgent{process: pi.Process, pid: pi.ProcessId}, nil } // sessionManager monitors the active console session and ensures a VNC agent // process is running in it. When the session changes (e.g., user switch, RDP // connect/disconnect), it kills the old agent and spawns a new one. Each -// spawn picks a per-session Unix-socket path the agent binds and the -// daemon dials over local IPC. +// spawn picks a fresh named-pipe name the agent serves and the daemon dials. type sessionManager struct { - mu sync.Mutex - agentProc windows.Handle + mu sync.Mutex + agentProc windows.Handle + // agentPID is the PID the agent's pipe server must report; it stays valid + // while agentProc is held open. + agentPID uint32 everSpawned bool agentStartedAt time.Time spawnFailures int @@ -380,30 +388,18 @@ type sessionManager struct { // jobHandle owns the agent processes via a Windows Job Object with // JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE. When the service exits or crashes, // the OS closes the handle and terminates every assigned agent: no - // orphaned agent processes holding a socket across restarts. + // orphaned agent processes holding a pipe across restarts. jobHandle windows.Handle } const ( - // agentSocketDirName is the dedicated subdirectory the agent socket lives - // in, created with agentSocketDirSDDL under the parent agentSocketParent - // picks. - agentSocketDirName = "netbird-vnc" - - // agentSocketDirSDDL grants full access to Local System (SY) and the - // Builtin Administrators group (BA) only, with the DACL protected - // (P) from inheritance so the parent's BUILTIN\Users grant does not - // flow in. AI is omitted; PAI marks the DACL protected and auto- - // inherited entries cleared. - agentSocketDirSDDL = "D:PAI(A;;FA;;;SY)(A;;FA;;;BA)" - - // agentSocketRandomLen is the number of random bytes mixed into each - // per-spawn socket name so the path is unguessable before the agent - // owns it. - agentSocketRandomLen = 16 + // agentPipeRandomLen is the number of random bytes mixed into each + // per-spawn pipe name, so a restarted agent never collides with a pipe a + // previous one left behind. + agentPipeRandomLen = 16 // agentReadyTimeout bounds how long the daemon waits for the freshly - // spawned agent to bind and accept on its socket before treating the + // spawned agent to create and accept on its pipe before treating the // spawn as failed. agentReadyTimeout = 5 * time.Second ) @@ -481,12 +477,10 @@ func createKillOnCloseJob() (windows.Handle, error) { return job, nil } -// Resolve returns the current agent socket path, shared token, and the -// uid the agent runs under (0 on Windows since the agent runs as -// SYSTEM in the interactive session; see validateAgentPeer for the -// Windows trust model). The path is only published after the spawned -// agent is confirmed listening, so a caller never receives a socket a -// squatter could be holding. When no agent is spawned yet (initial +// Resolve returns the current agent pipe path, shared token, and the PID +// the pipe server must report (see validateAgentPeer). The path is only +// published after the spawned agent is confirmed serving it. When no +// agent is spawned yet (initial // boot, between session switches, or permanently disabled when // SE_TCB_NAME is missing) it surfaces a distinct error so the daemon // can reject the connection with a meaningful message instead of timing @@ -499,10 +493,10 @@ func (m *sessionManager) Resolve(ctx context.Context) (string, string, uint32, e for { m.mu.Lock() - socketPath, token := m.socketPath, m.authToken + socketPath, token, pid := m.socketPath, m.authToken, m.agentPID m.mu.Unlock() if socketPath != "" { - return socketPath, token, 0, nil + return socketPath, token, pid, nil } // With no session on the console there is nothing to wait for: the @@ -619,6 +613,7 @@ func (m *sessionManager) reapExitedAgent() { log.Debugf("close agent handle: %v", err) } m.agentProc = 0 + m.agentPID = 0 m.authToken = "" m.socketPath = "" } @@ -649,30 +644,17 @@ func (m *sessionManager) maybeSpawnAgent(sid uint32) bool { return true } - if err := ensureAgentSocketDir(); err != nil { - log.Warnf("prepare agent socket dir: %v", err) - m.nextSpawnAt = time.Now().Add(5 * time.Second) - return true - } - - // The leaf name carries a cryptographically random component so a local - // user cannot pre-create the path at a guessable location. The session - // id is kept for diagnostics only; security does not rely on it. - socketPath, err := newAgentSocketPath(sid) + socketPath, err := newAgentPipePath(sid) if err != nil { - log.Warnf("generate agent socket path: %v", err) + log.Warnf("generate agent pipe path: %v", err) return true } - // Covers a previous-run crash that escaped Job Object kill-on-close. - if err := os.Remove(socketPath); err != nil && !os.IsNotExist(err) { - log.Debugf("clear stale agent socket %s: %v", socketPath, err) - } token, err := generateAuthToken() if err != nil { log.Warnf("generate agent auth token: %v", err) return true } - h, err := spawnAgentInSession(sid, socketPath, token, m.jobHandle) + agent, err := spawnAgentInSession(sid, socketPath, token, m.jobHandle) if err != nil { if errors.Is(err, windows.ERROR_PRIVILEGE_NOT_HELD) { // SE_TCB_NAME (token-impersonation across sessions) is only @@ -685,127 +667,61 @@ func (m *sessionManager) maybeSpawnAgent(sid uint32) bool { return true } - // Gate on listen-readiness before publishing the path: do not hand a - // caller a socket the agent has not bound yet. On timeout, fail closed - // by killing the agent and leaving socketPath/authToken unset so - // Resolve keeps returning errAgentNotReady. - if err := waitForAgentListening(socketPath, agentReadyTimeout); err != nil { + // Gate on readiness before publishing the path: do not hand a caller a + // pipe the agent is not serving yet. On failure, kill the agent and leave + // socketPath/authToken unset so Resolve keeps returning errAgentNotReady. + if err := waitForAgentListening(socketPath, agent.pid, agentReadyTimeout); err != nil { log.Warnf("agent in session %d did not start listening: %v", sid, err) - _ = windows.TerminateProcess(h, 1) - _ = windows.CloseHandle(h) - if rmErr := os.Remove(socketPath); rmErr != nil && !os.IsNotExist(rmErr) { - log.Debugf("clear unready agent socket %s: %v", socketPath, rmErr) - } + _ = windows.TerminateProcess(agent.process, 1) + _ = windows.CloseHandle(agent.process) m.scheduleNextSpawn(0, 0) return true } m.authToken = token m.socketPath = socketPath - m.agentProc = h + m.agentProc = agent.process + m.agentPID = agent.pid m.agentStartedAt = time.Now() m.everSpawned = true return true } -// ensureAgentSocketDir creates the dedicated socket directory with a -// restrictive DACL (SYSTEM + Administrators only). A pre-existing directory -// is torn down and recreated rather than reused: it may have been created by -// an unprivileged user with a permissive ACL, and it only ever holds our -// transient sockets, so removing it loses nothing. Fails closed: returns an -// error if the directory cannot be created with the intended security. -func ensureAgentSocketDir() error { - agentSocketDir := agentSocketDirPath() - sd, err := windows.SecurityDescriptorFromString(agentSocketDirSDDL) - if err != nil { - return fmt.Errorf("parse socket dir SDDL: %w", err) - } - var sa windows.SecurityAttributes - sa.Length = uint32(unsafe.Sizeof(sa)) - sa.SecurityDescriptor = sd - - dirW, err := windows.UTF16PtrFromString(agentSocketDir) - if err != nil { - return fmt.Errorf("encode socket dir path: %w", err) - } - err = windows.CreateDirectory(dirW, &sa) - if errors.Is(err, windows.ERROR_ALREADY_EXISTS) { - if rmErr := os.RemoveAll(agentSocketDir); rmErr != nil { - return fmt.Errorf("remove pre-existing socket dir %s: %w", agentSocketDir, rmErr) - } - err = windows.CreateDirectory(dirW, &sa) - } - if err != nil { - return fmt.Errorf("create socket dir %s: %w", agentSocketDir, err) - } - return nil -} - -// newAgentSocketPath returns a per-spawn socket path inside the secured -// socket directory. The leaf name mixes a cryptographically random component -// with the session id (for diagnostics) so the path is unguessable before the -// agent binds it. -func newAgentSocketPath(sessionID uint32) (string, error) { - b := make([]byte, agentSocketRandomLen) +// newAgentPipePath returns a per-spawn pipe path in the protected namespace. +// The random component keeps a respawned agent from colliding with a pipe a +// previous one still holds; the session id is there for diagnostics. +func newAgentPipePath(sessionID uint32) (string, error) { + b := make([]byte, agentPipeRandomLen) if _, err := crand.Read(b); err != nil { return "", fmt.Errorf("read random: %w", err) } - name := fmt.Sprintf("netbird-vnc-%d-%s.sock", sessionID, hex.EncodeToString(b)) - return filepath.Join(agentSocketDirPath(), name), nil + return fmt.Sprintf("%s%d-%s", agentPipePrefix, sessionID, hex.EncodeToString(b)), nil } -// agentSocketDirPath returns the directory the agent socket lives in. -// -// The parent is %SystemRoot%\SystemTemp where it exists: the temp directory -// Windows reserves for SYSTEM, with an ACL that admits SYSTEM and -// Administrators only. No unprivileged account can create anything in it, so a -// user cannot pre-create the socket directory, or a junction in its place, to -// intercept the daemon-to-agent stream. It is present on current Windows 11 and -// Server 2022 and later, and on Windows 10 and Server 2019 through servicing. -// -// Only where it is missing does this fall back to %SystemRoot%\Temp, whose ACL -// lets Users create entries. The protected DACL on the subdirectory and the -// tear-down-and-recreate in ensureAgentSocketDir are what hold there. -func agentSocketDirPath() string { - return filepath.Join(agentSocketParent(), agentSocketDirName) -} - -// agentSocketParent picks the parent directory for agentSocketDirPath. A -// SystemTemp that is a reparse point is not trusted, since only an -// administrator could have made it one and it no longer names the directory -// whose ACL is the point of using it. -func agentSocketParent() string { - winDir, err := windows.GetSystemWindowsDirectory() - if err != nil || winDir == "" { - winDir = `C:\Windows` - } - systemTemp := filepath.Join(winDir, "SystemTemp") - if info, err := os.Lstat(systemTemp); err == nil && info.IsDir() && info.Mode()&os.ModeType == os.ModeDir { - return systemTemp - } - return filepath.Join(winDir, "Temp") -} - -// waitForAgentListening dials the agent's Unix socket until it answers or the -// timeout elapses. Mirrors the darwin readiness gate so the daemon never -// exposes a socket path before the legitimate agent owns it. -func waitForAgentListening(socketPath string, wait time.Duration) error { - var d net.Dialer +// waitForAgentListening dials the agent's pipe until it answers from the +// expected process or the timeout elapses, so the daemon never publishes a +// pipe the spawned agent does not serve. +func waitForAgentListening(pipePath string, agentPID uint32, wait time.Duration) error { deadline := time.Now().Add(wait) var lastErr error for time.Now().Before(deadline) { - c, err := d.Dial("unix", socketPath) + ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond) + c, err := dialAgent(ctx, pipePath) + cancel() if err == nil { - _ = c.Close() - return nil + err = validateAgentPeer(c, agentPID) + if closeErr := c.Close(); closeErr != nil { + log.Debugf("close agent readiness probe: %v", closeErr) + } + return err } lastErr = err time.Sleep(100 * time.Millisecond) } if lastErr == nil { - lastErr = fmt.Errorf("timeout") + lastErr = errors.New("timeout") } - return fmt.Errorf("dial %s: %w", socketPath, lastErr) + return fmt.Errorf("dial %s: %w", pipePath, lastErr) } func (m *sessionManager) killAgent() { @@ -815,6 +731,7 @@ func (m *sessionManager) killAgent() { _ = windows.TerminateProcess(m.agentProc, 0) _ = windows.CloseHandle(m.agentProc) m.agentProc = 0 + m.agentPID = 0 m.authToken = "" m.socketPath = "" log.Info("killed old agent") diff --git a/client/vnc/server/service_agent.go b/client/vnc/server/service_agent.go index f4715779b..189d71db3 100644 --- a/client/vnc/server/service_agent.go +++ b/client/vnc/server/service_agent.go @@ -3,16 +3,15 @@ package server import "context" // sessionAgent abstracts the per-platform manager that spawns and tracks -// the user-session VNC agent. Resolve returns the agent's Unix-socket -// path, the shared per-spawn token, and the uid the agent was spawned -// under (used to validate peer credentials before the daemon hands the -// token to whoever is on the other end of the socket). Resolve may spawn -// the agent lazily. +// the user-session VNC agent. Resolve returns the agent's socket path (a +// named pipe on Windows), the shared per-spawn token, and the peer identity +// the daemon expects on the other end: the uid the agent runs under on +// darwin, the agent's PID on Windows. Resolve may spawn the agent lazily. // Release reports that one proxied connection is done with the agent, so a // platform that recycles the agent per connection can tear it down once the last // one is gone. Every successful Resolve owes exactly one Release. type sessionAgent interface { - Resolve(ctx context.Context) (socketPath, token string, peerUID uint32, err error) + Resolve(ctx context.Context) (socketPath, token string, peerID uint32, err error) Release() }