Harden VNC server, IPC, and management plumbing

client/cmd/vnc_agent.go

@@ -14,10 +14,14 @@ import (
 	vncserver "github.com/netbirdio/netbird/client/vnc/server"
 )
 
-var vncAgentSocket string
+var (
+	vncAgentSocket    string
+	vncAgentTargetUID uint32
+)
 
 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 should drop privileges to before listening (darwin only; 0 = stay as current uid)")
 	rootCmd.AddCommand(vncAgentCmd)
 }
 
@@ -47,6 +51,18 @@ var vncAgentCmd = &cobra.Command{
 			log.Debugf("unset NB_VNC_AGENT_TOKEN: %v", err)
 		}
 
+		// Drop root privileges to the target console user BEFORE creating
+		// the listening socket: keeps a post-auth bug in the encoder /
+		// input / capture paths confined to the user's own privileges
+		// rather than escalating to host root, and makes the daemon's
+		// LOCAL_PEERCRED check see the right uid. No-op on Windows
+		// (both processes run as SYSTEM) and when --target-uid is 0.
+		if vncAgentTargetUID != 0 {
+			if err := dropAgentPrivileges(vncAgentTargetUID); err != nil {
+				return fmt.Errorf("drop privileges to uid %d: %w", vncAgentTargetUID, err)
+			}
+		}
+
 		if err := os.Remove(vncAgentSocket); err != nil && !os.IsNotExist(err) {
 			log.Debugf("remove stale socket %s: %v", vncAgentSocket, err)
 		}

client/cmd/vnc_agent_dropprivs_darwin.go

@@ -0,0 +1,50 @@
+//go:build darwin && !ios
+
+package cmd
+
+import (
+	"fmt"
+	"os"
+	"syscall"
+)
+
+// dropAgentPrivileges drops the vnc-agent process from root (its
+// launchctl-asuser-inherited starting uid) to the target console user
+// before any other initialisation runs. Without this the agent runs as
+// root for the lifetime of the session; any post-auth memory-safety
+// issue in the capture/input/encode paths would then be a root-level
+// RCE on the host instead of a user-level one. Also makes the daemon's
+// LOCAL_PEERCRED check correctly identify the agent as the console user,
+// not as root.
+//
+// Returns an error when the agent is running as a non-root uid that
+// differs from targetUID: non-root can only setuid to itself, so a
+// mismatch here means the spawn went to the wrong session.
+func dropAgentPrivileges(targetUID uint32) error {
+	if targetUID == 0 {
+		return fmt.Errorf("refusing to keep agent running as root (target uid 0)")
+	}
+	cur := uint32(os.Getuid())
+	if cur == targetUID {
+		return nil
+	}
+	if cur != 0 {
+		return fmt.Errorf("agent uid %d does not match expected %d and we lack root to fix it", cur, targetUID)
+	}
+	// Drop supplementary groups first: setgid alone doesn't touch the
+	// auxiliary group list, leaving root's groups attached would let the
+	// dropped process write to root-only group-writable files.
+	if err := syscall.Setgroups([]int{}); err != nil {
+		return fmt.Errorf("setgroups([]): %w", err)
+	}
+	if err := syscall.Setgid(int(targetUID)); err != nil {
+		return fmt.Errorf("setgid(%d): %w", targetUID, err)
+	}
+	if err := syscall.Setuid(int(targetUID)); err != nil {
+		return fmt.Errorf("setuid(%d): %w", targetUID, err)
+	}
+	if uint32(os.Getuid()) != targetUID || uint32(os.Geteuid()) != targetUID {
+		return fmt.Errorf("setuid verification: uid=%d euid=%d, expected %d", os.Getuid(), os.Geteuid(), targetUID)
+	}
+	return nil
+}

client/cmd/vnc_agent_dropprivs_darwin_test.go

@@ -0,0 +1,55 @@
+//go:build darwin && !ios
+
+package cmd
+
+import (
+	"strings"
+	"testing"
+)
+
+// TestDropAgentPrivileges_RefusesRootTarget locks in the contract that
+// dropAgentPrivileges must never be a no-op when asked to keep the
+// agent as root (target uid 0). A future caller that passes 0 by
+// mistake would otherwise leave the post-auth attack surface running
+// with full root privileges.
+func TestDropAgentPrivileges_RefusesRootTarget(t *testing.T) {
+	err := dropAgentPrivileges(0)
+	if err == nil {
+		t.Fatal("expected refusal for target uid 0, got nil")
+	}
+	if !strings.Contains(err.Error(), "root") {
+		t.Fatalf("error should mention root, got: %v", err)
+	}
+}
+
+// TestDropAgentPrivileges_NoOpWhenAlreadyTarget covers the dev path
+// where the agent is launched by hand as the target user (no root
+// available, no setuid needed). The helper must succeed silently
+// instead of trying (and failing) a setuid to its current uid.
+func TestDropAgentPrivileges_NoOpWhenAlreadyTarget(t *testing.T) {
+	// Skip when running as root: the early-return path we want to
+	// cover only fires when current uid == target uid.
+	uid := currentUIDForTest()
+	if uid == 0 {
+		t.Skip("test must not run as root; cannot exercise the no-op early-return")
+	}
+	if err := dropAgentPrivileges(uid); err != nil {
+		t.Fatalf("expected no-op when current uid == target, got: %v", err)
+	}
+}
+
+// TestDropAgentPrivileges_RefusesMismatchedNonRoot guards the "non-root
+// caller tries to setuid to a different uid" path: setuid would fail
+// with EPERM anyway, but the helper should surface a clear error
+// before issuing the syscall so a misconfigured spawn (wrong --target-uid
+// flag) is debuggable.
+func TestDropAgentPrivileges_RefusesMismatchedNonRoot(t *testing.T) {
+	uid := currentUIDForTest()
+	if uid == 0 {
+		t.Skip("test must not run as root; covered case requires non-root caller")
+	}
+	err := dropAgentPrivileges(uid + 1)
+	if err == nil {
+		t.Fatal("expected refusal when non-root caller asks to setuid elsewhere")
+	}
+}

client/cmd/vnc_agent_dropprivs_testhelpers_darwin.go

@@ -0,0 +1,11 @@
+//go:build darwin && !ios
+
+package cmd
+
+import "os"
+
+// currentUIDForTest exposes os.Getuid for the darwin dropprivs tests
+// without leaking an os import into the test file itself.
+func currentUIDForTest() uint32 {
+	return uint32(os.Getuid())
+}

client/cmd/vnc_agent_dropprivs_windows.go

@@ -0,0 +1,14 @@
+//go:build windows
+
+package cmd
+
+// dropAgentPrivileges is a no-op on Windows: the agent and the daemon
+// both run as SYSTEM (the daemon spawns the agent into the interactive
+// session via CreateProcessAsUser with an impersonation token, but the
+// resulting process still runs under SYSTEM, not under the user's
+// account). The Windows path relies on the C:\Windows\Temp socket
+// location (admin/SYSTEM-write-only) and the per-spawn token for
+// integrity instead.
+func dropAgentPrivileges(_ uint32) error {
+	return nil
+}

client/internal/approval/broker.go

@@ -28,6 +28,37 @@ const (
 	MetaExpiresAt = "expires_at"
 )
 
