From 5e2830be8a2942cfdc09638ab8a08f10573da027 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sun, 24 May 2026 16:02:36 +0200 Subject: [PATCH] Harden VNC server, IPC, and management plumbing --- client/cmd/vnc_agent.go | 18 +- client/cmd/vnc_agent_dropprivs_darwin.go | 50 +++ client/cmd/vnc_agent_dropprivs_darwin_test.go | 55 +++ .../vnc_agent_dropprivs_testhelpers_darwin.go | 11 + client/cmd/vnc_agent_dropprivs_windows.go | 14 + client/internal/approval/broker.go | 31 ++ client/internal/approval/fingerprint_test.go | 62 +++ client/ui/approval.go | 28 +- client/ui/client_ui.go | 35 +- client/vnc/server/agent_darwin.go | 107 ++++- client/vnc/server/agent_ipc.go | 53 ++- client/vnc/server/agent_peercred_darwin.go | 46 +++ .../vnc/server/agent_peercred_darwin_test.go | 115 ++++++ client/vnc/server/agent_peercred_windows.go | 19 + client/vnc/server/agent_windows.go | 18 +- client/vnc/server/copyrect.go | 14 +- client/vnc/server/cursor_windows.go | 3 + client/vnc/server/handshake.go | 43 +- client/vnc/server/noise_auth_test.go | 36 +- client/vnc/server/rfb.go | 43 +- client/vnc/server/security_hardening_test.go | 374 ++++++++++++++++++ client/vnc/server/server.go | 55 ++- client/vnc/server/session_cursor.go | 40 +- client/vnc/server/session_encode.go | 26 +- client/wasm/internal/vnc/proxy.go | 21 + .../http/handlers/peers/peers_handler.go | 20 + .../peers/temporary_access_permission_test.go | 106 +++++ .../server/types/policy_authorized_users.go | 10 +- .../policy_authorized_users_security_test.go | 85 ++++ 29 files changed, 1433 insertions(+), 105 deletions(-) create mode 100644 client/cmd/vnc_agent_dropprivs_darwin.go create mode 100644 client/cmd/vnc_agent_dropprivs_darwin_test.go create mode 100644 client/cmd/vnc_agent_dropprivs_testhelpers_darwin.go create mode 100644 client/cmd/vnc_agent_dropprivs_windows.go create mode 100644 client/internal/approval/fingerprint_test.go create mode 100644 client/vnc/server/agent_peercred_darwin.go create mode 100644 client/vnc/server/agent_peercred_darwin_test.go create mode 100644 client/vnc/server/agent_peercred_windows.go create mode 100644 client/vnc/server/security_hardening_test.go create mode 100644 management/server/http/handlers/peers/temporary_access_permission_test.go create mode 100644 management/server/types/policy_authorized_users_security_test.go diff --git a/client/cmd/vnc_agent.go b/client/cmd/vnc_agent.go index 742b498ba..e8993baa6 100644 --- a/client/cmd/vnc_agent.go +++ b/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) } diff --git a/client/cmd/vnc_agent_dropprivs_darwin.go b/client/cmd/vnc_agent_dropprivs_darwin.go new file mode 100644 index 000000000..2e0f080da --- /dev/null +++ b/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 +} diff --git a/client/cmd/vnc_agent_dropprivs_darwin_test.go b/client/cmd/vnc_agent_dropprivs_darwin_test.go new file mode 100644 index 000000000..3d650f0a8 --- /dev/null +++ b/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") + } +} diff --git a/client/cmd/vnc_agent_dropprivs_testhelpers_darwin.go b/client/cmd/vnc_agent_dropprivs_testhelpers_darwin.go new file mode 100644 index 000000000..d4e895b2a --- /dev/null +++ b/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()) +} diff --git a/client/cmd/vnc_agent_dropprivs_windows.go b/client/cmd/vnc_agent_dropprivs_windows.go new file mode 100644 index 000000000..7e6353740 --- /dev/null +++ b/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 +} diff --git a/client/internal/approval/broker.go b/client/internal/approval/broker.go index 0cc0d9514..08eaf72f6 100644 --- a/client/internal/approval/broker.go +++ b/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 ( diff --git a/client/internal/approval/fingerprint_test.go b/client/internal/approval/fingerprint_test.go new file mode 100644 index 000000000..691b2289e --- /dev/null +++ b/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) + } +} diff --git a/client/ui/approval.go b/client/ui/approval.go index 95e16882e..7c99585f9 100644 --- a/client/ui/approval.go +++ b/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 { diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index f05e8dceb..18ca9d08b 100644 --- a/client/ui/client_ui.go +++ b/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 } diff --git a/client/vnc/server/agent_darwin.go b/client/vnc/server/agent_darwin.go index edaf5bdf1..3c470f9da 100644 --- a/client/vnc/server/agent_darwin.go +++ b/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() diff --git a/client/vnc/server/agent_ipc.go b/client/vnc/server/agent_ipc.go index abe903d9e..aba3e7da5 100644 --- a/client/vnc/server/agent_ipc.go +++ b/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") diff --git a/client/vnc/server/agent_peercred_darwin.go b/client/vnc/server/agent_peercred_darwin.go new file mode 100644 index 000000000..d487687a9 --- /dev/null +++ b/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 +} diff --git a/client/vnc/server/agent_peercred_darwin_test.go b/client/vnc/server/agent_peercred_darwin_test.go new file mode 100644 index 000000000..7e559dc51 --- /dev/null +++ b/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() +} diff --git a/client/vnc/server/agent_peercred_windows.go b/client/vnc/server/agent_peercred_windows.go new file mode 100644 index 000000000..6f2d3bc5c --- /dev/null +++ b/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 +} diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index 32c803834..64192d469 100644 --- a/client/vnc/server/agent_windows.go +++ b/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") diff --git a/client/vnc/server/copyrect.go b/client/vnc/server/copyrect.go index 2e0fb56fd..ce75e41fe 100644 --- a/client/vnc/server/copyrect.go +++ b/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 diff --git a/client/vnc/server/cursor_windows.go b/client/vnc/server/cursor_windows.go index b9a92834f..be65a4b1a 100644 --- a/client/vnc/server/cursor_windows.go +++ b/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, diff --git a/client/vnc/server/handshake.go b/client/vnc/server/handshake.go index 50231cff8..ddbc475fc 100644 --- a/client/vnc/server/handshake.go +++ b/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 { diff --git a/client/vnc/server/noise_auth_test.go b/client/vnc/server/noise_auth_test.go index 08eae0839..9ec30d442 100644 --- a/client/vnc/server/noise_auth_test.go +++ b/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) diff --git a/client/vnc/server/rfb.go b/client/vnc/server/rfb.go index f88967642..04d9c7490 100644 --- a/client/vnc/server/rfb.go +++ b/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++ { diff --git a/client/vnc/server/security_hardening_test.go b/client/vnc/server/security_hardening_test.go new file mode 100644 index 000000000..6c60c21f7 --- /dev/null +++ b/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) + } +} diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 0f137d3c5..4c31aae21 100644 --- a/client/vnc/server/server.go +++ b/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, diff --git a/client/vnc/server/session_cursor.go b/client/vnc/server/session_cursor.go index 2ab62ea27..a58ebb437 100644 --- a/client/vnc/server/session_cursor.go +++ b/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)) diff --git a/client/vnc/server/session_encode.go b/client/vnc/server/session_encode.go index bff37e6e7..529c3bae5 100644 --- a/client/vnc/server/session_encode.go +++ b/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 diff --git a/client/wasm/internal/vnc/proxy.go b/client/wasm/internal/vnc/proxy.go index 2d57927ed..1a486c5e6 100644 --- a/client/wasm/internal/vnc/proxy.go +++ b/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, }) diff --git a/management/server/http/handlers/peers/peers_handler.go b/management/server/http/handlers/peers/peers_handler.go index 8d410de77..28f414309 100644 --- a/management/server/http/handlers/peers/peers_handler.go +++ b/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 { diff --git a/management/server/http/handlers/peers/temporary_access_permission_test.go b/management/server/http/handlers/peers/temporary_access_permission_test.go new file mode 100644 index 000000000..6a318c8d0 --- /dev/null +++ b/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()) + } +} diff --git a/management/server/types/policy_authorized_users.go b/management/server/types/policy_authorized_users.go index 672dce3c6..8ce0c48f7 100644 --- a/management/server/types/policy_authorized_users.go +++ b/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, diff --git a/management/server/types/policy_authorized_users_security_test.go b/management/server/types/policy_authorized_users_security_test.go new file mode 100644 index 000000000..b040c9d2f --- /dev/null +++ b/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)) + } +}