From f2c79201b314de93f5b7a8b5ab1fe36ec56c5b91 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Wed, 10 Jun 2026 10:57:50 +0200 Subject: [PATCH] Fix review findings for embedded VNC server --- client/cmd/vnc_agent_dropprivs_darwin.go | 28 ++- client/cmd/vnc_agent_dropprivs_windows.go | 6 +- client/internal/engine_ssh.go | 2 +- client/internal/engine_vnc.go | 13 +- client/ssh/proxy/proxy_test.go | 2 +- client/ssh/server/jwt_test.go | 2 +- client/ssh/server/server.go | 2 +- client/ui/approval.go | 67 +++++++- client/ui/client_ui.go | 30 +--- client/vnc/ports.go | 6 +- client/vnc/server/agent_peercred_windows.go | 28 ++- client/vnc/server/agent_windows.go | 161 +++++++++++++++--- client/vnc/server/capture_darwin.go | 22 +-- client/vnc/server/capture_fb_unix.go | 2 +- client/vnc/server/capture_x11.go | 2 +- client/vnc/server/copyrect.go | 25 +++ client/vnc/server/copyrect_test.go | 63 +++++++ client/vnc/server/cursor_x11.go | 2 +- client/vnc/server/handshake.go | 65 ++++--- client/vnc/server/input_darwin.go | 2 + client/vnc/server/input_x11.go | 2 +- client/vnc/server/metrics_conn.go | 60 ++++--- client/vnc/server/server_test.go | 8 +- client/vnc/server/server_x11.go | 2 +- client/vnc/server/session.go | 62 +++---- client/vnc/server/session_encode.go | 6 +- client/vnc/server/virtual_x11.go | 2 +- client/vnc/server/xauth_x11.go | 2 +- client/wasm/cmd/main.go | 13 +- client/wasm/internal/vnc/proxy.go | 141 +++++++++------ .../peers/temporary_access_permission_test.go | 7 +- management/server/peer/peer.go | 1 - management/server/store/sql_store.go | 12 +- management/server/types/account.go | 3 +- .../server/types/networkmap_components.go | 21 ++- .../server/types/policy_authorized_users.go | 2 +- .../policy_authorized_users_security_test.go | 71 +++++++- shared/sessionauth/auth.go | 14 +- util/capture/text.go | 24 +-- 39 files changed, 694 insertions(+), 289 deletions(-) diff --git a/client/cmd/vnc_agent_dropprivs_darwin.go b/client/cmd/vnc_agent_dropprivs_darwin.go index 2e0f080da..d61e33747 100644 --- a/client/cmd/vnc_agent_dropprivs_darwin.go +++ b/client/cmd/vnc_agent_dropprivs_darwin.go @@ -5,6 +5,8 @@ package cmd import ( "fmt" "os" + "os/user" + "strconv" "syscall" ) @@ -31,14 +33,21 @@ func dropAgentPrivileges(targetUID uint32) error { if cur != 0 { return fmt.Errorf("agent uid %d does not match expected %d and we lack root to fix it", cur, targetUID) } + // Resolve the target user's real primary group rather than reusing + // targetUID as the gid: a user's primary group on macOS is typically + // staff(20), not gid==uid. Fail closed if the lookup fails. + targetGID, err := primaryGroupID(targetUID) + if err != nil { + return err + } // 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.Setgid(targetGID); err != nil { + return fmt.Errorf("setgid(%d): %w", targetGID, err) } if err := syscall.Setuid(int(targetUID)); err != nil { return fmt.Errorf("setuid(%d): %w", targetUID, err) @@ -48,3 +57,18 @@ func dropAgentPrivileges(targetUID uint32) error { } return nil } + +// primaryGroupID resolves the real primary group id of the user with the +// given uid. Fails closed: a lookup or parse error returns an error so the +// caller never falls back to using uid as the gid. +func primaryGroupID(targetUID uint32) (int, error) { + u, err := user.LookupId(strconv.Itoa(int(targetUID))) + if err != nil { + return 0, fmt.Errorf("look up uid %d: %w", targetUID, err) + } + gid, err := strconv.Atoi(u.Gid) + if err != nil { + return 0, fmt.Errorf("parse gid %q for uid %d: %w", u.Gid, targetUID, err) + } + return gid, nil +} diff --git a/client/cmd/vnc_agent_dropprivs_windows.go b/client/cmd/vnc_agent_dropprivs_windows.go index 7e6353740..7803be77c 100644 --- a/client/cmd/vnc_agent_dropprivs_windows.go +++ b/client/cmd/vnc_agent_dropprivs_windows.go @@ -6,9 +6,9 @@ package cmd // 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. +// account). The Windows path relies on the DACL-restricted socket +// directory, the unpredictable per-spawn socket name, the listen-readiness +// gate, and the per-spawn token for integrity instead. func dropAgentPrivileges(_ uint32) error { return nil } diff --git a/client/internal/engine_ssh.go b/client/internal/engine_ssh.go index 17296d6d7..0dbd4a16f 100644 --- a/client/internal/engine_ssh.go +++ b/client/internal/engine_ssh.go @@ -12,10 +12,10 @@ import ( firewallManager "github.com/netbirdio/netbird/client/firewall/manager" "github.com/netbirdio/netbird/client/iface/netstack" nftypes "github.com/netbirdio/netbird/client/internal/netflow/types" - sshauth "github.com/netbirdio/netbird/shared/sessionauth" sshconfig "github.com/netbirdio/netbird/client/ssh/config" sshserver "github.com/netbirdio/netbird/client/ssh/server" mgmProto "github.com/netbirdio/netbird/shared/management/proto" + sshauth "github.com/netbirdio/netbird/shared/sessionauth" sshuserhash "github.com/netbirdio/netbird/shared/sshauth" ) diff --git a/client/internal/engine_vnc.go b/client/internal/engine_vnc.go index 9cebd014e..7aff50f90 100644 --- a/client/internal/engine_vnc.go +++ b/client/internal/engine_vnc.go @@ -17,12 +17,11 @@ import ( "github.com/netbirdio/netbird/client/internal/peer" "github.com/netbirdio/netbird/client/vnc" vncserver "github.com/netbirdio/netbird/client/vnc/server" - sshauth "github.com/netbirdio/netbird/shared/sessionauth" mgmProto "github.com/netbirdio/netbird/shared/management/proto" + sshauth "github.com/netbirdio/netbird/shared/sessionauth" sshuserhash "github.com/netbirdio/netbird/shared/sshauth" ) - type vncServer interface { Start(ctx context.Context, addr netip.AddrPort, network netip.Prefix) error Stop() error @@ -188,13 +187,13 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) { } sessionPubKeys := make([]sshauth.SessionPubKey, 0, len(vncAuth.GetSessionPubKeys())) - for _, e := range vncAuth.GetSessionPubKeys() { - pub := e.GetPubKey() + for _, pk := range vncAuth.GetSessionPubKeys() { + pub := pk.GetPubKey() if len(pub) != 32 { log.Warnf("VNC session pubkey wrong length %d", len(pub)) continue } - hash := e.GetUserIdHash() + hash := pk.GetUserIdHash() if len(hash) != 16 { log.Warnf("VNC session user id hash wrong length %d", len(hash)) continue @@ -202,7 +201,7 @@ func (e *Engine) updateVNCServerAuth(vncAuth *mgmProto.VNCAuth) { sessionPubKeys = append(sessionPubKeys, sshauth.SessionPubKey{ PubKey: pub, UserIDHash: sshuserhash.UserIDHash(hash), - DisplayName: e.GetDisplayName(), + DisplayName: pk.GetDisplayName(), }) } @@ -236,7 +235,7 @@ func (e *Engine) stopVNCServer() error { log.Warnf("cleanup VNC port redirection: %v", err) } - if netstackNet := e.wgInterface.GetNet(); netstackNet != nil { + if e.wgInterface != nil && e.wgInterface.GetNet() != nil { if registrar, ok := e.firewall.(interface { UnregisterNetstackService(protocol nftypes.Protocol, port uint16) }); ok { diff --git a/client/ssh/proxy/proxy_test.go b/client/ssh/proxy/proxy_test.go index 02cd1d58c..8c2b67a4f 100644 --- a/client/ssh/proxy/proxy_test.go +++ b/client/ssh/proxy/proxy_test.go @@ -28,10 +28,10 @@ import ( "github.com/netbirdio/netbird/client/proto" nbssh "github.com/netbirdio/netbird/client/ssh" - sshauth "github.com/netbirdio/netbird/shared/sessionauth" "github.com/netbirdio/netbird/client/ssh/server" "github.com/netbirdio/netbird/client/ssh/testutil" nbjwt "github.com/netbirdio/netbird/shared/auth/jwt" + sshauth "github.com/netbirdio/netbird/shared/sessionauth" sshuserhash "github.com/netbirdio/netbird/shared/sshauth" ) diff --git a/client/ssh/server/jwt_test.go b/client/ssh/server/jwt_test.go index def3658c9..66d6b9e04 100644 --- a/client/ssh/server/jwt_test.go +++ b/client/ssh/server/jwt_test.go @@ -23,11 +23,11 @@ import ( "github.com/stretchr/testify/require" nbssh "github.com/netbirdio/netbird/client/ssh" - sshauth "github.com/netbirdio/netbird/shared/sessionauth" "github.com/netbirdio/netbird/client/ssh/client" "github.com/netbirdio/netbird/client/ssh/detection" "github.com/netbirdio/netbird/client/ssh/testutil" nbjwt "github.com/netbirdio/netbird/shared/auth/jwt" + sshauth "github.com/netbirdio/netbird/shared/sessionauth" sshuserhash "github.com/netbirdio/netbird/shared/sshauth" ) diff --git a/client/ssh/server/server.go b/client/ssh/server/server.go index 499743c66..20ed55986 100644 --- a/client/ssh/server/server.go +++ b/client/ssh/server/server.go @@ -23,10 +23,10 @@ import ( "golang.zx2c4.com/wireguard/tun/netstack" "github.com/netbirdio/netbird/client/iface/wgaddr" - sshauth "github.com/netbirdio/netbird/shared/sessionauth" "github.com/netbirdio/netbird/client/ssh/detection" "github.com/netbirdio/netbird/shared/auth" "github.com/netbirdio/netbird/shared/auth/jwt" + sshauth "github.com/netbirdio/netbird/shared/sessionauth" "github.com/netbirdio/netbird/util/netrelay" "github.com/netbirdio/netbird/version" ) diff --git a/client/ui/approval.go b/client/ui/approval.go index 7c99585f9..0c2dd1f7e 100644 --- a/client/ui/approval.go +++ b/client/ui/approval.go @@ -4,7 +4,10 @@ package main import ( "context" + "errors" "fmt" + "os" + "os/exec" "strings" "time" @@ -17,6 +20,18 @@ import ( "github.com/netbirdio/netbird/client/proto" ) +// Approval metadata that is remote-peer or dashboard controlled is passed to +// the forked netbird-ui via environment variables rather than argv, so it is +// not exposed to other local users through ps. +const ( + envApprovalInitiator = "NB_APPROVAL_INITIATOR" + envApprovalPeerName = "NB_APPROVAL_PEER_NAME" + envApprovalSourceIP = "NB_APPROVAL_SOURCE_IP" + envApprovalUsername = "NB_APPROVAL_USERNAME" + envApprovalKeyFingerprint = "NB_APPROVAL_KEY_FINGERPRINT" + envApprovalSubject = "NB_APPROVAL_SUBJECT" +) + // handleApprovalEvent forks a netbird-ui child process to render the // dialog on its own fyne main loop. Top-level windows opened from a // background goroutine of the tray process don't render reliably on @@ -31,18 +46,56 @@ func (s *serviceClient) handleApprovalEvent(ev *proto.SystemEvent) { log.Warnf("approval event missing request_id: %v", ev.Metadata) return } + + // Only the request id, kind, and deadline stay on argv: they are + // daemon-issued and non-sensitive. The remote-influenced fields go + // through the child's environment. args := []string{ "--approval-request-id=" + requestID, "--approval-kind=" + ev.Metadata["kind"], - "--approval-initiator=" + ev.Metadata["initiator"], - "--approval-peer-name=" + ev.Metadata["peer_name"], - "--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...) + env := append(os.Environ(), + envApprovalInitiator+"="+ev.Metadata["initiator"], + envApprovalPeerName+"="+ev.Metadata["peer_name"], + envApprovalSourceIP+"="+ev.Metadata["source_ip"], + envApprovalUsername+"="+ev.Metadata["username"], + envApprovalKeyFingerprint+"="+ev.Metadata["peer_pubkey"], + envApprovalSubject+"="+ev.UserMessage, + ) + go s.runApprovalCommand(s.ctx, env, args) +} + +// runApprovalCommand forks netbird-ui to render the approval dialog, +// inheriting the parent environment plus the approval-specific variables. It +// mirrors runSelfCommand but sets cmd.Env so the sensitive metadata never +// appears on the child's argv. +func (s *serviceClient) runApprovalCommand(ctx context.Context, env, args []string) { + proc, err := os.Executable() + if err != nil { + log.Errorf("get executable path: %v", err) + return + } + + cmdArgs := append([]string{"--approval=true", "--daemon-addr=" + s.addr}, args...) + cmd := exec.CommandContext(ctx, proc, cmdArgs...) + cmd.Env = env + + if out := s.attachOutput(cmd); out != nil { + defer func() { + if err := out.Close(); err != nil { + log.Errorf("close log file %s: %v", s.logFile, err) + } + }() + } + + log.Printf("running approval command: %s", cmd.String()) + if err := cmd.Run(); err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + log.Printf("approval command failed with exit code %d", exitErr.ExitCode()) + } + } } // showApprovalUI runs the dialog on the forked process's fyne main loop diff --git a/client/ui/client_ui.go b/client/ui/client_ui.go index d030937a6..164bbcf8a 100644 --- a/client/ui/client_ui.go +++ b/client/ui/client_ui.go @@ -101,13 +101,13 @@ func main() { approvalRequest: approvalRequest{ requestID: flags.approvalRequestID, kind: flags.approvalKind, - initiator: flags.approvalInitiator, - peerName: flags.approvalPeerName, - sourceIP: flags.approvalSourceIP, - username: flags.approvalUsername, - subject: flags.approvalSubject, + initiator: os.Getenv(envApprovalInitiator), + peerName: os.Getenv(envApprovalPeerName), + sourceIP: os.Getenv(envApprovalSourceIP), + username: os.Getenv(envApprovalUsername), + subject: os.Getenv(envApprovalSubject), expiresAt: flags.approvalExpiresAt, - keyFingerprint: flags.approvalKeyFingerprint, + keyFingerprint: os.Getenv(envApprovalKeyFingerprint), }, }) @@ -154,15 +154,9 @@ type cliFlags struct { showUpdateVersion string showApproval bool - approvalRequestID string - approvalKind string - approvalInitiator string - approvalPeerName string - approvalSourceIP string - approvalUsername string - approvalSubject string - approvalExpiresAt string - approvalKeyFingerprint string + approvalRequestID string + approvalKind string + approvalExpiresAt string } // parseFlags reads and returns all needed command-line flags. @@ -187,13 +181,7 @@ func parseFlags() *cliFlags { flag.BoolVar(&flags.showApproval, "approval", false, "show inbound-connection approval prompt window") flag.StringVar(&flags.approvalRequestID, "approval-request-id", "", "approval prompt: daemon-issued request id") flag.StringVar(&flags.approvalKind, "approval-kind", "", "approval prompt: subsystem kind (vnc, ssh, ...)") - flag.StringVar(&flags.approvalInitiator, "approval-initiator", "", "approval prompt: display name of the user who initiated the connection") - flag.StringVar(&flags.approvalPeerName, "approval-peer-name", "", "approval prompt: remote peer FQDN") - flag.StringVar(&flags.approvalSourceIP, "approval-source-ip", "", "approval prompt: remote source IP") - 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/ports.go b/client/vnc/ports.go index 5e80810c1..33c9bc2cf 100644 --- a/client/vnc/ports.go +++ b/client/vnc/ports.go @@ -11,9 +11,9 @@ package vnc // sockets; kept here so packet captures from older builds still get // tagged, and so any future on-wire agent variant has a reserved port. const ( - ExternalPort uint16 = 5900 - InternalPort uint16 = 25900 - AgentLegacyPort uint16 = 15900 + ExternalPort uint16 = 5900 + InternalPort uint16 = 25900 + AgentLegacyPort uint16 = 15900 ) // WellKnownPorts is the unordered set of ports a packet capture should diff --git a/client/vnc/server/agent_peercred_windows.go b/client/vnc/server/agent_peercred_windows.go index 6f2d3bc5c..d6d4f82d6 100644 --- a/client/vnc/server/agent_peercred_windows.go +++ b/client/vnc/server/agent_peercred_windows.go @@ -6,14 +6,26 @@ 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. +// validateAgentPeer is a documented no-op on Windows. AF_UNIX on Windows +// exposes no SO_PEERCRED equivalent and no supported API to recover the +// peer process from an accepted AF_UNIX connection, so the daemon cannot +// match the connected peer against the agent PID it spawned the way the +// darwin path does via LOCAL_PEERCRED. The Windows trust model therefore +// rests on three other measures, none of which assume the socket path is +// secret: +// +// - the socket lives in a dedicated directory (agentSocketDir) created +// with a DACL granting only SYSTEM and Administrators, so an +// unprivileged local user cannot create or squat a socket there; +// - each spawn uses a cryptographically random socket name, so the path +// is unguessable before the agent binds it; +// - the daemon publishes the path only after confirming the spawned +// agent is listening (see waitForAgentListening), and gates every +// connection on the per-spawn auth-token preamble that follows this +// call. +// +// If a future Windows release exposes peer-PID retrieval for AF_UNIX, +// this function should verify the peer against the spawned agent PID. func validateAgentPeer(_ net.Conn, _ uint32) error { return nil } diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index 74fa7e0dc..b726bcc37 100644 --- a/client/vnc/server/agent_windows.go +++ b/client/vnc/server/agent_windows.go @@ -4,10 +4,14 @@ package server import ( "context" + crand "crypto/rand" "encoding/binary" + "encoding/hex" "errors" "fmt" + "net" "os" + "path/filepath" "runtime" "sync" "time" @@ -362,10 +366,33 @@ type sessionManager struct { jobHandle windows.Handle } -// agentSocketPathFmt parameterizes the per-session agent socket path by -// the Windows session id. C:\Windows\Temp is writable to both the daemon -// (SYSTEM) and the spawned agent (SYSTEM token impersonating the session). -const agentSocketPathFmt = `C:\Windows\Temp\netbird-vnc-%d.sock` +const ( + // agentSocketDir is a dedicated subdirectory under C:\Windows\Temp that + // the daemon creates with a restrictive DACL (SYSTEM + Administrators + // only). The default ACL on C:\Windows\Temp grants BUILTIN\Users + // create-file rights, so the agent socket must not live directly there: + // an unprivileged local user could pre-create a predictable path and + // intercept the daemon→agent stream. Both the daemon and the agent run + // as SYSTEM, so a SYSTEM-write-only directory is sufficient. + agentSocketDir = `C:\Windows\Temp\netbird-vnc` + + // agentSocketDirSDDL grants full access to Local System (SY) and the + // Builtin Administrators group (BA) only, with the DACL protected + // (P) from inheritance so the parent's BUILTIN\Users grant does not + // flow in. AI is omitted; PAI marks the DACL protected and auto- + // inherited entries cleared. + agentSocketDirSDDL = "D:PAI(A;;FA;;;SY)(A;;FA;;;BA)" + + // agentSocketRandomLen is the number of random bytes mixed into each + // per-spawn socket name so the path is unguessable before the agent + // owns it. + agentSocketRandomLen = 16 + + // agentReadyTimeout bounds how long the daemon waits for the freshly + // spawned agent to bind and accept on its socket before treating the + // spawn as failed. + agentReadyTimeout = 5 * time.Second +) func newSessionManager() *sessionManager { m := &sessionManager{sessionID: ^uint32(0), done: make(chan struct{})} @@ -427,11 +454,14 @@ func createKillOnCloseJob() (windows.Handle, 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. +// SYSTEM in the interactive session; see validateAgentPeer for the +// Windows trust model). The path is only published after the spawned +// agent is confirmed listening, so a caller never receives a socket a +// squatter could be holding. When no agent is spawned yet (initial +// 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() @@ -547,13 +577,21 @@ func (m *sessionManager) maybeSpawnAgent(sid uint32) bool { if m.agentProc != 0 || sid == 0xFFFFFFFF || !time.Now().After(m.nextSpawnAt) { return true } - // Reap any orphan still holding the agent port from a previous - // service instance, only on our very first spawn. Once we own - // an agent, we manage its lifecycle ourselves and never need to - // kill an unknown listener; if a kill+respawn races on port - // release, the spawn-failure backoff handles it without forcing - // a synchronous wait or duplicate kill. - socketPath := fmt.Sprintf(agentSocketPathFmt, sid) + + if err := ensureAgentSocketDir(); err != nil { + log.Warnf("prepare agent socket dir: %v", err) + m.nextSpawnAt = time.Now().Add(5 * time.Second) + return true + } + + // The leaf name carries a cryptographically random component so a local + // user cannot pre-create the path at a guessable location. The session + // id is kept for diagnostics only; security does not rely on it. + socketPath, err := newAgentSocketPath(sid) + if err != nil { + log.Warnf("generate agent socket path: %v", err) + return true + } // Covers a previous-run crash that escaped Job Object kill-on-close. if err := os.Remove(socketPath); err != nil && !os.IsNotExist(err) { log.Debugf("clear stale agent socket %s: %v", socketPath, err) @@ -563,12 +601,8 @@ func (m *sessionManager) maybeSpawnAgent(sid uint32) bool { log.Warnf("generate agent auth token: %v", err) return true } - m.authToken = token - m.socketPath = socketPath - h, err := spawnAgentInSession(sid, socketPath, m.authToken, m.jobHandle) + h, err := spawnAgentInSession(sid, socketPath, token, m.jobHandle) if err != nil { - m.authToken = "" - m.socketPath = "" if errors.Is(err, windows.ERROR_PRIVILEGE_NOT_HELD) { // SE_TCB_NAME (token-impersonation across sessions) is only // granted to SYSTEM. Without it spawnAgent will fail every 2 @@ -579,12 +613,97 @@ func (m *sessionManager) maybeSpawnAgent(sid uint32) bool { log.Warnf("spawn agent in session %d: %v", sid, err) return true } + + // Gate on listen-readiness before publishing the path: do not hand a + // caller a socket the agent has not bound yet. On timeout, fail closed + // by killing the agent and leaving socketPath/authToken unset so + // Resolve keeps returning errAgentNotReady. + if err := waitForAgentListening(socketPath, agentReadyTimeout); err != nil { + log.Warnf("agent in session %d did not start listening: %v", sid, err) + _ = windows.TerminateProcess(h, 1) + _ = windows.CloseHandle(h) + if rmErr := os.Remove(socketPath); rmErr != nil && !os.IsNotExist(rmErr) { + log.Debugf("clear unready agent socket %s: %v", socketPath, rmErr) + } + m.scheduleNextSpawn(0, 0) + return true + } + + m.authToken = token + m.socketPath = socketPath m.agentProc = h m.agentStartedAt = time.Now() m.everSpawned = true return true } +// ensureAgentSocketDir creates the dedicated socket directory with a +// restrictive DACL (SYSTEM + Administrators only). A pre-existing directory +// is torn down and recreated rather than reused: it may have been created by +// an unprivileged user with a permissive ACL, and it only ever holds our +// transient sockets, so removing it loses nothing. Fails closed: returns an +// error if the directory cannot be created with the intended security. +func ensureAgentSocketDir() error { + sd, err := windows.SecurityDescriptorFromString(agentSocketDirSDDL) + if err != nil { + return fmt.Errorf("parse socket dir SDDL: %w", err) + } + var sa windows.SecurityAttributes + sa.Length = uint32(unsafe.Sizeof(sa)) + sa.SecurityDescriptor = sd + + dirW, err := windows.UTF16PtrFromString(agentSocketDir) + if err != nil { + return fmt.Errorf("encode socket dir path: %w", err) + } + err = windows.CreateDirectory(dirW, &sa) + if errors.Is(err, windows.ERROR_ALREADY_EXISTS) { + if rmErr := os.RemoveAll(agentSocketDir); rmErr != nil { + return fmt.Errorf("remove pre-existing socket dir %s: %w", agentSocketDir, rmErr) + } + err = windows.CreateDirectory(dirW, &sa) + } + if err != nil { + return fmt.Errorf("create socket dir %s: %w", agentSocketDir, err) + } + return nil +} + +// newAgentSocketPath returns a per-spawn socket path inside the secured +// socket directory. The leaf name mixes a cryptographically random component +// with the session id (for diagnostics) so the path is unguessable before the +// agent binds it. +func newAgentSocketPath(sessionID uint32) (string, error) { + b := make([]byte, agentSocketRandomLen) + if _, err := crand.Read(b); err != nil { + return "", fmt.Errorf("read random: %w", err) + } + name := fmt.Sprintf("netbird-vnc-%d-%s.sock", sessionID, hex.EncodeToString(b)) + return filepath.Join(agentSocketDir, name), nil +} + +// waitForAgentListening dials the agent's Unix socket until it answers or the +// timeout elapses. Mirrors the darwin readiness gate so the daemon never +// exposes a socket path before the legitimate agent owns it. +func waitForAgentListening(socketPath string, wait time.Duration) error { + var d net.Dialer + deadline := time.Now().Add(wait) + var lastErr error + for time.Now().Before(deadline) { + c, err := d.Dial("unix", socketPath) + if err == nil { + _ = c.Close() + return nil + } + lastErr = err + time.Sleep(100 * time.Millisecond) + } + if lastErr == nil { + lastErr = fmt.Errorf("timeout") + } + return fmt.Errorf("dial %s: %w", socketPath, lastErr) +} + func (m *sessionManager) killAgent() { if m.agentProc == 0 { return diff --git a/client/vnc/server/capture_darwin.go b/client/vnc/server/capture_darwin.go index 6b975ed87..d3c31a1a1 100644 --- a/client/vnc/server/capture_darwin.go +++ b/client/vnc/server/capture_darwin.go @@ -204,10 +204,11 @@ func (c *CGCapturer) Width() int { return c.w } // Height returns the screen height. func (c *CGCapturer) Height() int { return c.h } -// Capture returns the current screen as an RGBA image. // CaptureInto writes a fresh frame directly into dst, skipping the -// per-frame image.RGBA allocation that Capture() does. Returns -// errFrameUnchanged when the screen hash matches the prior call. +// per-frame image.RGBA allocation that Capture() does. It always fills +// dst: the capturer is shared across all sessions, so dedup here would +// starve every consumer but the first one to poll after a change. +// Per-session prevFrame diffing in the session layer handles no-op frames. func (c *CGCapturer) CaptureInto(dst *image.RGBA) error { cgImage := cgDisplayCreateImage(c.displayID) if cgImage == 0 { @@ -233,12 +234,6 @@ func (c *CGCapturer) CaptureInto(dst *image.RGBA) error { return fmt.Errorf("empty image data") } src := unsafe.Slice((*byte)(unsafe.Pointer(dataPtr)), dataLen) - hash := maphash.Bytes(c.hashSeed, src) - if c.hasHash && hash == c.lastHash { - return errFrameUnchanged - } - c.lastHash = hash - c.hasHash = true ds := c.downscale if ds < 1 { @@ -565,14 +560,7 @@ func (p *MacPoller) CaptureInto(dst *image.RGBA) error { if err := p.ensureCapturerLocked(); err != nil { return err } - err := p.capturer.CaptureInto(dst) - if errors.Is(err, errFrameUnchanged) { - // Caller (session) treats this as "no change"; the dst buffer - // keeps its prior contents from the previous capture cycle so - // the diff stays meaningful. - return err - } - if err != nil { + if err := p.capturer.CaptureInto(dst); err != nil { p.capturer = nil return fmt.Errorf("macos capture: %w", err) } diff --git a/client/vnc/server/capture_fb_unix.go b/client/vnc/server/capture_fb_unix.go index 0c2a0dac0..63351dcdb 100644 --- a/client/vnc/server/capture_fb_unix.go +++ b/client/vnc/server/capture_fb_unix.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/capture_x11.go b/client/vnc/server/capture_x11.go index bdfaf7c42..cb28201d7 100644 --- a/client/vnc/server/capture_x11.go +++ b/client/vnc/server/capture_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/copyrect.go b/client/vnc/server/copyrect.go index ce75e41fe..4e73e2a54 100644 --- a/client/vnc/server/copyrect.go +++ b/client/vnc/server/copyrect.go @@ -191,6 +191,16 @@ func (d *copyRectDetector) extractCopyRectTiles(cur *image.RGBA, dirtyTiles [][4 for _, r := range dirtyTiles { if r[2] == ts && r[3] == ts { if sx, sy, ok := d.findTileMatch(cur, r[0], r[1]); ok { + // The client applies moves sequentially against its live + // framebuffer. If this move's source overlaps the + // destination of any move already queued, that destination + // has overwritten the source pixels client-side, so the + // copy would read corrupted data. Drop it and let the tile + // fall through to normal pixel encoding instead. + if tileOverlapsPriorDst(moves, sx, sy, ts) { + remaining = append(remaining, r) + continue + } moves = append(moves, copyRectMove{ srcX: sx, srcY: sy, dstX: r[0], dstY: r[1], }) @@ -201,3 +211,18 @@ func (d *copyRectDetector) extractCopyRectTiles(cur *image.RGBA, dirtyTiles [][4 } return moves, remaining } + +// tileOverlapsPriorDst reports whether the tileSize-square source rectangle +// at (srcX, srcY) intersects the destination rectangle of any move already +// emitted. All move rectangles are ts×ts, so the test reduces to a +// per-axis distance check. +func tileOverlapsPriorDst(moves []copyRectMove, srcX, srcY, ts int) bool { + for _, m := range moves { + dx := srcX - m.dstX + dy := srcY - m.dstY + if dx > -ts && dx < ts && dy > -ts && dy < ts { + return true + } + } + return false +} diff --git a/client/vnc/server/copyrect_test.go b/client/vnc/server/copyrect_test.go index 0295e6c26..610a04aba 100644 --- a/client/vnc/server/copyrect_test.go +++ b/client/vnc/server/copyrect_test.go @@ -83,6 +83,69 @@ func TestCopyRectDetector_DetectsVerticalScroll(t *testing.T) { } } +// rectsOverlap reports whether two ts×ts tiles at the given origins overlap. +func tilesOverlap(ax, ay, bx, by, ts int) bool { + return ax < bx+ts && bx < ax+ts && ay < by+ts && by < ay+ts +} + +// TestCopyRectDetector_DownwardScrollNoOverlap exercises a downward scroll, +// where each move's source is the destination of the move one row above it. +// Emitting all of them in order would corrupt the client framebuffer because +// the earlier move overwrites the source pixels the later move reads. The +// detector must drop any move whose source overlaps a prior move's +// destination and route that tile to pixel encoding instead. +func TestCopyRectDetector_DownwardScrollNoOverlap(t *testing.T) { + const w, h = 256, 192 // 4×3 tiles at 64px + const ts = 64 + + prev := image.NewRGBA(image.Rect(0, 0, w, h)) + cur := image.NewRGBA(image.Rect(0, 0, w, h)) + + // prev: 12 tiles each with a unique colour. + for ty := 0; ty < 3; ty++ { + for tx := 0; tx < 4; tx++ { + fillTile(prev, tx*ts, ty*ts, ts, byte(tx*40), byte(ty*60), 0x80) + } + } + // cur: scroll downward by one row. Rows 1 and 2 are copied from prev + // rows 0 and 1; the top row is new content. + for ty := 1; ty < 3; ty++ { + for tx := 0; tx < 4; tx++ { + copyTile(cur, prev, tx*ts, (ty-1)*ts, tx*ts, ty*ts, ts) + } + } + for tx := 0; tx < 4; tx++ { + fillTile(cur, tx*ts, 0, ts, 0xff, 0xff, 0xff) + } + + d := newCopyRectDetector(ts) + d.rebuild(prev, w, h) + + tiles := diffTiles(prev, cur, w, h, ts) + wantTiles := len(tiles) + moves, remaining := d.extractCopyRectTiles(cur, tiles) + + // No move's source may overlap an earlier move's destination. + for i, m := range moves { + for _, prior := range moves[:i] { + if tilesOverlap(m.srcX, m.srcY, prior.dstX, prior.dstY, ts) { + t.Fatalf("move %d src (%d,%d) overlaps prior dst (%d,%d)", + i, m.srcX, m.srcY, prior.dstX, prior.dstY) + } + } + } + + // The dropped row-2 moves must fall through to pixel encoding rather than + // being silently skipped, so the region still updates correctly. + if len(moves)+len(remaining) != wantTiles { + t.Fatalf("moves(%d)+remaining(%d) != dirty tiles(%d): a tile was lost", + len(moves), len(remaining), wantTiles) + } + if len(moves) != 4 { + t.Fatalf("moves: want 4 (top scrolled row only), got %d", len(moves)) + } +} + func TestCopyRectDetector_RejectsSelfMatch(t *testing.T) { const w, h = 128, 128 const ts = 64 diff --git a/client/vnc/server/cursor_x11.go b/client/vnc/server/cursor_x11.go index a00ddf55c..e14ff856b 100644 --- a/client/vnc/server/cursor_x11.go +++ b/client/vnc/server/cursor_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/handshake.go b/client/vnc/server/handshake.go index ddbc475fc..ae2460e2f 100644 --- a/client/vnc/server/handshake.go +++ b/client/vnc/server/handshake.go @@ -3,7 +3,6 @@ package server import ( - "bufio" "bytes" "crypto/subtle" "encoding/binary" @@ -119,21 +118,35 @@ func (s *Server) readConnectionHeader(conn net.Conn) (*connectionHeader, error) username = string(buf) } - br := bufio.NewReader(conn) - clientStatic, identityVerified, err := s.maybeRunNoiseHandshake(conn, br, mode, username) + // Read the 4-byte magic candidate directly off the wire instead of + // buffering ahead with a bufio.Reader: the session reads the raw conn + // after this returns, so any bytes a bufio.Reader buffered past the + // header would be silently dropped. When the bytes aren't the v3 magic + // they are the start of the session_id field and feed straight into it. + var magicBuf [4]byte + if _, err := io.ReadFull(conn, magicBuf[:]); err != nil { + return &connectionHeader{mode: mode, username: username}, nil + } + + clientStatic, identityVerified, magicConsumed, err := s.maybeRunNoiseHandshake(conn, magicBuf, mode, username) if err != nil { return nil, err } var sessionID uint32 - var sidBuf [4]byte - if _, err := io.ReadFull(br, sidBuf[:]); err == nil { - sessionID = binary.BigEndian.Uint32(sidBuf[:]) + var width, height uint16 + if magicConsumed { + var sidBuf [4]byte + if _, err := io.ReadFull(conn, sidBuf[:]); err == nil { + sessionID = binary.BigEndian.Uint32(sidBuf[:]) + } + } else { + // No magic: the 4 bytes we already read are the session_id. + sessionID = binary.BigEndian.Uint32(magicBuf[:]) } - var width, height uint16 var geomBuf [4]byte - if _, err := io.ReadFull(br, geomBuf[:]); err == nil { + if _, err := io.ReadFull(conn, geomBuf[:]); err == nil { width = binary.BigEndian.Uint16(geomBuf[0:2]) height = binary.BigEndian.Uint16(geomBuf[2:4]) } @@ -155,18 +168,14 @@ func (s *Server) readConnectionHeader(conn net.Conn) (*connectionHeader, 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 - } - if _, err := br.Discard(len(vncIdentityMagic)); err != nil { - return nil, false, fmt.Errorf("discard identity magic: %w", err) +func (s *Server) maybeRunNoiseHandshake(conn net.Conn, magic [4]byte, headerMode byte, headerUsername string) (clientStatic []byte, identityVerified, magicConsumed bool, err error) { + if !bytes.Equal(magic[:], vncIdentityMagic) { + return nil, false, false, nil } msg1 := make([]byte, noiseInitiatorMsgLen) - if _, err := io.ReadFull(br, msg1); err != nil { - return nil, false, fmt.Errorf("read noise msg1: %w", err) + if _, err := io.ReadFull(conn, msg1); err != nil { + return nil, false, true, fmt.Errorf("read noise msg1: %w", err) } // Agents on loopback authenticate via the agent token, not this @@ -179,11 +188,11 @@ func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader, headerM // short-circuit will see the truthful "no Noise identity proved // here" rather than a stale true. if s.disableAuth { - return nil, false, nil + return nil, false, true, nil } if len(s.identityKey) != 32 || len(s.identityPublic) != 32 { - return nil, false, errors.New("identity key not configured") + return nil, false, true, errors.New("identity key not configured") } state, err := noise.NewHandshakeState(noise.Config{ CipherSuite: vncNoiseSuite, @@ -193,27 +202,27 @@ func (s *Server) maybeRunNoiseHandshake(conn net.Conn, br *bufio.Reader, headerM StaticKeypair: noise.DHKey{Private: s.identityKey, Public: s.identityPublic}, }) if err != nil { - return nil, false, fmt.Errorf("noise responder init: %w", err) + return nil, false, true, fmt.Errorf("noise responder init: %w", err) } if _, _, _, err := state.ReadMessage(nil, msg1); err != nil { - return nil, false, fmt.Errorf("noise read msg1: %w", err) + return nil, false, true, fmt.Errorf("noise read msg1: %w", err) } msg2, _, _, err := state.WriteMessage(nil, nil) if err != nil { - return nil, false, fmt.Errorf("noise write msg2: %w", err) + return nil, false, true, fmt.Errorf("noise write msg2: %w", err) } if len(msg2) != noiseResponderMsgLen { - return nil, false, fmt.Errorf("noise responder produced %d bytes, expected %d", len(msg2), noiseResponderMsgLen) + return nil, false, true, fmt.Errorf("noise responder produced %d bytes, expected %d", len(msg2), noiseResponderMsgLen) } if _, err := conn.Write(msg2); err != nil { - return nil, false, fmt.Errorf("write noise msg2: %w", err) + return nil, false, true, fmt.Errorf("write noise msg2: %w", err) } - clientStatic := state.PeerStatic() - if len(clientStatic) != 32 { - return nil, false, errors.New("noise peer static missing") + peerStatic := state.PeerStatic() + if len(peerStatic) != 32 { + return nil, false, true, errors.New("noise peer static missing") } - return clientStatic, true, nil + return peerStatic, true, true, nil } // verifyAgentToken validates the agent token prefix when configured and diff --git a/client/vnc/server/input_darwin.go b/client/vnc/server/input_darwin.go index b1b4e9926..76f4c3a79 100644 --- a/client/vnc/server/input_darwin.go +++ b/client/vnc/server/input_darwin.go @@ -281,6 +281,8 @@ func releasePreventIdleSleep() { } func ensureEventSource() uintptr { + pmMu.Lock() + defer pmMu.Unlock() if darwinEventSource != 0 { return darwinEventSource } diff --git a/client/vnc/server/input_x11.go b/client/vnc/server/input_x11.go index d0588470e..a3eecb95c 100644 --- a/client/vnc/server/input_x11.go +++ b/client/vnc/server/input_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/metrics_conn.go b/client/vnc/server/metrics_conn.go index 75749629a..275822cf8 100644 --- a/client/vnc/server/metrics_conn.go +++ b/client/vnc/server/metrics_conn.go @@ -3,6 +3,7 @@ package server import ( + "encoding/binary" "net" "sync" "sync/atomic" @@ -169,26 +170,20 @@ func (m *metricsConn) BusyFraction() float64 { return m.busyFraction } -// isFBUHeader reports whether the given Write payload is the 4-byte -// FramebufferUpdate header (message type 0, padding 0, rect-count high -// byte). Rect bodies are written separately by sendDirtyAndMoves, so the -// FBU/rect boundary lines up with Write boundaries. -func isFBUHeader(p []byte) bool { - return len(p) == 4 && p[0] == serverFramebufferUpdate +// startsFBU reports whether the Write payload begins a FramebufferUpdate +// message (message type byte 0). This holds both for the standalone 4-byte +// header that sendDirtyAndMoves writes before its rect bodies and for the +// single framed Write that sendFullUpdate / sendEmptyUpdate use to emit a +// whole FBU (header plus body) at once. Either way the FBU boundary lines +// up with this Write boundary. +func startsFBU(p []byte) bool { + return len(p) >= 1 && p[0] == serverFramebufferUpdate } func (m *metricsConn) Write(p []byte) (int, error) { - if isFBUHeader(p) { - if b := m.fbuBytes.Swap(0); b > 0 { - if b > m.maxFBUBytes.Load() { - m.maxFBUBytes.Store(b) - } - } - if r := m.fbuRects.Swap(0); r > 0 { - if r > m.maxFBURects.Load() { - m.maxFBURects.Store(r) - } - } + fbuStart := startsFBU(p) + if fbuStart { + m.flushFBUMax() m.fbus.Add(1) } @@ -197,28 +192,41 @@ func (m *metricsConn) Write(p []byte) (int, error) { m.writeNanos.Add(uint64(time.Since(t0).Nanoseconds())) m.bytesOut.Add(uint64(n)) m.writes.Add(1) - if !isFBUHeader(p) { - m.fbuBytes.Add(uint64(n)) - m.fbuRects.Add(1) + + m.fbuBytes.Add(uint64(n)) + if fbuStart { + // Rect count is carried in bytes 2:3 of the FBU header. A standalone + // header records it here; the rect bodies that follow only add bytes. + if len(p) >= 4 { + m.fbuRects.Add(uint64(binary.BigEndian.Uint16(p[2:4]))) + } } + if uint64(n) > m.largestPkt.Load() { m.largestPkt.Store(uint64(n)) } return n, err } +// flushFBUMax folds the bytes and rects accumulated for the FBU that just +// ended into the per-tick high-water marks, then resets the accumulators +// for the next FBU. +func (m *metricsConn) flushFBUMax() { + if b := m.fbuBytes.Swap(0); b > m.maxFBUBytes.Load() { + m.maxFBUBytes.Store(b) + } + if r := m.fbuRects.Swap(0); r > m.maxFBURects.Load() { + m.maxFBURects.Store(r) + } +} + func (m *metricsConn) Close() error { m.closeOnce.Do(func() { close(m.done) if m.recorder == nil { return } - if b := m.fbuBytes.Swap(0); b > m.maxFBUBytes.Load() { - m.maxFBUBytes.Store(b) - } - if r := m.fbuRects.Swap(0); r > m.maxFBURects.Load() { - m.maxFBURects.Store(r) - } + m.flushFBUMax() m.flushTick(true) }) return m.Conn.Close() diff --git a/client/vnc/server/server_test.go b/client/vnc/server/server_test.go index a820469e6..fa42ea81a 100644 --- a/client/vnc/server/server_test.go +++ b/client/vnc/server/server_test.go @@ -406,7 +406,7 @@ func TestGateApproval_Disabled_NoApproverCall(t *testing.T) { header := &connectionHeader{mode: ModeAttach} _, err := srv.gateApproval(conn, header) - allowed := err == nil + allowed := err == nil assert.True(t, allowed, "gate must pass through when requireApproval is false") assert.Equal(t, int32(0), app.calls.Load(), "approver must not be called when disabled") } @@ -475,7 +475,7 @@ func TestGateApproval_ApproverDenies(t *testing.T) { header := &connectionHeader{mode: ModeAttach} _, err := srv.gateApproval(conn, header) - allowed := err == nil + allowed := err == nil assert.False(t, allowed, "approver error %v must deny", tc.err) assert.Equal(t, int32(1), app.calls.Load()) }) @@ -493,7 +493,7 @@ func TestGateApproval_ApproverAccepts(t *testing.T) { header := &connectionHeader{mode: ModeAttach, username: "alice"} _, err := srv.gateApproval(conn, header) - allowed := err == nil + allowed := err == nil assert.True(t, allowed, "approver returning nil must let the gate pass") assert.Equal(t, int32(1), app.calls.Load()) assert.Equal(t, "alice", app.lastIn.Username, "header username must reach the approver") @@ -515,7 +515,7 @@ func TestGateApproval_PassesPubKeyHex(t *testing.T) { } header := &connectionHeader{mode: ModeAttach, clientStatic: pub} _, err := srv.gateApproval(conn, header) - allowed := err == nil + allowed := err == nil assert.True(t, allowed) assert.Equal(t, hex.EncodeToString(pub), app.lastIn.PeerPubKey) } diff --git a/client/vnc/server/server_x11.go b/client/vnc/server/server_x11.go index 6e6c53fcb..6c0b6b643 100644 --- a/client/vnc/server/server_x11.go +++ b/client/vnc/server/server_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index 8a5d9bd31..1d87947bb 100644 --- a/client/vnc/server/session.go +++ b/client/vnc/server/session.go @@ -20,6 +20,12 @@ const ( maxCutTextBytes = 1 << 20 // 1 MiB ) +// handshakeDeadline bounds the RFB handshake exchange (version, security, +// ClientInit). Without it an authenticated peer can park a connection +// between the connection-header deadlines and messageLoop's own deadline, +// pinning a connSem slot. +const handshakeDeadline = 10 * time.Second + const tileSize = 64 // pixels per tile for dirty-rect detection // fullFramePromoteNum/Den trigger full-frame encoding when the dirty area @@ -48,9 +54,12 @@ const ( ) type session struct { - conn net.Conn - capturer ScreenCapturer - injector InputInjector + conn net.Conn + capturer ScreenCapturer + injector InputInjector + // serverW and serverH are the current framebuffer dimensions. The + // encoder goroutine updates them on resize while the message loop reads + // them for pointer scaling, so both accesses are guarded by encMu. serverW int serverH int desktopName string @@ -200,6 +209,11 @@ func (s *session) serve() { } func (s *session) handshake() error { + if err := s.conn.SetDeadline(time.Now().Add(handshakeDeadline)); err != nil { + return fmt.Errorf("set handshake deadline: %w", err) + } + defer s.conn.SetDeadline(time.Time{}) //nolint:errcheck + // Send protocol version. if _, err := io.WriteString(s.conn, rfbProtocolVersion); err != nil { return fmt.Errorf("send version: %w", err) @@ -540,38 +554,6 @@ func (s *session) handleFBUpdateRequest() error { return nil } -// SendDesktopName pushes a DesktopName pseudo-encoded update to the -// client if it advertised support. Lets the client keep its window title -// in sync with the active session (e.g. username changes after login on -// a virtual session). -func (s *session) SendDesktopName(name string) error { - if s.viewOnly { - name = ViewOnlyDesktopNamePrefix + name - } - s.encMu.RLock() - supported := s.clientSupportsDesktopName - s.encMu.RUnlock() - if !supported { - s.desktopName = name - return nil - } - s.desktopName = name - header := make([]byte, 4) - header[0] = serverFramebufferUpdate - binary.BigEndian.PutUint16(header[2:4], 1) - - body := encodeDesktopNameBody(name) - s.writeMu.Lock() - defer s.writeMu.Unlock() - if _, err := s.conn.Write(header); err != nil { - return err - } - if _, err := s.conn.Write(body); err != nil { - return err - } - return nil -} - func (s *session) handleKeyEvent() error { var data [7]byte if _, err := io.ReadFull(s.conn, data[:]); err != nil { @@ -639,7 +621,10 @@ func (s *session) handlePointerEvent() error { s.lastPointerX = x s.lastPointerY = y s.pointerMu.Unlock() - s.injector.InjectPointer(mask, x, y, s.serverW, s.serverH) + s.encMu.RLock() + w, h := s.serverW, s.serverH + s.encMu.RUnlock() + s.injector.InjectPointer(mask, x, y, w, h) return nil } @@ -673,5 +658,8 @@ func (s *session) releaseStickyInput() { s.pointerMu.Lock() x, y := s.lastPointerX, s.lastPointerY s.pointerMu.Unlock() - s.injector.InjectPointer(0, x, y, s.serverW, s.serverH) + s.encMu.RLock() + w, h := s.serverW, s.serverH + s.encMu.RUnlock() + s.injector.InjectPointer(0, x, y, w, h) } diff --git a/client/vnc/server/session_encode.go b/client/vnc/server/session_encode.go index 529c3bae5..56d1b5ebe 100644 --- a/client/vnc/server/session_encode.go +++ b/client/vnc/server/session_encode.go @@ -257,8 +257,10 @@ func (s *session) handleResize() error { return nil } s.log.Debugf("framebuffer resized: %dx%d -> %dx%d", s.serverW, s.serverH, w, h) + s.encMu.Lock() s.serverW = w s.serverH = h + s.encMu.Unlock() // Drop the prev frame so the next encode produces a full update at // the new dimensions rather than diffing against a stale-sized buffer. s.prevFrame = nil @@ -405,7 +407,7 @@ func promoteToBoundingBox(rects [][4]int) ([][4]int, bool) { if bbox < bboxPromoteMinArea { return nil, false } - if dirty*100 < bbox*bboxPromoteDensityPct { + if int64(dirty)*100 < int64(bbox)*bboxPromoteDensityPct { return nil, false } return [][4]int{{x0, y0, w, h}}, true @@ -423,7 +425,7 @@ func (s *session) shouldPromoteToFullFrame(rects [][4]int) bool { for _, r := range rects { dirty += r[2] * r[3] } - return dirty*fullFramePromoteDen > s.serverW*s.serverH*fullFramePromoteNum + return int64(dirty)*fullFramePromoteDen > int64(s.serverW)*int64(s.serverH)*fullFramePromoteNum } // swapPrevCur makes the just-encoded frame the new prevFrame (for the next diff --git a/client/vnc/server/virtual_x11.go b/client/vnc/server/virtual_x11.go index f19dbdbf5..2068265fa 100644 --- a/client/vnc/server/virtual_x11.go +++ b/client/vnc/server/virtual_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/vnc/server/xauth_x11.go b/client/vnc/server/xauth_x11.go index a8fbae884..66855aa4c 100644 --- a/client/vnc/server/xauth_x11.go +++ b/client/vnc/server/xauth_x11.go @@ -1,4 +1,4 @@ -//go:build unix && !darwin && !ios && !android +//go:build (linux && !android) || freebsd package server diff --git a/client/wasm/cmd/main.go b/client/wasm/cmd/main.go index 1ebae635a..8187657f8 100644 --- a/client/wasm/cmd/main.go +++ b/client/wasm/cmd/main.go @@ -409,12 +409,13 @@ func createGenerateVNCSessionKeyMethod() js.Func { // createVNCProxyMethod creates the VNC proxy method for raw TCP-over-WebSocket bridging. // JS signature: createVNCProxy(hostname, port, mode?, username?, keySessionID?, sessionID?, width?, height?, peerPublicKey?) -// mode: "attach" (default) or "session" -// username: required when mode is "session" -// keySessionID: handle for the wasm-resident session keypair minted by netbirdGenerateVNCSessionKey -// sessionID: Windows session ID (0 = console/auto) -// width/height: requested viewport size for session mode (0 = server default) -// peerPublicKey: base64 X25519 static pubkey of the destination peer (required for auth) +// +// mode: "attach" (default) or "session" +// username: required when mode is "session" +// keySessionID: handle for the wasm-resident session keypair minted by netbirdGenerateVNCSessionKey +// sessionID: Windows session ID (0 = console/auto) +// width/height: requested viewport size for session mode (0 = server default) +// peerPublicKey: base64 X25519 static pubkey of the destination peer (required for auth) func createVNCProxyMethod(client *netbird.Client) js.Func { return js.FuncOf(func(_ js.Value, args []js.Value) any { params, err := parseVNCProxyArgs(args) diff --git a/client/wasm/internal/vnc/proxy.go b/client/wasm/internal/vnc/proxy.go index 09314f438..5df541edf 100644 --- a/client/wasm/internal/vnc/proxy.go +++ b/client/wasm/internal/vnc/proxy.go @@ -21,6 +21,12 @@ import ( var cryptoRandRead = crand.Read +// proxyIDCounter is process-unique across every createVNCProxy call so each +// proxy/connection registers a distinct global handler name. A per-proxy +// counter would restart at 1 for every new VNCProxy, letting a reconnect's +// cleanup delete the new proxy's handler. +var proxyIDCounter atomic.Uint64 + // vncIdentityMagic mirrors the server side in client/vnc/server/server.go. var vncIdentityMagic = []byte("NBV3") @@ -115,7 +121,7 @@ type vncNBClient interface { } type VNCProxy struct { - nbClient vncNBClient + nbClient vncNBClient activeConnections map[string]*vncConnection destinations map[string]vncDestination // pendingHandlers holds the js.Func for handleVNCWebSocket_ between @@ -123,19 +129,18 @@ type VNCProxy struct { // vncConnection for later release. pendingHandlers map[string]js.Func mu sync.Mutex - nextID atomic.Uint64 } type vncDestination struct { - address string - mode byte - username string - sessionPriv []byte - sessionPub []byte - sessionID uint32 - width uint16 - height uint16 - peerPubKey []byte + address string + mode byte + username string + sessionPriv []byte + sessionPub []byte + sessionID uint32 + width uint16 + height uint16 + peerPubKey []byte } type vncConnection struct { @@ -152,6 +157,10 @@ type vncConnection struct { wsHandlerFn js.Func onMessageFn js.Func onCloseFn js.Func + // writeQueue carries inbound WS payloads to a single writer goroutine so + // vncConn.Write calls stay serialized in arrival order. + writeQueue chan []byte + cleanupOnce sync.Once } // NewVNCProxy creates a new VNC proxy. @@ -253,7 +262,7 @@ func (p *VNCProxy) newProxyPromise(address, mode, username string, dest vncDesti go func() { defer executor.Release() - proxyID := fmt.Sprintf("vnc_proxy_%d", p.nextID.Add(1)) + proxyID := fmt.Sprintf("vnc_proxy_%d", proxyIDCounter.Add(1)) p.mu.Lock() if p.destinations == nil { @@ -309,6 +318,7 @@ func (p *VNCProxy) handleWebSocketConnection(ws js.Value, proxyID string) { ctx: ctx, cancel: cancel, wsHandlerFn: handlerFn, + writeQueue: make(chan []byte, 256), } p.mu.Lock() @@ -326,8 +336,7 @@ func (p *VNCProxy) setupWebSocketHandlers(ws js.Value, conn *vncConnection) { if len(args) < 1 { return nil } - data := args[0] - go p.handleWebSocketMessage(conn, data) + p.enqueueWebSocketMessage(conn, args[0]) return nil }) ws.Set("onGoMessage", conn.onMessageFn) @@ -340,7 +349,12 @@ func (p *VNCProxy) setupWebSocketHandlers(ws js.Value, conn *vncConnection) { ws.Set("onGoClose", conn.onCloseFn) } -func (p *VNCProxy) handleWebSocketMessage(conn *vncConnection, data js.Value) { +// enqueueWebSocketMessage copies an inbound WS payload into Go memory and +// hands it to the writer goroutine in arrival order. JS onmessage events are +// delivered single-threaded on the event loop, so copying here preserves +// stream order. When the queue is full the connection is torn down rather +// than dropping bytes, which would corrupt the RFB stream. +func (p *VNCProxy) enqueueWebSocketMessage(conn *vncConnection, data js.Value) { if !data.InstanceOf(js.Global().Get("Uint8Array")) { return } @@ -349,16 +363,30 @@ func (p *VNCProxy) handleWebSocketMessage(conn *vncConnection, data js.Value) { buf := make([]byte, length) js.CopyBytesToGo(buf, data) - conn.mu.Lock() - vncConn := conn.vncConn - conn.mu.Unlock() - - if vncConn == nil { - return + select { + case <-conn.ctx.Done(): + case conn.writeQueue <- buf: + default: + log.Debugf("VNC write queue full for %s; closing connection", conn.id) + conn.cancel() } +} - if _, err := vncConn.Write(buf); err != nil { - log.Debugf("write to VNC server: %v", err) +// writeQueueLoop drains the ordered write queue and performs the blocking +// vncConn.Write sequentially, serializing WS→TCP writes. It exits when the +// connection context is cancelled. +func (p *VNCProxy) writeQueueLoop(conn *vncConnection, vncConn net.Conn) { + for { + select { + case <-conn.ctx.Done(): + return + case buf := <-conn.writeQueue: + if _, err := vncConn.Write(buf); err != nil { + log.Debugf("write to VNC server: %v", err) + conn.cancel() + return + } + } } } @@ -394,9 +422,10 @@ func (p *VNCProxy) connectToVNC(conn *vncConnection) { return } - // WS→TCP is handled by the onGoMessage handler set in setupWebSocketHandlers, - // which writes directly to the VNC connection as data arrives from JS. - // Only the TCP→WS direction needs a read loop here. + // WS→TCP payloads are enqueued in arrival order by the onGoMessage handler + // and drained sequentially by a single writer goroutine, keeping the RFB + // stream ordered. The TCP→WS direction has its own read loop. + go p.writeQueueLoop(conn, vncConn) go p.forwardConnToWS(conn) <-conn.ctx.Done() @@ -573,33 +602,47 @@ func (p *VNCProxy) sendToWebSocket(conn *vncConnection, data []byte) { } func (p *VNCProxy) cleanupConnection(conn *vncConnection) { - log.Debugf("cleaning up VNC connection %s", conn.id) - conn.cancel() + conn.cleanupOnce.Do(func() { + log.Debugf("cleaning up VNC connection %s", conn.id) + conn.cancel() - conn.mu.Lock() - vncConn := conn.vncConn - conn.vncConn = nil - conn.mu.Unlock() + conn.mu.Lock() + vncConn := conn.vncConn + conn.vncConn = nil + conn.mu.Unlock() - if vncConn != nil { - if err := vncConn.Close(); err != nil { - log.Debugf("close VNC connection: %v", err) + if vncConn != nil { + if err := vncConn.Close(); err != nil { + log.Debugf("close VNC connection: %v", err) + } } - } - // Remove the global JS handler registered in CreateProxy. - globalName := fmt.Sprintf("handleVNCWebSocket_%s", conn.id) - js.Global().Delete(globalName) + // Remove the global JS handler registered in CreateProxy. + js.Global().Delete(fmt.Sprintf("handleVNCWebSocket_%s", conn.id)) - // Release all js.Func handles; js.FuncOf pins the Go closure and the - // allocations it captures until Release is called. - conn.wsHandlerFn.Release() - conn.onMessageFn.Release() - conn.onCloseFn.Release() + // Detach before releasing so a late WS event surfaces as a TypeError + // instead of calling a released js.Func and panicking the runtime. + if conn.wsHandlers.Truthy() { + conn.wsHandlers.Set("onGoMessage", js.Undefined()) + conn.wsHandlers.Set("onGoClose", js.Undefined()) + } - p.mu.Lock() - delete(p.activeConnections, conn.id) - delete(p.destinations, conn.id) - delete(p.pendingHandlers, conn.id) - p.mu.Unlock() + // wsHandlerFn is the zero js.Func when the pendingHandlers lookup + // missed on a second connect. + if conn.wsHandlerFn.Truthy() { + conn.wsHandlerFn.Release() + } + if conn.onMessageFn.Truthy() { + conn.onMessageFn.Release() + } + if conn.onCloseFn.Truthy() { + conn.onCloseFn.Release() + } + + p.mu.Lock() + delete(p.activeConnections, conn.id) + delete(p.destinations, conn.id) + delete(p.pendingHandlers, conn.id) + p.mu.Unlock() + }) } diff --git a/management/server/http/handlers/peers/temporary_access_permission_test.go b/management/server/http/handlers/peers/temporary_access_permission_test.go index 6a318c8d0..5bbe6592e 100644 --- a/management/server/http/handlers/peers/temporary_access_permission_test.go +++ b/management/server/http/handlers/peers/temporary_access_permission_test.go @@ -2,6 +2,7 @@ package peers import ( "bytes" + "context" "encoding/json" "net/http" "net/http/httptest" @@ -34,7 +35,7 @@ func TestCreateTemporaryAccess_RejectsCallerWithoutPeersCreate(t *testing.T) { // 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). + Return(false, context.Background(), nil). Times(1) h := &Handler{ @@ -74,11 +75,11 @@ func TestCreateTemporaryAccess_RejectsCallerWithoutPoliciesCreate(t *testing.T) permMgr.EXPECT(). ValidateUserPermissions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(modules.Peers), gomock.Eq(operations.Create)). - Return(true, nil). + Return(true, context.Background(), nil). Times(1) permMgr.EXPECT(). ValidateUserPermissions(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Eq(modules.Policies), gomock.Eq(operations.Create)). - Return(false, nil). + Return(false, context.Background(), nil). Times(1) h := &Handler{ diff --git a/management/server/peer/peer.go b/management/server/peer/peer.go index 8c3e9dc3d..e13defb51 100644 --- a/management/server/peer/peer.go +++ b/management/server/peer/peer.go @@ -138,7 +138,6 @@ type Flags struct { DisableIPv6 bool LazyConnectionEnabled bool - } // PeerSystemMeta is a metadata of a Peer machine system diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index c6ced2642..6e18a5c4b 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -2536,7 +2536,7 @@ func (s *SqlStore) getPolicyRules(ctx context.Context, policyIDs []string) ([]*t if len(policyIDs) == 0 { return nil, nil } - const query = `SELECT id, policy_id, name, description, enabled, action, destinations, destination_resource, sources, source_resource, bidirectional, protocol, ports, port_ranges, authorized_groups, authorized_user FROM policy_rules WHERE policy_id = ANY($1)` + const query = `SELECT id, policy_id, name, description, enabled, action, destinations, destination_resource, sources, source_resource, bidirectional, protocol, ports, port_ranges, authorized_groups, authorized_user, session_pub_key, session_display_name FROM policy_rules WHERE policy_id = ANY($1)` rows, err := s.pool.Query(ctx, query, policyIDs) if err != nil { return nil, err @@ -2545,8 +2545,8 @@ func (s *SqlStore) getPolicyRules(ctx context.Context, policyIDs []string) ([]*t var r types.PolicyRule var dest, destRes, sources, sourceRes, ports, portRanges, authorizedGroups []byte var enabled, bidirectional sql.NullBool - var authorizedUser sql.NullString - err := row.Scan(&r.ID, &r.PolicyID, &r.Name, &r.Description, &enabled, &r.Action, &dest, &destRes, &sources, &sourceRes, &bidirectional, &r.Protocol, &ports, &portRanges, &authorizedGroups, &authorizedUser) + var authorizedUser, sessionPubKey, sessionDisplayName sql.NullString + err := row.Scan(&r.ID, &r.PolicyID, &r.Name, &r.Description, &enabled, &r.Action, &dest, &destRes, &sources, &sourceRes, &bidirectional, &r.Protocol, &ports, &portRanges, &authorizedGroups, &authorizedUser, &sessionPubKey, &sessionDisplayName) if err == nil { if enabled.Valid { r.Enabled = enabled.Bool @@ -2578,6 +2578,12 @@ func (s *SqlStore) getPolicyRules(ctx context.Context, policyIDs []string) ([]*t if authorizedUser.Valid { r.AuthorizedUser = authorizedUser.String } + if sessionPubKey.Valid { + r.SessionPubKey = sessionPubKey.String + } + if sessionDisplayName.Valid { + r.SessionDisplayName = sessionDisplayName.String + } } return &r, err }) diff --git a/management/server/types/account.go b/management/server/types/account.go index f9228b54a..2831f8bf1 100644 --- a/management/server/types/account.go +++ b/management/server/types/account.go @@ -14,7 +14,6 @@ import ( "github.com/rs/xid" log "github.com/sirupsen/logrus" - auth "github.com/netbirdio/netbird/shared/sessionauth" nbdns "github.com/netbirdio/netbird/dns" proxydomain "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" @@ -29,6 +28,7 @@ import ( "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" "github.com/netbirdio/netbird/shared/management/status" + auth "github.com/netbirdio/netbird/shared/sessionauth" "github.com/netbirdio/netbird/version" ) @@ -170,7 +170,6 @@ func (a *Account) GetGroup(groupID string) *Group { return a.Groups[groupID] } - func (a *Account) addNetworksRoutingPeers( networkResourcesRoutes []*route.Route, peer *nbpeer.Peer, diff --git a/management/server/types/networkmap_components.go b/management/server/types/networkmap_components.go index 1182eaf40..99feb4754 100644 --- a/management/server/types/networkmap_components.go +++ b/management/server/types/networkmap_components.go @@ -9,13 +9,13 @@ import ( "strings" "time" - auth "github.com/netbirdio/netbird/shared/sessionauth" nbdns "github.com/netbirdio/netbird/dns" resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types" routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" + auth "github.com/netbirdio/netbird/shared/sessionauth" ) type NetworkMapComponents struct { @@ -109,7 +109,7 @@ func (c *NetworkMapComponents) Calculate(ctx context.Context) *NetworkMap { peerGroups := c.GetPeerGroups(targetPeerID) - connRes := c.getPeerConnectionResources(targetPeerID) + connRes := c.getPeerConnectionResources(ctx, targetPeerID) aclPeers := connRes.peers peersToConnect, expiredPeers := c.filterPeersByLoginExpiration(aclPeers) @@ -182,7 +182,7 @@ type peerConnectionResult struct { sshEnabled bool } -func (c *NetworkMapComponents) getPeerConnectionResources(targetPeerID string) peerConnectionResult { +func (c *NetworkMapComponents) getPeerConnectionResources(ctx context.Context, targetPeerID string) peerConnectionResult { targetPeer := c.GetPeerInfo(targetPeerID) if targetPeer == nil { return peerConnectionResult{} @@ -202,7 +202,7 @@ func (c *NetworkMapComponents) getPeerConnectionResources(targetPeerID string) p if !rule.Enabled { continue } - c.applyPolicyRule(rule, policy.SourcePostureChecks, targetPeer, targetPeerID, generateResources, state) + c.applyPolicyRule(ctx, rule, policy.SourcePostureChecks, targetPeer, targetPeerID, generateResources, state) } } @@ -218,6 +218,7 @@ func (c *NetworkMapComponents) getPeerConnectionResources(targetPeerID string) p } func (c *NetworkMapComponents) applyPolicyRule( + ctx context.Context, rule *PolicyRule, sourcePostureChecks []string, targetPeer *nbpeer.Peer, @@ -229,8 +230,12 @@ func (c *NetworkMapComponents) applyPolicyRule( destinationPeers, peerInDestinations := c.resolveRuleEndpoint(rule.DestinationResource, rule.Destinations, targetPeerID, nil) cb := ruleAuthCallbacks{ - collectSSHUsers: c.collectAuthorizedUsers, - collectVNCUsers: c.collectAuthorizedUsers, + collectSSHUsers: func(r *PolicyRule, t map[string]map[string]struct{}) { + c.collectAuthorizedUsers(ctx, r, t) + }, + collectVNCUsers: func(r *PolicyRule, t map[string]map[string]struct{}) { + c.collectAuthorizedUsers(ctx, r, t) + }, getAllowedUserIDs: c.getAllowedUserIDs, } applyResolvedRuleToState(rule, sourcePeers, destinationPeers, peerInSources, peerInDestinations, targetPeer.SSHEnabled, generateResources, cb, state) @@ -249,10 +254,10 @@ func (c *NetworkMapComponents) resolveRuleEndpoint( } // collectAuthorizedUsers populates the target map with authorized user mappings from the rule. -func (c *NetworkMapComponents) collectAuthorizedUsers(rule *PolicyRule, target map[string]map[string]struct{}) { +func (c *NetworkMapComponents) collectAuthorizedUsers(ctx context.Context, rule *PolicyRule, target map[string]map[string]struct{}) { switch { case len(rule.AuthorizedGroups) > 0: - mergeAuthorizedGroupUsers(context.Background(), rule.AuthorizedGroups, c.GroupIDToUserIDs, target) + mergeAuthorizedGroupUsers(ctx, rule.AuthorizedGroups, c.GroupIDToUserIDs, target) case rule.AuthorizedUser != "": ensureWildcardUser(target, rule.AuthorizedUser) default: diff --git a/management/server/types/policy_authorized_users.go b/management/server/types/policy_authorized_users.go index 486446f95..46bce625d 100644 --- a/management/server/types/policy_authorized_users.go +++ b/management/server/types/policy_authorized_users.go @@ -6,8 +6,8 @@ import ( log "github.com/sirupsen/logrus" - auth "github.com/netbirdio/netbird/shared/sessionauth" nbpeer "github.com/netbirdio/netbird/management/server/peer" + auth "github.com/netbirdio/netbird/shared/sessionauth" ) // peerConnResolveState carries the in-progress maps mutated by per-rule diff --git a/management/server/types/policy_authorized_users_security_test.go b/management/server/types/policy_authorized_users_security_test.go index b040c9d2f..28b1dc072 100644 --- a/management/server/types/policy_authorized_users_security_test.go +++ b/management/server/types/policy_authorized_users_security_test.go @@ -1,6 +1,10 @@ package types -import "testing" +import ( + "testing" + + nbpeer "github.com/netbirdio/netbird/management/server/peer" +) // TestHandleVNCRule_BidirectionalDistributesPubkeyToSourcePeer covers the // latent bug where a bidirectional VNC rule used to drop the @@ -83,3 +87,68 @@ func TestHandleVNCRule_DestinationAlwaysGetsPubkey(t *testing.T) { t.Fatalf("expected 1 session pubkey for destination peer, got %d", len(state.vncSessionPubKeys)) } } + +// TestApplyResolvedRule_BidirectionalSSHEnablesSourcePeer locks the +// bidirectional widening for netbird-ssh rules: a peer that appears only +// in the rule's sources of a bidirectional SSH rule must get SSH enabled +// and its authorized users collected, because the rule grants access in +// both directions. A unidirectional rule must not do this for a +// source-only peer. +func TestApplyResolvedRule_BidirectionalSSHEnablesSourcePeer(t *testing.T) { + collected := false + cb := ruleAuthCallbacks{ + collectSSHUsers: func(_ *PolicyRule, target map[string]map[string]struct{}) { + collected = true + target["local"] = map[string]struct{}{"user1": {}} + }, + } + rule := &PolicyRule{ + Protocol: PolicyRuleProtocolNetbirdSSH, + Bidirectional: true, + } + state := &peerConnResolveState{ + authorizedUsers: make(map[string]map[string]struct{}), + vncAuthorizedUsers: make(map[string]map[string]struct{}), + } + + applyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, false, func(*PolicyRule, []*nbpeer.Peer, int) {}, cb, state) + + if !state.sshEnabled { + t.Fatal("expected SSH enabled on source-side peer of bidirectional SSH rule") + } + if !collected { + t.Fatal("expected authorized users collected on source-side peer of bidirectional SSH rule") + } + if _, ok := state.authorizedUsers["local"]; !ok { + t.Fatal("expected authorized users map populated for source-side peer") + } +} + +// TestApplyResolvedRule_UnidirectionalSSHSkipsSourcePeer is the negative +// counterpart: a unidirectional SSH rule must not enable SSH for a peer +// that appears only in sources. +func TestApplyResolvedRule_UnidirectionalSSHSkipsSourcePeer(t *testing.T) { + collected := false + cb := ruleAuthCallbacks{ + collectSSHUsers: func(_ *PolicyRule, _ map[string]map[string]struct{}) { + collected = true + }, + } + rule := &PolicyRule{ + Protocol: PolicyRuleProtocolNetbirdSSH, + Bidirectional: false, + } + state := &peerConnResolveState{ + authorizedUsers: make(map[string]map[string]struct{}), + vncAuthorizedUsers: make(map[string]map[string]struct{}), + } + + applyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, false, func(*PolicyRule, []*nbpeer.Peer, int) {}, cb, state) + + if state.sshEnabled { + t.Fatal("expected SSH NOT enabled on source-only peer of unidirectional SSH rule") + } + if collected { + t.Fatal("expected NO authorized users collected on source-only peer of unidirectional SSH rule") + } +} diff --git a/shared/sessionauth/auth.go b/shared/sessionauth/auth.go index e438d8831..9628a6b0d 100644 --- a/shared/sessionauth/auth.go +++ b/shared/sessionauth/auth.go @@ -20,11 +20,11 @@ const ( ) var ( - ErrEmptyUserID = errors.New("JWT user ID is empty") - ErrUserNotAuthorized = errors.New("user is not authorized to access this peer") - ErrNoMachineUserMapping = errors.New("no authorization mapping for OS user") - ErrUserNotMappedToOSUser = errors.New("user is not authorized to login as OS user") - ErrSessionKeyNotKnown = errors.New("session pubkey not registered") + ErrEmptyUserID = errors.New("JWT user ID is empty") + ErrUserNotAuthorized = errors.New("user is not authorized to access this peer") + ErrNoMachineUserMapping = errors.New("no authorization mapping for OS user") + ErrUserNotMappedToOSUser = errors.New("user is not authorized to login as OS user") + ErrSessionKeyNotKnown = errors.New("session pubkey not registered") ) // Authorizer handles SSH fine-grained access control authorization @@ -83,8 +83,8 @@ type SessionPubKey struct { // NewAuthorizer creates a new SSH authorizer with empty configuration func NewAuthorizer() *Authorizer { a := &Authorizer{ - userIDClaim: DefaultUserIDClaim, - machineUsers: make(map[string][]uint32), + userIDClaim: DefaultUserIDClaim, + machineUsers: make(map[string][]uint32), sessionPubKeys: make(map[[sessionPubKeyLen]byte]sshuserhash.UserIDHash), sessionDisplayNames: make(map[[sessionPubKeyLen]byte]string), } diff --git a/util/capture/text.go b/util/capture/text.go index 95068136a..6ffdebef0 100644 --- a/util/capture/text.go +++ b/util/capture/text.go @@ -466,19 +466,19 @@ func isWellKnownVNCPort(p uint16) bool { // message-type recognitions fire only when length matches the fixed // size for that type, to avoid mis-tagging Noise handshake bytes. func annotateVNCClientToServer(p []byte) string { - if len(p) >= 10 && (p[0] == 0 || p[0] == 1) { - userLen := int(p[1]) - // width and height are uint16 fields the dashboard often leaves - // zero (default). A header without an OS user has total length - // 10; with one, 10+userLen. - if 10+userLen <= len(p) { + if len(p) >= 11 && (p[0] == 0 || p[0] == 1) { + // Connection header layout: mode(1) + u16 BE username length + + // username(N), then sessionID(4) + width(2) + height(2). Prefix is + // 3+N, full header 11+N. + userLen := int(binary.BigEndian.Uint16(p[1:3])) + if 11+userLen <= len(p) { mode := "attach" if p[0] == 1 { mode = "session" } tag := fmt.Sprintf("connect mode=%s", mode) - if userLen > 0 { - tag += fmt.Sprintf(" user(%d)", userLen) + if userLen > 0 && 3+userLen <= len(p) { + tag += fmt.Sprintf(" user(%s)", p[3:3+userLen]) } return tag } @@ -528,9 +528,11 @@ func annotateVNCServerToClient(p []byte) string { // matchRFBSecurityFailure recognises the RFB 3.8 security-result body the // server sends when authentication or session setup fails. Format: -// byte 0 : 0x00 (security types count = 0 = failure) -// bytes 1-4: uint32 reason length -// bytes 5+: reason text +// +// byte 0 : 0x00 (security types count = 0 = failure) +// bytes 1-4: uint32 reason length +// bytes 5+: reason text +// // Returns the reason text and ok=true when the length self-checks. func matchRFBSecurityFailure(p []byte) (string, bool) { if len(p) < 5 || p[0] != 0 {