+// ShortKeyFingerprint formats a hex-encoded Noise_IK static pubkey as a
+// short, eyeball-able fingerprint to display in the approval dialog.
+// The dashboard-supplied display name attached to a SessionPubKey isn't
+// cryptographically asserted by the connecting client, so the prompt
+// must also show something that IS: the key fingerprint, a hash of
+// the static public key the client just proved possession of during the
+// Noise handshake. Returns the empty string when the input is too short
+// to plausibly be a hex pubkey, so the row is omitted rather than
+// rendered as a misleading partial.
+//
+// Output format: 16 hex chars grouped as XXXX-XXXX-XXXX-XXXX (64 bits of
+// fingerprint, resistant to random-prefix collisions and easy for a human
+// to compare with an out-of-band reference).
+func ShortKeyFingerprint(hexKey string) string {
+	if len(hexKey) < 8 {
+		return ""
+	}
+	src := hexKey
+	if len(src) > 16 {
+		src = src[:16]
+	}
+	var out []byte
+	for i, c := range src {
+		if i > 0 && i%4 == 0 {
+			out = append(out, '-')
+		}
+		out = append(out, byte(c))
+	}
+	return string(out)
+}
+
 // Kind values for the well-known prompt subjects. New subsystems should
 // add a constant here so the UI can dispatch on a known string.
 const (

client/internal/approval/fingerprint_test.go

@@ -0,0 +1,62 @@
+package approval
+
+import "testing"
+
+// TestShortKeyFingerprint locks in the format the VNC approval prompt
+// shows to the user. The fingerprint is the user's only cryptographic
+// anchor against a malicious management server that pushes a spoofed
+// display name, so accidental changes to its format would silently
+// undermine that defence.
+func TestShortKeyFingerprint(t *testing.T) {
+	cases := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{
+			name: "full_32_byte_pubkey",
+			in:   "0123456789abcdeffedcba9876543210ffeeddccbbaa99887766554433221100",
+			want: "0123-4567-89ab-cdef",
+		},
+		{
+			name: "exactly_16_chars",
+			in:   "0123456789abcdef",
+			want: "0123-4567-89ab-cdef",
+		},
+		{
+			name: "borderline_8_chars",
+			in:   "01234567",
+			want: "0123-4567",
+		},
+		{
+			name: "too_short_returns_empty",
+			in:   "0123",
+			want: "",
+		},
+		{
+			name: "empty_returns_empty",
+			in:   "",
+			want: "",
+		},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := ShortKeyFingerprint(tc.in)
+			if got != tc.want {
+				t.Fatalf("ShortKeyFingerprint(%q) = %q, want %q", tc.in, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestShortKeyFingerprint_DistinctKeysDistinctOutputs guards against a
+// formatting bug that would collapse different prefixes onto the same
+// displayed fingerprint and let an attacker substitute their pubkey for
+// a victim's while keeping the prompt visually identical.
+func TestShortKeyFingerprint_DistinctKeysDistinctOutputs(t *testing.T) {
+	a := ShortKeyFingerprint("0123456789abcdef" + "rest_of_pubkey_ignored")
+	b := ShortKeyFingerprint("0123456789abcde0" + "rest_of_pubkey_ignored")
+	if a == b {
+		t.Fatalf("expected distinct outputs for distinct prefixes, both = %q", a)
+	}
+}

client/ui/approval.go

@@ -13,6 +13,7 @@ import (
 	"fyne.io/fyne/v2/widget"
 	log "github.com/sirupsen/logrus"
 
+	"github.com/netbirdio/netbird/client/internal/approval"
 	"github.com/netbirdio/netbird/client/proto"
 )
 
@@ -38,6 +39,7 @@ func (s *serviceClient) handleApprovalEvent(ev *proto.SystemEvent) {
 		"--approval-source-ip=" + ev.Metadata["source_ip"],
 		"--approval-username=" + ev.Metadata["username"],
 		"--approval-expires-at=" + ev.Metadata["expires_at"],
+		"--approval-key-fingerprint=" + ev.Metadata["peer_pubkey"],
 		"--approval-subject=" + ev.UserMessage,
 	}
 	go s.eventHandler.runSelfCommand(s.ctx, "approval", args...)
@@ -53,8 +55,17 @@ func (s *serviceClient) showApprovalUI(req approvalRequest) {
 
 	var rows []string
 	if req.initiator != "" {
+		// The display name comes from the management dashboard and is
+		// not cryptographically asserted by the connecting client. The
+		// key fingerprint that follows IS: it's the Noise_IK static
+		// public key the client just proved possession of. Show both
+		// so the user can sanity-check that "Alice" is really the
+		// Alice they trust.
 		rows = append(rows, "From user:  "+req.initiator)
 	}
+	if fp := approval.ShortKeyFingerprint(req.keyFingerprint); fp != "" {
+		rows = append(rows, "Key fp:     "+fp)
+	}
 	if req.peerName != "" {
 		rows = append(rows, "Via peer:   "+req.peerName)
 	}
@@ -149,14 +160,15 @@ func (s *serviceClient) sendApprovalResponse(requestID string, accept, viewOnly
 // approvalRequest is the parsed --approval-* CLI args that the forked
 // dialog process consumes.
 type approvalRequest struct {
-	requestID string
-	kind      string
-	initiator string
-	peerName  string
-	sourceIP  string
-	username  string
-	subject   string
-	expiresAt string
+	requestID      string
+	kind           string
+	initiator      string
+	peerName       string
+	sourceIP       string
+	username       string
+	subject        string
+	expiresAt      string
+	keyFingerprint string
 }
 
 func (r approvalRequest) displayPeer() string {

client/ui/client_ui.go

@@ -99,14 +99,15 @@ func main() {
 		showUpdateVersion: flags.showUpdateVersion,
 		showApproval:      flags.showApproval,
 		approvalRequest: approvalRequest{
-			requestID: flags.approvalRequestID,
-			kind:      flags.approvalKind,
-			initiator: flags.approvalInitiator,
-			peerName:  flags.approvalPeerName,
-			sourceIP:  flags.approvalSourceIP,
-			username:  flags.approvalUsername,
-			subject:   flags.approvalSubject,
-			expiresAt: flags.approvalExpiresAt,
+			requestID:      flags.approvalRequestID,
+			kind:           flags.approvalKind,
+			initiator:      flags.approvalInitiator,
+			peerName:       flags.approvalPeerName,
+			sourceIP:       flags.approvalSourceIP,
+			username:       flags.approvalUsername,
+			subject:        flags.approvalSubject,
+			expiresAt:      flags.approvalExpiresAt,
+			keyFingerprint: flags.approvalKeyFingerprint,
 		},
 	})
 
@@ -153,14 +154,15 @@ type cliFlags struct {
 	showUpdateVersion string
 	showApproval      bool
 
-	approvalRequestID string
-	approvalKind      string
-	approvalInitiator string
-	approvalPeerName  string
-	approvalSourceIP  string
-	approvalUsername  string
-	approvalSubject   string
-	approvalExpiresAt string
+	approvalRequestID      string
+	approvalKind           string
+	approvalInitiator      string
+	approvalPeerName       string
+	approvalSourceIP       string
+	approvalUsername       string
+	approvalSubject        string
+	approvalExpiresAt      string
+	approvalKeyFingerprint string
 }
 
 // parseFlags reads and returns all needed command-line flags.
@@ -191,6 +193,7 @@ func parseFlags() *cliFlags {
 	flag.StringVar(&flags.approvalUsername, "approval-username", "", "approval prompt: requested OS username")
 	flag.StringVar(&flags.approvalSubject, "approval-subject", "", "approval prompt: human-readable subject line")
 	flag.StringVar(&flags.approvalExpiresAt, "approval-expires-at", "", "approval prompt: RFC3339 deadline at which the daemon auto-denies")
+	flag.StringVar(&flags.approvalKeyFingerprint, "approval-key-fingerprint", "", "approval prompt: hex-encoded Noise static pubkey of the connecting client")
 	flag.Parse()
 	return &flags
 }

client/vnc/server/agent_darwin.go

@@ -43,11 +43,11 @@ func newDarwinAgentManager(ctx context.Context) *darwinAgentManager {
 	return m
 }
 
-// agentSocketPathFmt parameterizes the agent's loopback Unix-socket path
-// by the console uid: /tmp is writable in the launchctl-asuser context
-// and predictable to the daemon. The agent chmods the file 0600 after
-// bind so only its uid (plus root) can dial.
-const agentSocketPathFmt = "/tmp/netbird-vnc-%d.sock"
+// agentSocketName is the file name inside the per-uid socket directory
+// the agent binds. The directory itself is created and chowned by the
+// daemon (see prepareAgentSocketDir) so a non-root local user cannot
+// pre-create or symlink the path before the agent listens.
+const agentSocketName = "agent.sock"
 
 // watchConsoleUser kills the cached agent whenever the console user
 // changes (logout, fast user switch, login window). Without it the daemon
@@ -87,44 +87,112 @@ func (m *darwinAgentManager) watchConsoleUser(ctx context.Context) {
 }
 
 // Resolve spawns or respawns the per-user agent process as needed and
-// returns its Unix-socket path and shared token. Each call is serialized
-// so concurrent VNC clients share the same agent.
-func (m *darwinAgentManager) Resolve(ctx context.Context) (string, string, error) {
+// returns its Unix-socket path, shared token, and the uid the agent was
+// spawned under (so the daemon can validate peer credentials before
+// dispatching the token). Each call is serialized so concurrent VNC
+// clients share the same agent.
+func (m *darwinAgentManager) Resolve(ctx context.Context) (string, string, uint32, error) {
 	consoleUID, err := consoleUserID()
 	if err != nil {
-		return "", "", fmt.Errorf("no console user: %w", err)
+		return "", "", 0, fmt.Errorf("no console user: %w", err)
 	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if m.running && m.uid == consoleUID && vncAgentRunning() {
-		return m.socketPath, m.authToken, nil
+		return m.socketPath, m.authToken, m.uid, nil
 	}
 	m.killLocked()
 	// Reap stray agents so the new token is the only accepted one.
 	killAllVNCAgents()
 
-	socketPath := fmt.Sprintf(agentSocketPathFmt, consoleUID)
+	socketDir, err := prepareAgentSocketDir(consoleUID)
+	if err != nil {
+		return "", "", 0, fmt.Errorf("prepare agent socket dir: %w", err)
+	}
+	socketPath := socketDir + "/" + agentSocketName
 	if err := os.Remove(socketPath); err != nil && !errors.Is(err, os.ErrNotExist) {
 		log.Debugf("clear stale agent socket %s: %v", socketPath, err)
 	}
 
 	token, err := generateAuthToken()
 	if err != nil {
-		return "", "", fmt.Errorf("generate agent auth token: %w", err)
+		return "", "", 0, fmt.Errorf("generate agent auth token: %w", err)
 	}
 	if err := spawnAgentForUser(consoleUID, socketPath, token); err != nil {
-		return "", "", err
+		return "", "", 0, err
 	}
 	if err := waitForAgent(ctx, socketPath, 5*time.Second); err != nil {
 		killAllVNCAgents()
-		return "", "", fmt.Errorf("agent did not start listening: %w", err)
+		return "", "", 0, fmt.Errorf("agent did not start listening: %w", err)
 	}
 	m.authToken = token
 	m.socketPath = socketPath
 	m.uid = consoleUID
 	m.running = true
 	log.Infof("spawned VNC agent for console uid=%d on %s", consoleUID, socketPath)
-	return socketPath, token, nil
+	return socketPath, token, consoleUID, nil
+}
+
+// agentSocketParentDir is the root the daemon creates (as root, mode 0755)
+// to hold per-uid agent-socket subdirectories. Keeping it under
+// /var/run/netbird-vnc (rather than /tmp) means a non-root local user
+// cannot squat the socket path: only root can create the parent, and
+// only the target user (plus root) can write inside the per-uid subdir.
+const agentSocketParentDir = "/var/run/netbird-vnc"
+
+// prepareAgentSocketDir creates (and tightens permissions on) a per-uid
+// subdirectory the agent will bind its socket inside, returning the
+// directory path. The subdirectory is owned by uid with mode 0700, so
+// the only writers are the target user and root. The parent is created
+// root-owned with mode 0755 if it doesn't already exist. Symlinks at
+// the per-uid level are refused (replaced with a fresh directory) to
+// avoid a low-priv user redirecting our chown.
+func prepareAgentSocketDir(uid uint32) (string, error) {
+	if err := os.MkdirAll(agentSocketParentDir, 0o755); err != nil {
+		return "", fmt.Errorf("mkdir %s: %w", agentSocketParentDir, err)
+	}
+	// Refuse to use the parent if it's a symlink or not owned by root.
+	pInfo, err := os.Lstat(agentSocketParentDir)
+	if err != nil {
+		return "", fmt.Errorf("lstat %s: %w", agentSocketParentDir, err)
+	}
+	if pInfo.Mode()&os.ModeSymlink != 0 {
+		return "", fmt.Errorf("%s is a symlink", agentSocketParentDir)
+	}
+	if st, ok := pInfo.Sys().(*syscall.Stat_t); ok && st.Uid != 0 {
+		return "", fmt.Errorf("%s not owned by root (uid=%d)", agentSocketParentDir, st.Uid)
+	}
+
+	subdir := fmt.Sprintf("%s/%d", agentSocketParentDir, uid)
+	// If a leftover entry exists, refuse it unless it's a real dir owned
+	// by the right uid with strict perms: otherwise remove and recreate
+	// from scratch under our control. Using os.Lstat (not Stat) so a
+	// symlink is detected and torn down.
+	if info, err := os.Lstat(subdir); err == nil {
+		bad := false
+		if info.Mode()&os.ModeSymlink != 0 {
+			bad = true
+		} else if !info.IsDir() {
+			bad = true
+		} else if st, ok := info.Sys().(*syscall.Stat_t); !ok || st.Uid != uid || info.Mode().Perm() != 0o700 {
+			bad = true
+		}
+		if bad {
+			if err := os.RemoveAll(subdir); err != nil {
+				return "", fmt.Errorf("remove stale %s: %w", subdir, err)
+			}
+		}
+	}
+	if err := os.Mkdir(subdir, 0o700); err != nil && !errors.Is(err, os.ErrExist) {
+		return "", fmt.Errorf("mkdir %s: %w", subdir, err)
+	}
+	if err := os.Chmod(subdir, 0o700); err != nil {
+		return "", fmt.Errorf("chmod %s: %w", subdir, err)
+	}
+	if err := os.Chown(subdir, int(uid), -1); err != nil {
+		return "", fmt.Errorf("chown %s -> uid %d: %w", subdir, uid, err)
+	}
+	return subdir, nil
 }
 
 // stop terminates the spawned agent, if any. Intended for daemon shutdown.
@@ -182,7 +250,14 @@ func spawnAgentForUser(uid uint32, socketPath, token string) error {
 	}
 	cmd := exec.Command(
 		"/bin/launchctl", "asuser", strconv.FormatUint(uint64(uid), 10),
-		exe, vncAgentSubcommand, "--socket", socketPath,
+		exe, vncAgentSubcommand,
+		"--socket", socketPath,
+		// Drop privs inside the agent: launchctl asuser preserves the
+		// daemon's uid (root), so without this the capture/input/
+		// encoder paths would run as root for the lifetime of the
+		// session. validateAgentPeer on the daemon side also relies on
+		// the agent's effective uid matching consoleUID.
+		"--target-uid", strconv.FormatUint(uint64(uid), 10),
 	)
 	cmd.Env = append(os.Environ(), agentTokenEnvVar+"="+token)
 	stderr, err := cmd.StderrPipe()

client/vnc/server/agent_ipc.go

@@ -26,9 +26,12 @@ var errNoConsoleUser = errors.New("no user logged into console")
 
 // sessionAgent abstracts the per-platform manager that spawns and tracks
 // the user-session VNC agent. Resolve returns the agent's Unix-socket
-// path and shared token, possibly spawning lazily.
+// 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.
 type sessionAgent interface {
-	Resolve(ctx context.Context) (socketPath, token string, err error)
+	Resolve(ctx context.Context) (socketPath, token string, peerUID uint32, err error)
 }
 
 // prefixConn replays already-consumed header bytes ahead of the proxy
@@ -70,7 +73,11 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) {
 		authedLog.Info("VNC connection rejected: auth failed")
 		return
 	}
-	s.registerConnAuth(conn, header)
+	if err := s.registerConnAuth(conn, header); err != nil {
+		rejectConnection(conn, codeMessage(RejectCodeAuthForbidden, err.Error()))
+		authedLog.Warnf("VNC connection rejected: %v", err)
+		return
+	}
 
 	decision, err := s.gateApproval(conn, header)
 	if err != nil {
@@ -83,7 +90,7 @@ func (s *Server) handleServiceConnection(conn net.Conn, sa sessionAgent) {
 		authedLog.Info("VNC connection approved by user")
 	}
 
-	socketPath, token, err := sa.Resolve(s.ctx)
+	socketPath, token, peerUID, err := sa.Resolve(s.ctx)
 	if err != nil {
 		code := RejectCodeCapturerError
 		if errors.Is(err, errNoConsoleUser) {
@@ -98,7 +105,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, decision.ViewOnly); err != nil {
+	if err := proxyToAgent(s.ctx, replayConn, socketPath, token, peerUID, decision.ViewOnly, authedLog); err != nil {
 		rejectConnection(conn, codeMessage(RejectCodeCapturerError, err.Error()))
 		authedLog.Warnf("VNC connection rejected: agent unreachable: %v", err)
 		return
@@ -134,14 +141,18 @@ func generateAuthToken() (string, error) {
 	return hex.EncodeToString(b), nil
 }
 
-// proxyToAgent dials the per-session agent's Unix socket, writes the
-// raw token bytes plus a single view-only flag byte, then copies bytes
-// both ways until either side closes. The token + flag prefix must
-// precede any RFB byte so the agent's verifyAgentToken can run first.
-// Returns nil once a stream is established; the caller is responsible
-// for sending an RFB-level rejection on error so the client sees a
-// reason instead of a bare timeout.
-func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken string, viewOnly bool) error {
+// proxyToAgent dials the per-session agent's Unix socket, validates the
+// peer's kernel-asserted uid (so the daemon never hands its per-spawn
+// token to an impostor that won the listen race), writes the raw token
+// bytes plus a single view-only flag byte, then copies bytes both ways
+// until either side closes. The token + flag prefix must precede any RFB
+// byte so the agent's verifyAgentToken can run first. Returns nil once a
+// stream is established; the caller is responsible for sending an
+// RFB-level rejection on error so the client sees a reason instead of a
+// bare timeout. authedLog receives one audit line per dispatched
+// preamble 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 {
 	tokenBytes, err := hex.DecodeString(authToken)
 	if err != nil || len(tokenBytes) != agentTokenLen {
 		return fmt.Errorf("invalid auth token (len=%d): %w", len(tokenBytes), err)
@@ -152,6 +163,11 @@ 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 {
+		_ = agentConn.Close()
+		return fmt.Errorf("agent peer validation failed: %w", err)
+	}
+
 	preamble := make([]byte, len(tokenBytes)+1)
 	copy(preamble, tokenBytes)
 	if viewOnly {
@@ -162,6 +178,17 @@ func proxyToAgent(ctx context.Context, client net.Conn, socketPath, authToken st
 		return fmt.Errorf("send auth preamble to agent: %w", err)
 	}
 
+	// Audit: one line per successfully-dispatched daemon→agent preamble.
+	// Token printed as its first 8 hex chars (enough to correlate, not
+	// enough to use). Kept at Info so the default deployment captures it.
+	tokenFp := authToken
+	if len(tokenFp) > 8 {
+		tokenFp = tokenFp[:8]
+	}
+	if authedLog != nil {
+		authedLog.Infof("VNC IPC: dispatched preamble to agent socket=%s peer_uid=%d view_only=%v token_fp=%s", socketPath, peerUID, viewOnly, tokenFp)
+	}
+
 	defer client.Close()
 	defer agentConn.Close()
 	log.Debugf("proxy connected to agent, starting bidirectional copy")

client/vnc/server/agent_peercred_darwin.go

@@ -0,0 +1,46 @@
+//go:build darwin && !ios
+
+package server
+
+import (
+	"fmt"
+	"net"
+
+	"golang.org/x/sys/unix"
+)
+
+// validateAgentPeer enforces that the peer behind the just-connected Unix
+// socket is the agent we expect it to be: a process running under
+// expectedUID, with the right effective uid stamped by the kernel on the
+// socket. Refuses (with a non-nil error) if anything else is listening on
+// the path (an unrelated local process that won the listen race or
+// squatted the path before us). Defends against the daemon shipping its
+// per-spawn auth token to a process that isn't the spawned agent.
+func validateAgentPeer(conn net.Conn, expectedUID uint32) error {
+	uconn, ok := conn.(*net.UnixConn)
+	if !ok {
+		return fmt.Errorf("peer cred: expected *net.UnixConn, got %T", conn)
+	}
+	raw, err := uconn.SyscallConn()
+	if err != nil {
+		return fmt.Errorf("peer cred: syscall conn: %w", err)
+	}
+	var cred *unix.Xucred
+	var inner error
+	ctlErr := raw.Control(func(fd uintptr) {
+		cred, inner = unix.GetsockoptXucred(int(fd), unix.SOL_LOCAL, unix.LOCAL_PEERCRED)
+	})
+	if ctlErr != nil {
+		return fmt.Errorf("peer cred: control: %w", ctlErr)
+	}
+	if inner != nil {
+		return fmt.Errorf("peer cred: getsockopt LOCAL_PEERCRED: %w", inner)
+	}
+	if cred == nil {
+		return fmt.Errorf("peer cred: nil xucred")
+	}
+	if cred.Uid != expectedUID {
+		return fmt.Errorf("peer cred: agent uid %d does not match expected %d", cred.Uid, expectedUID)
+	}
+	return nil
+}

client/vnc/server/agent_peercred_darwin_test.go

@@ -0,0 +1,115 @@
+//go:build darwin && !ios
+
+package server
+
+import (
+	"net"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+)
+
+// TestValidateAgentPeerAcceptsOwnUID confirms the happy path: a Unix
+// socket whose peer is the current process must validate when the
+// expected uid matches the process's own. Both sides of a unix-socket
+// pair share the same kernel cred, so this exercises the real getsockopt
+// LOCAL_PEERCRED path.
+func TestValidateAgentPeerAcceptsOwnUID(t *testing.T) {
+	dir := t.TempDir()
+	sockPath := filepath.Join(dir, "test.sock")
+	ln, err := net.Listen("unix", sockPath)
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	defer ln.Close()
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		c, err := ln.Accept()
+		if err == nil {
+			_ = c.Close()
+		}
+	}()
+	c, err := net.Dial("unix", sockPath)
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer c.Close()
+
+	if err := validateAgentPeer(c, uint32(os.Getuid())); err != nil {
+		t.Fatalf("validateAgentPeer rejected own uid: %v", err)
+	}
+	wg.Wait()
+}
+
+// TestValidateAgentPeerRejectsWrongUID ensures the validator fails when
+// the expected uid differs from the kernel-reported peer uid. This is
+// the path that catches a hostile process that won the listen race.
+func TestValidateAgentPeerRejectsWrongUID(t *testing.T) {
+	dir := t.TempDir()
+	sockPath := filepath.Join(dir, "test.sock")
+	ln, err := net.Listen("unix", sockPath)
+	if err != nil {
+		t.Fatalf("listen: %v", err)
+	}
+	defer ln.Close()
+
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		c, err := ln.Accept()
+		if err == nil {
+			_ = c.Close()
+		}
+	}()
+	c, err := net.Dial("unix", sockPath)
+	if err != nil {
+		t.Fatalf("dial: %v", err)
+	}
+	defer c.Close()
+
+	// Pick a uid the test process certainly isn't running as.
+	wrongUID := uint32(os.Getuid()) + 1
+	err = validateAgentPeer(c, wrongUID)
+	if err == nil {
+		t.Fatal("expected mismatch error, got nil")
+	}
+	if !strings.Contains(err.Error(), "does not match expected") {
+		t.Fatalf("error should mention uid mismatch, got: %v", err)
+	}
+	wg.Wait()
+}
+
+// TestValidateAgentPeerRejectsNonUnix protects against being handed a
+// non-Unix-socket connection (the validator can't enforce anything on
+// e.g. a *net.TCPConn so it must refuse rather than silently pass).
+func TestValidateAgentPeerRejectsNonUnix(t *testing.T) {
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatalf("listen tcp: %v", err)
+	}
+	defer ln.Close()
+	var wg sync.WaitGroup
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		c, err := ln.Accept()
+		if err == nil {
+			_ = c.Close()
+		}
+	}()
+	c, err := net.Dial("tcp", ln.Addr().String())
+	if err != nil {
+		t.Fatalf("dial tcp: %v", err)
+	}
+	defer c.Close()
+	if err := validateAgentPeer(c, 0); err == nil {
+		t.Fatal("expected refusal on non-unix conn, got nil")
+	}
+	wg.Wait()
+}

client/vnc/server/agent_peercred_windows.go

@@ -0,0 +1,19 @@
+//go:build windows
+
+package server
+
+import (
+	"net"
+)
+
+// validateAgentPeer is a best-effort no-op on Windows: AF_UNIX sockets on
+// Windows do not expose SO_PEERCRED equivalents, and both the daemon and
+// the spawned agent run as SYSTEM in distinct sessions. The remaining
+// trust comes from the location of the socket file (under
+// C:\Windows\Temp, writable only by SYSTEM/Administrators) and from the
+// per-spawn auth token preamble that follows this call. Documented as a
+// known gap; a future hardening pass could interrogate the connected
+// pipe's PID via process-token APIs.
+func validateAgentPeer(_ net.Conn, _ uint32) error {
+	return nil
+}

client/vnc/server/agent_windows.go

@@ -421,18 +421,20 @@ func createKillOnCloseJob() (windows.Handle, error) {
 	return job, nil
 }
 
-// Resolve returns the current agent socket path and token. 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 out the proxy dial.
-func (m *sessionManager) Resolve(_ context.Context) (string, string, error) {
+// 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; validateAgentPeer is a no-op
+// there). 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 out the proxy dial.
+func (m *sessionManager) Resolve(_ context.Context) (string, string, uint32, error) {
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if m.socketPath == "" {
-		return "", "", errAgentNotReady
+		return "", "", 0, errAgentNotReady
 	}
-	return m.socketPath, m.authToken, nil
+	return m.socketPath, m.authToken, 0, nil
 }
 
 var errAgentNotReady = errors.New("VNC agent not running yet")

client/vnc/server/copyrect.go

@@ -156,9 +156,21 @@ func (d *copyRectDetector) findTileMatch(cur *image.RGBA, dstX, dstY int) (int,
 	if pos[0] == dstX && pos[1] == dstY {
 		return 0, 0, false
 	}
+	// Reject source coords that fall outside the current framebuffer
+	// (frame may have shrunk since the source position was recorded). A
+	// CopyRect with an out-of-range source would have the client copy
+	// from undefined pixels, so drop the match and let the encoder send
+	// the rect normally.
+	if pos[0] < 0 || pos[1] < 0 || pos[0]+ts > cur.Rect.Dx() || pos[1]+ts > cur.Rect.Dy() {
+		return 0, 0, false
+	}
 	// Reject stale entries: the position the map points at must still
 	// carry the same hash according to our per-tile array.
-	if d.tileHash[(pos[1]/ts)*d.cols+(pos[0]/ts)] != sum {
+	hashIdx := (pos[1]/ts)*d.cols + pos[0]/ts
+	if hashIdx < 0 || hashIdx >= len(d.tileHash) {
+		return 0, 0, false
+	}
+	if d.tileHash[hashIdx] != sum {
 		return 0, 0, false
 	}
 	return pos[0], pos[1], true

client/vnc/server/cursor_windows.go

@@ -227,6 +227,9 @@ func dibCopy(hbm windows.Handle, w, h int32) ([]byte, error) {
 	bih.BiBitCount = 32
 	bih.BiCompression = biRgb
 
+	if w <= 0 || h <= 0 || w > maxCursorDim || h > maxCursorDim {
+		return nil, fmt.Errorf("dibCopy: cursor dims %dx%d out of range (max %d)", w, h, maxCursorDim)
+	}
 	buf := make([]byte, int(w)*int(h)*4)
 	r, _, err := procGetDIBits.Call(
 		hdcMem,

client/vnc/server/handshake.go

@@ -33,6 +33,29 @@ const (
 // it requires bumping vncIdentityMagic so old clients fail closed.
 var vncNoiseSuite = noise.NewCipherSuite(noise.DH25519, noise.CipherChaChaPoly, noise.HashSHA256)
 
+// vncNoisePrologueMagic prefixes the Noise prologue. Both sides mix the
+// magic + mode byte + length-prefixed username into the handshake hash
+// before any message is sent. Catches a client that lies about its
+// mode/username in the cleartext header prefix: the cleartext header
+// would say one thing and the Noise hash would expect another, so the
+// responder's AEAD MAC over the handshake state fails to verify and the
+// handshake collapses. Bumping the magic forces old clients to fail
+// closed because their prologue stops matching ours.
+var vncNoisePrologueMagic = []byte("NetBird/VNC/Noise/v1\x00")
+
+// BuildVNCNoisePrologue returns the deterministic byte sequence both
+// sides feed to noise.Config.Prologue for a VNC handshake. Exported so
+// the WASM proxy client computes the exact same bytes; any divergence
+// makes the handshake fail.
+func BuildVNCNoisePrologue(mode byte, username string) []byte {
+	out := make([]byte, 0, len(vncNoisePrologueMagic)+1+2+len(username))
+	out = append(out, vncNoisePrologueMagic...)
+	out = append(out, mode)
+	out = append(out, byte(len(username)>>8), byte(len(username)))
+	out = append(out, []byte(username)...)
+	return out
+}
+
 func (s *Server) authenticateSession(header *connectionHeader) (string, error) {
 	if !header.identityVerified {
 		return "", fmt.Errorf("identity proof missing")
@@ -97,7 +120,7 @@ func (s *Server) readConnectionHeader(conn net.Conn) (*connectionHeader, error)
 	}
 
 	br := bufio.NewReader(conn)
-	clientStatic, identityVerified, err := s.maybeRunNoiseHandshake(conn, br)
+	clientStatic, identityVerified, err := s.maybeRunNoiseHandshake(conn, br, mode, username)
 	if err != nil {
 		return nil, err
 	}
@@ -129,8 +152,10 @@ func (s *Server) readConnectionHeader(conn net.Conn) (*connectionHeader, error)
 // maybeRunNoiseHandshake performs the responder side of a Noise_IK
 // handshake when the client sends the v3 magic. Returns the client static
 // public key learned from the handshake. Any handshake failure is fatal
-// (fail closed).
-func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader) ([]byte, bool, error) {
+// (fail closed). headerMode and headerUsername are mixed into the Noise
+// prologue so the client cannot lie in the cleartext header prefix
+// without making its own AEAD MAC verify-fail on the responder side.
+func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader, headerMode byte, headerUsername string) ([]byte, bool, error) {
 	peek, _ := br.Peek(len(vncIdentityMagic))
 	if !bytes.Equal(peek, vncIdentityMagic) {
 		return nil, false, nil
@@ -145,9 +170,16 @@ func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader) ([]byte
 	}
 
 	// Agents on loopback authenticate via the agent token, not this
-	// handshake. Consume the replayed bytes and skip the response.
+	// handshake: the daemon already ran Noise on the public side and
+	// is now proxying the replayed bytes through to us. Consume the
+	// bytes and report identityVerified=false: the agent's own
+	// authorizeSession short-circuits on disableAuth and never reaches
+	// authenticateSession, so this return value has no effect on the
+	// agent's accept path, but a future caller that forgets the
+	// short-circuit will see the truthful "no Noise identity proved
+	// here" rather than a stale true.
 	if s.disableAuth {
-		return nil, true, nil
+		return nil, false, nil
 	}
 
 	if len(s.identityKey) != 32 || len(s.identityPublic) != 32 {
@@ -157,6 +189,7 @@ func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader) ([]byte
 		CipherSuite:   vncNoiseSuite,
 		Pattern:       noise.HandshakeIK,
 		Initiator:     false,
+		Prologue:      BuildVNCNoisePrologue(headerMode, headerUsername),
 		StaticKeypair: noise.DHKey{Private: s.identityKey, Public: s.identityPublic},
 	})
 	if err != nil {

client/vnc/server/noise_auth_test.go

@@ -65,11 +65,22 @@ func registerSessionKey(t *testing.T, srv *Server, userID string) noise.DHKey {
 	return kp
 }
 
-// writeHeaderPrefix writes the mode + zero-length-username prefix that
+// writeHeaderPrefix writes the mode + (optional) username prefix that
 // precedes the optional Noise handshake in the NetBird VNC header.
 func writeHeaderPrefix(t *testing.T, conn net.Conn, mode byte) {
 	t.Helper()
-	prefix := []byte{mode, 0, 0}
+	writeHeaderPrefixWithUser(t, conn, mode, "")
+}
+
+// writeHeaderPrefixWithUser is the username-aware variant used by tests
+// that need to verify the Noise prologue binds to the cleartext header.
+func writeHeaderPrefixWithUser(t *testing.T, conn net.Conn, mode byte, username string) {
+	t.Helper()
+	if len(username) > 0xFFFF {
+		t.Fatalf("test username too long: %d", len(username))
+	}
+	prefix := []byte{mode, byte(len(username) >> 8), byte(len(username))}
+	prefix = append(prefix, []byte(username)...)
 	_, err := conn.Write(prefix)
 	require.NoError(t, err)
 }
@@ -85,14 +96,22 @@ func writeHeaderTail(t *testing.T, conn net.Conn) {
 
 // performInitiator drives the initiator side of Noise_IK against the
 // server's identity public key, returns the resulting state. The Noise
-// msg2 produced by the server is read and consumed.
+// msg2 produced by the server is read and consumed. headerMode and
+// headerUsername are mixed into the prologue and MUST match what the
+// caller already wrote in the cleartext header prefix.
 func performInitiator(t *testing.T, conn net.Conn, clientKey noise.DHKey, serverPub []byte) {
 	t.Helper()
+	performInitiatorWithHeader(t, conn, clientKey, serverPub, ModeAttach, "")
+}
+
+func performInitiatorWithHeader(t *testing.T, conn net.Conn, clientKey noise.DHKey, serverPub []byte, headerMode byte, headerUsername string) {
+	t.Helper()
 
 	state, err := noise.NewHandshakeState(noise.Config{
 		CipherSuite:   vncNoiseSuite,
 		Pattern:       noise.HandshakeIK,
 		Initiator:     true,
+		Prologue:      BuildVNCNoisePrologue(headerMode, headerUsername),
 		StaticKeypair: clientKey,
 		PeerStatic:    serverPub,
 	})
@@ -414,18 +433,15 @@ func TestNoise_SessionMode_OSUserCheckRunsAfterHandshake(t *testing.T) {
 		},
 	})
 
-	// Request session for "bob" — Noise succeeds, OS-user check denies.
+	// Request session for "bob": Noise succeeds, OS-user check denies.
 	conn, err := net.Dial("tcp", addr.String())
 	require.NoError(t, err)
 	defer conn.Close()
 
-	bob := []byte("bob")
-	prefix := []byte{ModeSession, 0, byte(len(bob))}
-	prefix = append(prefix, bob...)
-	_, err = conn.Write(prefix)
-	require.NoError(t, err)
+	bob := "bob"
+	writeHeaderPrefixWithUser(t, conn, ModeSession, bob)
 
-	performInitiator(t, conn, clientKey, serverPub)
+	performInitiatorWithHeader(t, conn, clientKey, serverPub, ModeSession, bob)
 	writeHeaderTail(t, conn)
 
 	reason := readRFBFailure(t, conn)

client/vnc/server/rfb.go

@@ -488,14 +488,27 @@ func tileChanged(prev, cur *image.RGBA, x, y, w, h int) bool {
 // tileIsUniform reports whether every pixel in the given rectangle of img is
 // the same RGBA value, and returns that pixel packed as 0xRRGGBBAA when so.
 // Uses uint32 comparisons across rows; returns early on the first mismatch.
+// Returns (0, false) on any out-of-range rectangle so an unsafe pointer
+// deref can never reach past img.Pix even if a capturer reports stale
+// dimensions or a resize race produces inconsistent state.
 func tileIsUniform(img *image.RGBA, x, y, w, h int) (uint32, bool) {
-	if w <= 0 || h <= 0 {
+	if w <= 0 || h <= 0 || x < 0 || y < 0 {
+		return 0, false
+	}
+	bounds := img.Rect
+	if x+w > bounds.Dx() || y+h > bounds.Dy() {
 		return 0, false
 	}
 	stride := img.Stride
 	base := y*stride + x*4
-	first := *(*uint32)(unsafe.Pointer(&img.Pix[base]))
 	rowBytes := w * 4
+	// Final row's last pixel must be inside Pix; guard against any caller
+	// that managed to slip past the bounds check above (e.g. negative
+	// stride from a forged image).
+	if base < 0 || base+(h-1)*stride+rowBytes > len(img.Pix) {
+		return 0, false
+	}
+	first := *(*uint32)(unsafe.Pointer(&img.Pix[base]))
 	for row := 0; row < h; row++ {
 		p := base + row*stride
 		for col := 0; col < rowBytes; col += 4 {
@@ -621,10 +634,18 @@ func writeTightRectHeader(buf []byte, x, y, w, h int) {
 // appendTightLength encodes a Tight compact length prefix (1, 2, or 3 bytes
 // LE-ish, top bit of each byte signals continuation). Lengths exceeding
 // tightMaxLength would silently truncate the high byte; callers must clamp
-// or fall back before reaching here.
+// or fall back before reaching here. Out-of-range values are clamped and
+// logged instead of panicking so a malformed encode can't tear the whole
+// server down: callers already check the cap, so this branch is just a
+// defence-in-depth backstop.
 func appendTightLength(buf []byte, n int) []byte {
-	if n < 0 || n > tightMaxLength {
-		panic(fmt.Sprintf("tight length out of range: %d", n))
+	if n < 0 {
+		log.Warnf("tight length negative (%d); clamping to 0", n)
+		n = 0
+	}
+	if n > tightMaxLength {
+		log.Warnf("tight length %d exceeds cap %d; clamping (caller should have fallen back)", n, tightMaxLength)
+		n = tightMaxLength
 	}
 	b0 := byte(n & 0x7f)
 	if n <= 0x7f {
@@ -765,7 +786,19 @@ func tightQualityFor(pixels int) int {
 // per-rect Tight encoding stays alloc-free. Cheap O(maxColors) per call.
 func sampledColorCountInto(seen map[uint32]struct{}, img *image.RGBA, x, y, w, h, maxColors int) int {
 	clear(seen)
+	if w <= 0 || h <= 0 || x < 0 || y < 0 {
+		return 0
+	}
+	bounds := img.Rect
+	if x+w > bounds.Dx() || y+h > bounds.Dy() {
+		return 0
+	}
 	stride := img.Stride
+	// Defensive: refuse to dereference past the buffer end if stride math
+	// somehow disagrees with bounds (e.g. caller passed a SubImage).
+	if (y+h-1)*stride+(x+w)*4 > len(img.Pix) {
+		return 0
+	}
 	step := max((w*h)/(maxColors*4), 1)
 	var idx int
 	for row := 0; row < h; row++ {

client/vnc/server/security_hardening_test.go

@@ -0,0 +1,374 @@
+//go:build !js && !ios && !android
+
+package server
+
+import (
+	"image"
+	"net"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/flynn/noise"
+	"github.com/stretchr/testify/require"
+)
+
+// TestTileIsUniformRejectsOutOfRange covers the bounds-check guard added to
+// tileIsUniform. Each case below would, before the guard, have produced an
+// unsafe.Pointer dereference past the end of img.Pix; the function must now
+// return (0,false) and not panic.
+func TestTileIsUniformRejectsOutOfRange(t *testing.T) {
+	img := makeUniformImage(64, 64, 0x11, 0x22, 0x33)
+	cases := []struct {
+		name       string
+		x, y, w, h int
+	}{
+		{"negative_x", -1, 0, 8, 8},
+		{"negative_y", 0, -1, 8, 8},
+		{"x_past_right_edge", 60, 0, 8, 8},
+		{"y_past_bottom_edge", 0, 60, 8, 8},
+		{"w_overflows_into_oob", 0, 0, 65, 8},
+		{"h_overflows_into_oob", 0, 0, 8, 65},
+		{"zero_width", 0, 0, 0, 8},
+		{"zero_height", 0, 0, 8, 0},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer func() {
+				if r := recover(); r != nil {
+					t.Fatalf("tileIsUniform panicked: %v", r)
+				}
+			}()
+			pixel, uniform := tileIsUniform(img, tc.x, tc.y, tc.w, tc.h)
+			if uniform {
+				t.Fatalf("expected uniform=false on out-of-range, got pixel=%#x", pixel)
+			}
+		})
+	}
+}
+
+func TestTileIsUniformInRangeStillWorks(t *testing.T) {
+	img := makeUniformImage(64, 64, 0x12, 0x34, 0x56)
+	pixel, uniform := tileIsUniform(img, 8, 8, 16, 16)
+	if !uniform {
+		t.Fatal("expected uniform=true for uniformly-painted rect")
+	}
+	// Pixel is BGRA-shifted internally; just confirm it is non-zero so we
+	// know the deref ran.
+	if pixel == 0 {
+		t.Fatal("expected non-zero packed pixel")
+	}
+}
+
+// TestSampledColorCountIntoRejectsOutOfRange mirrors the bounds-check guard
+// added to sampledColorCountInto: any out-of-range rect must yield 0 with
+// no panic and no map mutation that would propagate stale colors.
+func TestSampledColorCountIntoRejectsOutOfRange(t *testing.T) {
+	img := makeUniformImage(64, 64, 0x11, 0x22, 0x33)
+	seen := make(map[uint32]struct{}, 16)
+	cases := []struct {
+		name       string
+		x, y, w, h int
+	}{
+		{"negative_x", -1, 0, 8, 8},
+		{"negative_y", 0, -1, 8, 8},
+		{"x_past_right_edge", 60, 0, 8, 8},
+		{"y_past_bottom_edge", 0, 60, 8, 8},
+		{"w_overflows_into_oob", 0, 0, 65, 8},
+		{"h_overflows_into_oob", 0, 0, 8, 65},
+		{"zero_dims", 0, 0, 0, 0},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			defer func() {
+				if r := recover(); r != nil {
+					t.Fatalf("sampledColorCountInto panicked: %v", r)
+				}
+			}()
+			n := sampledColorCountInto(seen, img, tc.x, tc.y, tc.w, tc.h, 256)
+			if n != 0 {
+				t.Fatalf("expected 0 colors on out-of-range rect, got %d", n)
+			}
+		})
+	}
+}
+
+// TestAppendTightLengthClampsInsteadOfPanicking ensures the function no
+// longer panics on out-of-range input: a panic would tear down the entire
+// VNC server when the encoder hits an unexpected length.
+func TestAppendTightLengthClampsInsteadOfPanicking(t *testing.T) {
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("appendTightLength panicked: %v", r)
+		}
+	}()
+	_ = appendTightLength(nil, -1)
+	_ = appendTightLength(nil, tightMaxLength+1)
+	_ = appendTightLength(nil, 1<<30)
+}
+
+// TestEncodeCursorPseudoRectCapsDimensions ensures the cursor encoder
+// refuses unreasonably large sprites: a bad platform-API response with
+// w*h*4 that overflows int would otherwise produce an undersized buf and
+// a heap overflow on the subsequent copy.
+func TestEncodeCursorPseudoRectCapsDimensions(t *testing.T) {
+	t.Run("oversized_rejected", func(t *testing.T) {
+		img := image.NewRGBA(image.Rect(0, 0, maxCursorDim+1, 1))
+		if buf := encodeCursorPseudoRect(img, 0, 0); buf != nil {
+			t.Fatalf("expected nil for oversized cursor, got %d bytes", len(buf))
+		}
+	})
+	t.Run("nil_rejected", func(t *testing.T) {
+		if buf := encodeCursorPseudoRect(nil, 0, 0); buf != nil {
+			t.Fatal("expected nil for nil image")
+		}
+	})
+	t.Run("zero_dims_rejected", func(t *testing.T) {
+		img := image.NewRGBA(image.Rect(0, 0, 0, 0))
+		if buf := encodeCursorPseudoRect(img, 0, 0); buf != nil {
+			t.Fatal("expected nil for zero-dim image")
+		}
+	})
+	t.Run("small_cursor_still_encodes", func(t *testing.T) {
+		img := image.NewRGBA(image.Rect(0, 0, 16, 16))
+		// Paint a quasi-opaque sprite so the mask path runs.
+		for i := range img.Pix {
+			img.Pix[i] = 0x80
+		}
+		buf := encodeCursorPseudoRect(img, 1, 2)
+		if buf == nil {
+			t.Fatal("expected encoded cursor, got nil")
+		}
+		// 12-byte rect header + w*h*4 pixels + ((w+7)/8)*h mask bytes.
+		want := 12 + 16*16*4 + ((16+7)/8)*16
+		if len(buf) != want {
+			t.Fatalf("cursor rect length: got %d want %d", len(buf), want)
+		}
+	})
+}
+
+// TestEncodeCursorPseudoRectAtMaxDim sanity-checks the boundary: the cap
+// must allow exactly maxCursorDim×maxCursorDim through.
+func TestEncodeCursorPseudoRectAtMaxDim(t *testing.T) {
+	img := image.NewRGBA(image.Rect(0, 0, maxCursorDim, maxCursorDim))
+	if buf := encodeCursorPseudoRect(img, 0, 0); buf == nil {
+		t.Fatal("expected non-nil for max-dim cursor (boundary)")
+	}
+}
+
+// TestCopyRectFindTileRejectsOutOfRangeSrc covers the additional source-
+// position guard added to findTileMatch. A scenario where the source
+// position recorded in prevTiles is now outside the (possibly shrunken)
+// current framebuffer must produce no match: otherwise the encoder would
+// emit a CopyRect telling the client to copy from undefined pixels.
+func TestCopyRectFindTileRejectsOutOfRangeSrc(t *testing.T) {
+	const ts = 64
+	const w, h = 128, 128
+	cur := image.NewRGBA(image.Rect(0, 0, w, h))
+	fillTile(cur, 0, 0, ts, 0x11, 0x22, 0x33)
+
+	d := newCopyRectDetector(ts)
+	// Pre-populate prevTiles with a stale source position that falls
+	// outside the current framebuffer; this mirrors what would happen
+	// after a resize. We compute the same hash the detector would use
+	// for the tile at (0,0) of cur and bind that hash to an out-of-range
+	// source.
+	hash := d.hashTile(cur, 0, 0)
+	d.cols = w / ts
+	d.tileHash = make([]uint64, (w/ts)*(h/ts))
+	d.prevTiles = map[uint64][2]int{
+		hash: {w + ts, h + ts}, // out of range
+	}
+
+	sx, sy, ok := d.findTileMatch(cur, 0, 0)
+	if ok {
+		t.Fatalf("expected no match for out-of-range source, got (%d,%d)", sx, sy)
+	}
+}
+
+// TestBuildVNCNoisePrologueDeterministic locks in the format both sides
+// MUST agree on. Drift here breaks every VNC handshake silently (with
+// just an "authentication failed" error), so any future refactor that
+// changes this output needs to bump the prologue magic and ship a
+// migration.
+func TestBuildVNCNoisePrologueDeterministic(t *testing.T) {
+	a := BuildVNCNoisePrologue(ModeAttach, "")
+	b := BuildVNCNoisePrologue(ModeAttach, "")
+	if string(a) != string(b) {
+		t.Fatalf("non-deterministic prologue: %x vs %x", a, b)
+	}
+
+	// Different mode must produce a distinct prologue.
+	if string(a) == string(BuildVNCNoisePrologue(ModeSession, "")) {
+		t.Fatal("mode change must change prologue")
+	}
+	// Different username must produce a distinct prologue.
+	if string(a) == string(BuildVNCNoisePrologue(ModeAttach, "alice")) {
+		t.Fatal("username change must change prologue")
+	}
+	// Magic prefix must be present so a missing/short prologue (e.g.
+	// an old client that wasn't recompiled) fails closed.
+	if !strings.HasPrefix(string(a), "NetBird/VNC/Noise/v1") {
+		t.Fatalf("prologue missing magic prefix: %q", a)
+	}
+}
+
+// TestNoise_ClientLiesAboutMode_HandshakeFails proves the prologue
+// binding catches a client that writes one mode in the cleartext header
+// prefix and then tries to mint a Noise handshake claiming a different
+// mode. Without binding, an attacker could declare mode=attach (loose
+// OS-user check) while the Noise hash committed to mode=session,
+// pretending to be a session user when the server's policy gate ran on
+// the attach path.
+func TestNoise_ClientLiesAboutMode_HandshakeFails(t *testing.T) {
+	addr, srv, serverPub := noiseTestServer(t)
+	clientKey := registerSessionKey(t, srv, "alice@example")
+
+	conn, err := net.Dial("tcp", addr.String())
+	require.NoError(t, err)
+	defer conn.Close()
+
+	// Cleartext header says session, but Noise prologue commits to
+	// attach. Server reads the cleartext, computes a prologue with
+	// session, and the AEAD MAC over the handshake state fails.
+	writeHeaderPrefixWithUser(t, conn, ModeSession, "alice")
+
+	state, err := noise.NewHandshakeState(noise.Config{
+		CipherSuite:   vncNoiseSuite,
+		Pattern:       noise.HandshakeIK,
+		Initiator:     true,
+		Prologue:      BuildVNCNoisePrologue(ModeAttach, "alice"),
+		StaticKeypair: clientKey,
+		PeerStatic:    serverPub,
+	})
+	require.NoError(t, err)
+	msg1, _, _, err := state.WriteMessage(nil, nil)
+	require.NoError(t, err)
+	_, err = conn.Write(append([]byte("NBV3"), msg1...))
+	require.NoError(t, err)
+
+	// Server must reject the connection: either by failing the read
+	// of msg2 (the connection is closed) or by sending an RFB failure.
+	require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second)))
+	msg2 := make([]byte, noiseResponderMsgLen)
+	if _, err := readFullOrEOF(conn, msg2); err == nil {
+		// If the server did write something, it must not be a valid
+		// Noise msg2: ReadMessage must fail.
+		_, _, _, derr := state.ReadMessage(nil, msg2)
+		if derr == nil {
+			t.Fatal("expected Noise read to fail when client lies about mode")
+		}
+	}
+}
+
+// TestNoise_ClientLiesAboutUsername_HandshakeFails mirrors the mode
+// check above for the username field, which is the other piece of
+// cleartext header the prologue binds to.
+func TestNoise_ClientLiesAboutUsername_HandshakeFails(t *testing.T) {
+	addr, srv, serverPub := noiseTestServer(t)
+	clientKey := registerSessionKey(t, srv, "alice@example")
+
+	conn, err := net.Dial("tcp", addr.String())
+	require.NoError(t, err)
+	defer conn.Close()
+
+	writeHeaderPrefixWithUser(t, conn, ModeSession, "alice")
+
+	state, err := noise.NewHandshakeState(noise.Config{
+		CipherSuite:   vncNoiseSuite,
+		Pattern:       noise.HandshakeIK,
+		Initiator:     true,
+		Prologue:      BuildVNCNoisePrologue(ModeSession, "bob"), // lies
+		StaticKeypair: clientKey,
+		PeerStatic:    serverPub,
+	})
+	require.NoError(t, err)
+	msg1, _, _, err := state.WriteMessage(nil, nil)
+	require.NoError(t, err)
+	_, err = conn.Write(append([]byte("NBV3"), msg1...))
+	require.NoError(t, err)
+
+	require.NoError(t, conn.SetReadDeadline(time.Now().Add(2*time.Second)))
+	msg2 := make([]byte, noiseResponderMsgLen)
+	if _, err := readFullOrEOF(conn, msg2); err == nil {
+		_, _, _, derr := state.ReadMessage(nil, msg2)
+		if derr == nil {
+			t.Fatal("expected Noise read to fail when client lies about username")
+		}
+	}
+}
+
+// readFullOrEOF returns nil if buf was fully populated, or an error if
+// the connection closed first. Used by the binding tests to tolerate
+// the server's two valid failure modes (close vs RFB failure).
+func readFullOrEOF(conn net.Conn, buf []byte) (int, error) {
+	n, err := conn.Read(buf)
+	for n < len(buf) && err == nil {
+		var k int
+		k, err = conn.Read(buf[n:])
+		n += k
+	}
+	return n, err
+}
+
+// TestRegisterConnAuth_RaceWithRevocation covers the TOCTOU race the
+// fix in registerConnAuth closes. Without the re-check, a concurrent
+// UpdateVNCAuth that removes the client's pubkey AFTER authorizeSession
+// runs but BEFORE registerConnAuth inserts into connAuth would leave an
+// unauthorized session running until the next policy push.
+func TestRegisterConnAuth_RaceWithRevocation(t *testing.T) {
+	_, srv, _ := noiseTestServer(t)
+	clientKey := registerSessionKey(t, srv, "alice@example")
+
+	header := &connectionHeader{
+		identityVerified: true,
+		clientStatic:     clientKey.Public,
+		mode:             ModeAttach,
+	}
+
+	// Authoritative simulation of the race: first registerConnAuth
+	// succeeds (caller is in policy), then policy is updated to remove
+	// the caller's pubkey, then a fresh registration attempt must be
+	// refused even though the original authorizeSession path already
+	// said ok=true.
+	conn1, conn2 := net.Pipe()
+	defer conn1.Close()
+	defer conn2.Close()
+	require.NoError(t, srv.registerConnAuth(conn1, header))
+
+	// Revoke: empty pubkey list, nobody is authorized anymore.
+	srv.UpdateVNCAuth(nil)
+
+	conn3, conn4 := net.Pipe()
+	defer conn3.Close()
+	defer conn4.Close()
+	err := srv.registerConnAuth(conn3, header)
+	if err == nil {
+		t.Fatal("expected registerConnAuth to refuse after revocation, got nil")
+	}
+	if !strings.Contains(err.Error(), "authorization revoked") {
+		t.Fatalf("unexpected error from post-revocation register: %v", err)
+	}
+}
+
+// TestEncoderPanicRecovery ensures processFBRequestSafe catches a panic
+// from the encode path and surfaces it as an error rather than tearing
+// down every session.
+func TestEncoderPanicRecovery(t *testing.T) {
+	// A session whose encMu is nil-safe-enough that processFBRequest can
+	// be called and induce a deterministic panic at one of its earliest
+	// dereferences. We only need the recover wrapper to engage.
+	s := &session{}
+	defer func() {
+		if r := recover(); r != nil {
+			t.Fatalf("processFBRequestSafe leaked a panic: %v", r)
+		}
+	}()
+	err := s.processFBRequestSafe(fbRequest{})
+	if err == nil {
+		t.Fatal("expected an error from the recovered panic, got nil")
+	}
+	if !strings.Contains(err.Error(), "encoder panic") {
+		t.Fatalf("error should mention encoder panic, got: %v", err)
+	}
+}

client/vnc/server/server.go

@@ -478,17 +478,30 @@ func sourceIPString(addr net.Addr) string {
 // registerConnAuth records the verified Noise_IK identity for a live
 // connection so UpdateVNCAuth can later revoke it if policy changes.
 // No-op when auth is disabled (e.g. agent-mode loopback connections).
-func (s *Server) registerConnAuth(c net.Conn, header *connectionHeader) {
+//
+// The original authorization check in authorizeSession and this
+// registration are not atomic, so a concurrent UpdateVNCAuth can revoke
+// the client's pubkey in between (revokeUnauthorizedSessions iterates
+// connAuth and would miss this connection because it isn't registered
+// yet). To close that window we re-run authenticateSession here under
+// the same sessionsMu that revokeUnauthorizedSessions holds; if the
+// caller's pubkey is no longer authorized at registration time, we
+// refuse the registration and the caller tears the connection down.
+func (s *Server) registerConnAuth(c net.Conn, header *connectionHeader) error {
 	if s.disableAuth || header == nil || len(header.clientStatic) != 32 {
-		return
+		return nil
 	}
 	s.sessionsMu.Lock()
+	defer s.sessionsMu.Unlock()
+	if _, err := s.authenticateSession(header); err != nil {
+		return fmt.Errorf("authorization revoked before session registration: %w", err)
+	}
 	s.connAuth[c] = connAuthInfo{
 		clientStatic: append([]byte(nil), header.clientStatic...),
 		mode:         header.mode,
 		username:     header.username,
 	}
-	s.sessionsMu.Unlock()
+	return nil
 }
 
 // tryAcquireConnSlot returns true when a connection slot was successfully
@@ -798,7 +811,11 @@ func (s *Server) handleConnection(conn net.Conn) {
 		connLog.Info("VNC connection rejected: auth failed")
 		return
 	}
-	s.registerConnAuth(conn, header)
+	if err := s.registerConnAuth(conn, header); err != nil {
+		rejectConnection(conn, codeMessage(RejectCodeAuthForbidden, err.Error()))
+		connLog.Warnf("VNC connection rejected: %v", err)
+		return
+	}
 
 	decision, err := s.gateApproval(conn, header)
 	if err != nil {
@@ -818,6 +835,23 @@ func (s *Server) handleConnection(conn net.Conn) {
 	}
 	defer sessionCleanup()
 
+	if err := s.validateCapturer(capturer); err != nil {
+		rejectConnection(conn, codeMessage(RejectCodeCapturerError, fmt.Sprintf("screen capturer: %v", err)))
+		connLog.Warnf("VNC connection rejected: capturer not ready: %v", err)
+		return
+	}
+
+	// Validate framebuffer dimensions BEFORE registering the active
+	// session: keeps a misbehaving capturer from briefly showing up in
+	// ActiveSessions output and ensures the rest of the pipeline only
+	// ever runs against an in-range frame.
+	w, h := capturer.Width(), capturer.Height()
+	if w <= 0 || h <= 0 || w > maxFramebufferDim || h > maxFramebufferDim {
+		rejectConnection(conn, codeMessage(RejectCodeCapturerError, fmt.Sprintf("framebuffer dimensions out of range: %dx%d", w, h)))
+		connLog.Warnf("VNC connection rejected: framebuffer %dx%d outside [1, %d]", w, h, maxFramebufferDim)
+		return
+	}
+
 	sessionID := s.addSession(ActiveSessionInfo{
 		RemoteAddress: conn.RemoteAddr().String(),
 		Mode:          modeString(header.mode),
@@ -826,19 +860,6 @@ func (s *Server) handleConnection(conn net.Conn) {
 	}, conn)
 	defer s.removeSession(sessionID)
 
-	if err := s.validateCapturer(capturer); err != nil {
-		rejectConnection(conn, codeMessage(RejectCodeCapturerError, fmt.Sprintf("screen capturer: %v", err)))
-		connLog.Warnf("VNC connection rejected: capturer not ready: %v", err)
-		return
-	}
-
-	w, h := capturer.Width(), capturer.Height()
-	if w <= 0 || h <= 0 || w > maxFramebufferDim || h > maxFramebufferDim {
-		rejectConnection(conn, codeMessage(RejectCodeCapturerError, fmt.Sprintf("framebuffer dimensions out of range: %dx%d", w, h)))
-		connLog.Warnf("VNC connection rejected: framebuffer %dx%d outside [1, %d]", w, h, maxFramebufferDim)
-		return
-	}
-
 	conn = newMetricsConn(conn, s.sessionRecorder)
 	sess := &session{
 		conn:     conn,

client/vnc/server/session_cursor.go

@@ -37,21 +37,59 @@ func (s *session) pendingCursorRect() []byte {
 		return nil
 	}
 	buf := encodeCursorPseudoRect(img, hotX, hotY)
+	if buf == nil {
+		return nil
+	}
+	// Re-check the serial under the write lock so a concurrent update
+	// from another goroutine can't be silently overwritten with a stale
+	// value: if someone advanced it past `serial` while we were encoding,
+	// keep their value and drop this rect.
 	s.encMu.Lock()
+	if serial == s.lastCursorSerial {
+		s.encMu.Unlock()
+		return nil
+	}
+	if uint64(serial-s.lastCursorSerial) > 1<<63 {
+		// `serial` is older than the current value (wraparound-aware
+		// comparison). Drop it.
+		s.encMu.Unlock()
+		return nil
+	}
 	s.lastCursorSerial = serial
 	s.encMu.Unlock()
 	return buf
 }
 
+// maxCursorDim caps the cursor sprite size we'll encode. Real platform
+// cursors are tiny (<=256×256 on every supported OS); a value past this
+// almost certainly indicates a corrupted platform-API response, and
+// blindly multiplying it into a buffer size would overflow int and produce
+// an undersized allocation that the encode loop would then walk past.
+const maxCursorDim = 256
+
 // encodeCursorPseudoRect packs the cursor sprite into a Cursor pseudo
 // rectangle (RFB 7.7.4, pseudo-encoding -239). Layout: 12-byte rect header
 // followed by w*h*4 BGRX pixel bytes and a 1-bit mask of (w+7)/8 bytes per
-// row, MSB-first, with each row independently padded.
+// row, MSB-first, with each row independently padded. Returns nil when
+// the source image's dimensions are non-positive or exceed maxCursorDim;
+// callers treat nil as "skip the cursor rect this frame."
 func encodeCursorPseudoRect(img *image.RGBA, hotX, hotY int) []byte {
+	if img == nil {
+		return nil
+	}
 	w, h := img.Rect.Dx(), img.Rect.Dy()
+	if w <= 0 || h <= 0 || w > maxCursorDim || h > maxCursorDim {
+		return nil
+	}
 	pixelBytes := w * h * 4
 	maskStride := (w + 7) / 8
 	maskBytes := maskStride * h
+	// Defensive: ensure the source image is actually big enough for the
+	// access pattern below. A SubImage that misreports its dx/dy would
+	// otherwise be read past the end.
+	if (h-1)*img.Stride+w*4 > len(img.Pix) {
+		return nil
+	}
 	buf := make([]byte, 12+pixelBytes+maskBytes)
 
 	binary.BigEndian.PutUint16(buf[0:2], uint16(hotX))

client/vnc/server/session_encode.go

@@ -13,10 +13,15 @@ import (
 // encoderLoop owns the capture → diff → encode → write pipeline. Running it
 // off the read loop prevents a slow encode (zlib full-frame, many dirty
 // tiles) from blocking inbound input events.
+//
+// Per-request panics are caught and turned into session teardown so a bug
+// in one encoder path (a malformed capture frame, a zlib corner case) can
+// only kill its own session, never the whole VNC server.
 func (s *session) encoderLoop(done chan<- struct{}) {
 	defer close(done)
 	for req := range s.encodeCh {
-		if err := s.processFBRequest(req); err != nil {
+		err := s.processFBRequestSafe(req)
+		if err != nil {
 			s.log.Debugf("encode: %v", err)
 			// On write/capture error, close the connection so messageLoop
 			// exits and the session terminates cleanly.
@@ -27,6 +32,25 @@ func (s *session) encoderLoop(done chan<- struct{}) {
 	}
 }
 
+// processFBRequestSafe wraps processFBRequest with a panic recover so a
+// crash in encode/diff/compress paths surfaces as a session-only error
+// instead of bringing down every peer's VNC sessions. The recover handler
+// avoids any further dereference of session state (the panic itself may
+// indicate a half-initialised session) so it can never re-panic.
+func (s *session) processFBRequestSafe(req fbRequest) (err error) {
+	defer func() {
+		r := recover()
+		if r == nil {
+			return
+		}
+		err = fmt.Errorf("encoder panic: %v", r)
+		if s != nil && s.log != nil {
+			s.log.Errorf("encoder panic recovered: %v", r)
+		}
+	}()
+	return s.processFBRequest(req)
+}
+
 func (s *session) processFBRequest(req fbRequest) error {
 	// Watch for resolution changes between cycles. When the capturer
 	// reports a new size, tell the client via DesktopSize so it can

client/wasm/internal/vnc/proxy.go

@@ -32,6 +32,26 @@ const (
 
 var vncNoiseSuite = noise.NewCipherSuite(noise.DH25519, noise.CipherChaChaPoly, noise.HashSHA256)
 
+// vncNoisePrologueMagic must stay byte-identical to the server side
+// (see client/vnc/server/handshake.go). Any drift here breaks every VNC
+// handshake.
+var vncNoisePrologueMagic = []byte("NetBird/VNC/Noise/v1\x00")
+
+// buildVNCNoisePrologue mirrors server.BuildVNCNoisePrologue. Both sides
+// hash (magic || mode || u16len(username) || username) into the
+// handshake hash; a client that lies about its mode/username in the
+// cleartext header prefix produces a divergent prologue, the responder
+// computes the truthful prologue from what it just read, and the AEAD
+// MAC over the handshake state fails to verify.
+func buildVNCNoisePrologue(mode byte, username string) []byte {
+	out := make([]byte, 0, len(vncNoisePrologueMagic)+1+2+len(username))
+	out = append(out, vncNoisePrologueMagic...)
+	out = append(out, mode)
+	out = append(out, byte(len(username)>>8), byte(len(username)))
+	out = append(out, []byte(username)...)
+	return out
+}
+
 // sessionKeyStore retains per-session X25519 keypairs so the JS layer
 // only sees an opaque session id + the public key; the private key never
 // leaves wasm.
@@ -437,6 +457,7 @@ func (p *VNCProxy) runNoiseHandshake(conn net.Conn, dest vncDestination) error {
 		CipherSuite:   vncNoiseSuite,
 		Pattern:       noise.HandshakeIK,
 		Initiator:     true,
+		Prologue:      buildVNCNoisePrologue(dest.mode, dest.username),
 		StaticKeypair: noise.DHKey{Private: dest.sessionPriv, Public: dest.sessionPub},
 		PeerStatic:    dest.peerPubKey,
 	})

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

@@ -465,6 +465,26 @@ func (h *Handler) CreateTemporaryAccess(w http.ResponseWriter, r *http.Request)
 		return
 	}
 
+	// Explicit defence-in-depth gate before any business logic: we already
+	// rely on AddPeer/SavePolicy to enforce the Peers.Create and
+	// Policies.Create permissions, but checking up-front means a future
+	// refactor that bypasses one of those calls can't silently widen the
+	// endpoint's authority.
+	if allowed, err := h.permissionsManager.ValidateUserPermissions(r.Context(), userAuth.AccountId, userAuth.UserId, modules.Peers, operations.Create); err != nil {
+		util.WriteError(r.Context(), status.NewPermissionValidationError(err), w)
+		return
+	} else if !allowed {
+		util.WriteError(r.Context(), status.NewPermissionDeniedError(), w)
+		return
+	}
+	if allowed, err := h.permissionsManager.ValidateUserPermissions(r.Context(), userAuth.AccountId, userAuth.UserId, modules.Policies, operations.Create); err != nil {
+		util.WriteError(r.Context(), status.NewPermissionValidationError(err), w)
+		return
+	} else if !allowed {
+		util.WriteError(r.Context(), status.NewPermissionDeniedError(), w)
+		return
+	}
+
 	var req api.PeerTemporaryAccessRequest
 	err = json.NewDecoder(r.Body).Decode(&req)
 	if err != nil {

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

@@ -0,0 +1,106 @@
+package peers
+
+import (
+	"bytes"
+	"encoding/json"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/gorilla/mux"
+
+	nbcontext "github.com/netbirdio/netbird/management/server/context"
+	"github.com/netbirdio/netbird/management/server/permissions"
+	"github.com/netbirdio/netbird/management/server/permissions/modules"
+	"github.com/netbirdio/netbird/management/server/permissions/operations"
+	"github.com/netbirdio/netbird/shared/auth"
+	"github.com/netbirdio/netbird/shared/management/http/api"
+)
+
+// TestCreateTemporaryAccess_RejectsCallerWithoutPeersCreate verifies the
+// defence-in-depth permission gate added to CreateTemporaryAccess: a user
+// who cannot create peers must be turned away with 403 before any
+// AccountManager call runs. Previously this endpoint relied entirely on
+// SavePolicy/AddPeer's internal permission checks; the explicit gate
+// makes sure a future refactor that bypasses one of those calls can't
+// silently widen the endpoint's authority.
+func TestCreateTemporaryAccess_RejectsCallerWithoutPeersCreate(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	permMgr := permissions.NewMockManager(ctrl)
+
+	// Caller lacks Peers.Create: handler must short-circuit before any
+	// AccountManager interaction. We deliberately leave accountManager
+	// nil so the test fails loudly if the handler tries to call it.
+	permMgr.EXPECT().
+		ValidateUserPermissions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(modules.Peers), gomock.Eq(operations.Create)).
+		Return(false, nil).
+		Times(1)
+
+	h := &Handler{
+		permissionsManager: permMgr,
+	}
+
+	pubKey := "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
+	body, _ := json.Marshal(api.PeerTemporaryAccessRequest{
+		Name:     "temp",
+		Rules:    []string{"netbird-vnc"},
+		WgPubKey: pubKey,
+	})
+
+	req := httptest.NewRequest(http.MethodPost, "/peers/peer-id/temporary-access", bytes.NewReader(body))
+	req = mux.SetURLVars(req, map[string]string{"peerId": "peer-id"})
+	req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{
+		UserId:    "regular_user",
+		Domain:    "example.com",
+		AccountId: "acct1",
+	})
+
+	rec := httptest.NewRecorder()
+	h.CreateTemporaryAccess(rec, req)
+
+	if rec.Code != http.StatusForbidden {
+		t.Fatalf("expected 403 Forbidden, got %d (body=%s)", rec.Code, rec.Body.String())
+	}
+}
+
+// TestCreateTemporaryAccess_RejectsCallerWithoutPoliciesCreate covers
+// the second leg of the gate: a user with Peers.Create but not
+// Policies.Create must still be refused. Catches a misconfiguration
+// where one permission is granted broadly but the other isn't.
+func TestCreateTemporaryAccess_RejectsCallerWithoutPoliciesCreate(t *testing.T) {
+	ctrl := gomock.NewController(t)
+	permMgr := permissions.NewMockManager(ctrl)
+
+	permMgr.EXPECT().
+		ValidateUserPermissions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(modules.Peers), gomock.Eq(operations.Create)).
+		Return(true, nil).
+		Times(1)
+	permMgr.EXPECT().
+		ValidateUserPermissions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(modules.Policies), gomock.Eq(operations.Create)).
+		Return(false, nil).
+		Times(1)
+
+	h := &Handler{
+		permissionsManager: permMgr,
+	}
+
+	body, _ := json.Marshal(api.PeerTemporaryAccessRequest{
+		Name:  "temp",
+		Rules: []string{"netbird-vnc"},
+	})
+	req := httptest.NewRequest(http.MethodPost, "/peers/peer-id/temporary-access", bytes.NewReader(body))
+	req = mux.SetURLVars(req, map[string]string{"peerId": "peer-id"})
+	req = nbcontext.SetUserAuthInRequest(req, auth.UserAuth{
+		UserId:    "regular_user",
+		Domain:    "example.com",
+		AccountId: "acct1",
+	})
+
+	rec := httptest.NewRecorder()
+	h.CreateTemporaryAccess(rec, req)
+
+	if rec.Code != http.StatusForbidden {
+		t.Fatalf("expected 403 Forbidden, got %d (body=%s)", rec.Code, rec.Body.String())
+	}
+}

management/server/types/policy_authorized_users.go

@@ -78,13 +78,17 @@ func applyResolvedRuleToState(
 }
 
 // handleVNCRule collects VNC authorized users and session pubkeys for a VNC
-// policy rule. Bidirectional rules grant access in both directions.
+// policy rule. Bidirectional rules grant access in both directions, so a
+// peer that appears in the rule's sources also needs the SessionPubKey
+// pushed (otherwise the Noise_IK handshake against that peer would fail
+// because its authorizer wouldn't know the client's static key).
 func (cb ruleAuthCallbacks) handleVNCRule(rule *PolicyRule, peerInSources, peerInDestinations bool, state *peerConnResolveState) {
-	if !peerInDestinations && !(rule.Bidirectional && peerInSources) {
+	receivingPeer := peerInDestinations || (rule.Bidirectional && peerInSources)
+	if !receivingPeer {
 		return
 	}
 	cb.collectVNCUsers(rule, state.vncAuthorizedUsers)
-	if peerInDestinations && rule.SessionPubKey != "" && rule.AuthorizedUser != "" {
+	if rule.SessionPubKey != "" && rule.AuthorizedUser != "" {
 		state.vncSessionPubKeys = append(state.vncSessionPubKeys, VNCSessionPubKey{
 			PubKey:      rule.SessionPubKey,
 			UserID:      rule.AuthorizedUser,

management/server/types/policy_authorized_users_security_test.go

@@ -0,0 +1,85 @@
+package types
+
+import "testing"
+
+// TestHandleVNCRule_BidirectionalDistributesPubkeyToSourcePeer covers the
+// latent bug where a bidirectional VNC rule used to drop the
+// SessionPubKey for the peer that appears only in sources, even though
+// the rule explicitly grants access in both directions. Without the
+// pubkey, the source peer's Noise_IK authorizer would not recognise the
+// client's static key and Noise handshakes against it would fail. The
+// fix in handleVNCRule must distribute the pubkey to either side of a
+// bidirectional rule.
+func TestHandleVNCRule_BidirectionalDistributesPubkeyToSourcePeer(t *testing.T) {
+	rule := &PolicyRule{
+		Protocol:           PolicyRuleProtocolNetbirdVNC,
+		Bidirectional:      true,
+		AuthorizedUser:     "user1",
+		SessionPubKey:      "pubkey-base64",
+		SessionDisplayName: "Alice",
+	}
+	cb := ruleAuthCallbacks{
+		collectVNCUsers: func(_ *PolicyRule, _ map[string]map[string]struct{}) {},
+	}
+	state := &peerConnResolveState{
+		vncAuthorizedUsers: make(map[string]map[string]struct{}),
+	}
+
+	cb.handleVNCRule(rule, true /*peerInSources*/, false /*peerInDestinations*/, state)
+
+	if len(state.vncSessionPubKeys) != 1 {
+		t.Fatalf("expected 1 session pubkey distributed to source peer of bidirectional rule, got %d", len(state.vncSessionPubKeys))
+	}
+	if state.vncSessionPubKeys[0].PubKey != "pubkey-base64" {
+		t.Fatalf("unexpected pubkey: %q", state.vncSessionPubKeys[0].PubKey)
+	}
+}
+
+// TestHandleVNCRule_UnidirectionalSourceGetsNoPubkey makes sure the fix
+// above didn't widen pubkey distribution past the bidirectional case:
+// a strictly source-to-destination rule still must not push the
+// SessionPubKey to peers that appear only in sources.
+func TestHandleVNCRule_UnidirectionalSourceGetsNoPubkey(t *testing.T) {
+	rule := &PolicyRule{
+		Protocol:       PolicyRuleProtocolNetbirdVNC,
+		Bidirectional:  false,
+		AuthorizedUser: "user1",
+		SessionPubKey:  "pubkey-base64",
+	}
+	cb := ruleAuthCallbacks{
+		collectVNCUsers: func(_ *PolicyRule, _ map[string]map[string]struct{}) {},
+	}
+	state := &peerConnResolveState{
+		vncAuthorizedUsers: make(map[string]map[string]struct{}),
+	}
+
+	cb.handleVNCRule(rule, true /*peerInSources*/, false /*peerInDestinations*/, state)
+
+	if len(state.vncSessionPubKeys) != 0 {
+		t.Fatalf("expected NO pubkey for source peer of unidirectional rule, got %d", len(state.vncSessionPubKeys))
+	}
+}
+
+// TestHandleVNCRule_DestinationAlwaysGetsPubkey is the baseline case:
+// destination peers must always receive the SessionPubKey since they're
+// the ones that need to authenticate the incoming Noise handshake.
+func TestHandleVNCRule_DestinationAlwaysGetsPubkey(t *testing.T) {
+	rule := &PolicyRule{
+		Protocol:       PolicyRuleProtocolNetbirdVNC,
+		Bidirectional:  false,
+		AuthorizedUser: "user1",
+		SessionPubKey:  "pubkey-base64",
+	}
+	cb := ruleAuthCallbacks{
+		collectVNCUsers: func(_ *PolicyRule, _ map[string]map[string]struct{}) {},
+	}
+	state := &peerConnResolveState{
+		vncAuthorizedUsers: make(map[string]map[string]struct{}),
+	}
+
+	cb.handleVNCRule(rule, false /*peerInSources*/, true /*peerInDestinations*/, state)
+
+	if len(state.vncSessionPubKeys) != 1 {
+		t.Fatalf("expected 1 session pubkey for destination peer, got %d", len(state.vncSessionPubKeys))
+	}
+}