From 921aa2b543f83c1568adb28b98b3f67bcd03b48c Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 22 Sep 2026 19:34:11 +0200 Subject: [PATCH] Refuse ambiguous X11 display selection, harden the xauth temp file, and reset the service-agent latch on restart --- client/server/server.go | 12 +- client/vnc/server/capture_x11.go | 151 +++++++++++++----- client/vnc/server/capture_x11_select_test.go | 20 ++- client/vnc/server/server.go | 1 + client/vnc/server/service_agent.go | 11 ++ client/vnc/server/xauth_x11.go | 30 +++- .../network_map_db/sqlite/policy_test.go | 32 ++++ 7 files changed, 212 insertions(+), 45 deletions(-) create mode 100644 management/internals/network_map_db/sqlite/policy_test.go diff --git a/client/server/server.go b/client/server/server.go index adec18ff7..5ca3748db 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -1178,8 +1178,15 @@ func (s *Server) storedLoginConfig(activeProf *profilemanager.ActiveProfileState return s.storedProfileConfig(handle, username) } -// storedConfigAtPath reads a profile config file, yielding nil when it does not -// exist yet. +// storedConfigAtPath reads a profile config file with the MDM policy applied on +// top, yielding nil when the file does not exist yet. +// +// The overlay matters because the privilege gates are this function's only +// consumers, and they have to reason about the config the engine will actually +// run. An MDM policy that enables a remote-access server leaves the on-disk +// value off, so without it the guards read a host publishing its console as one +// running nothing, and let an unprivileged caller repoint the management +// identity that decides who may connect to it. func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) { if _, err := os.Stat(path); err != nil { if os.IsNotExist(err) { @@ -1192,6 +1199,7 @@ func (s *Server) storedConfigAtPath(path string) (*profilemanager.Config, error) if err != nil { return nil, fmt.Errorf("read profile config: %w", err) } + cfg.ApplyMDMPolicy(s.mdmLoader.Load()) return cfg, nil } diff --git a/client/vnc/server/capture_x11.go b/client/vnc/server/capture_x11.go index 43f83a73c..866326d94 100644 --- a/client/vnc/server/capture_x11.go +++ b/client/vnc/server/capture_x11.go @@ -8,6 +8,7 @@ import ( "math" "os" "os/exec" + "slices" "strconv" "strings" "sync" @@ -142,52 +143,88 @@ func detectX11FromProc() x11Detection { return x11Detected } -// pickXorgCandidate chooses which X server to attach to. -// -// The one on the active VT wins outright. Failing that the choice has to be -// unambiguous, because guessing wrong means handing a remote user another local -// user's screen and keyboard: a single candidate is taken, and anything else is -// refused. The lowest display number only breaks ties among servers that all -// record no VT, where there is no active-session signal to go on at all. +// pickXorgCandidate chooses which X server to attach to. Guessing wrong hands a +// remote user another local user's screen and keyboard, so an unresolvable +// choice is refused rather than approximated. func pickXorgCandidate(candidates []xorgCandidate, activeVT int) (xorgCandidate, x11Detection) { if len(candidates) == 0 { return xorgCandidate{}, x11NotFound } if activeVT > 0 { - for _, c := range candidates { - if c.vt == activeVT { - return c, x11Detected - } + return pickForActiveVT(candidates, activeVT) + } + return pickWithoutActiveVT(candidates) +} + +// pickForActiveVT chooses when the console's VT is known. The server on that VT +// wins outright. Otherwise every candidate that records a VT serves some seat +// that is demonstrably not the console's, which makes it a session this +// connection has no business showing — however few of them there are. Only a +// server recording no VT (Xvfb, Xwayland, a nested server) stays eligible. +func pickForActiveVT(candidates []xorgCandidate, activeVT int) (xorgCandidate, x11Detection) { + for _, c := range candidates { + if c.vt == activeVT { + return c, x11Detected } } + + vtless := candidatesWithoutVT(candidates) + if len(vtless) == 0 { + log.Warnf("found %d X server(s), none serving the active VT (%s); not attaching to any", + len(candidates), describeActiveVT(activeVT)) + return xorgCandidate{}, x11Ambiguous + } + best := lowestDisplay(vtless) + if len(vtless) > 1 { + log.Warnf("found %d VT-less X servers and none serving the active VT (%s); attaching to the lowest display %s", + len(vtless), describeActiveVT(activeVT), best.display) + } + return best, x11Detected +} + +// pickWithoutActiveVT chooses when there is no active-VT signal at all: no +// sysfs, a seat with no VT, FreeBSD. A lone X server is taken as the host's +// only one. Among several there is nothing to separate the console's from +// another seat's, so the VT-less ones are preferred and the lowest display +// breaks the tie. +func pickWithoutActiveVT(candidates []xorgCandidate) (xorgCandidate, x11Detection) { if len(candidates) == 1 { return candidates[0], x11Detected } - // Several servers and no way to tell which is on the console. One that - // records a VT is on some seat other than the active one, so it is a - // session this connection has no business showing. + vtless := candidatesWithoutVT(candidates) + if len(vtless) == 0 { + log.Warnf("found %d X servers on virtual terminals and no active-VT signal; not attaching to any", + len(candidates)) + return xorgCandidate{}, x11Ambiguous + } + best := lowestDisplay(vtless) + log.Warnf("found %d X servers and no active-VT signal; attaching to the lowest VT-less display %s", + len(candidates), best.display) + return best, x11Detected +} + +// candidatesWithoutVT returns the candidates that record no virtual terminal. +func candidatesWithoutVT(candidates []xorgCandidate) []xorgCandidate { var vtless []xorgCandidate for _, c := range candidates { if c.vt < 0 { vtless = append(vtless, c) } } - if len(vtless) == 0 { - log.Warnf("found %d X servers, all on virtual terminals and none of them the active one (%s); not attaching to any", - len(candidates), describeActiveVT(activeVT)) - return xorgCandidate{}, x11Ambiguous - } + return vtless +} - best := vtless[0] - for _, c := range vtless[1:] { +// lowestDisplay returns the candidate with the lowest display number. candidates +// must not be empty. +func lowestDisplay(candidates []xorgCandidate) xorgCandidate { + best := candidates[0] + for _, c := range candidates[1:] { if displayNumber(c.display) < displayNumber(best.display) { best = c } } - log.Warnf("found %d X servers and none on the active VT (%s); attaching to the lowest VT-less display %s", - len(candidates), describeActiveVT(activeVT), best.display) - return best, x11Detected + return best } // describeActiveVT renders the active VT for a log line, keeping "we could not @@ -242,17 +279,20 @@ func displayNumber(display string) int { return n } -// detectX11FromSockets checks /tmp/.X11-unix/ for X sockets and uses ps -// to find the auth file. Works on FreeBSD and other systems without /proc. +// detectX11FromSockets checks /tmp/.X11-unix/ for X sockets and uses ps to find +// the auth file. Works on FreeBSD and other systems without /proc. +// +// A socket carries no hint of which seat it serves, so the display is only used +// when exactly one exists. Picking among several would attach the VNC session to +// an arbitrary user's screen and route the remote user's input there, and this +// path runs precisely where the /proc VT evidence is unavailable. func detectX11FromSockets() bool { entries, err := os.ReadDir(x11SocketDir) if err != nil { return false } - // Pick the lowest numeric display rather than the lexically first - // entry, so X10 doesn't win over X2. - minDisplay := -1 + var displays []int for _, e := range entries { name := e.Name() if len(name) < 2 || name[0] != 'X' { @@ -262,16 +302,20 @@ func detectX11FromSockets() bool { if err != nil { continue } - if minDisplay < 0 || n < minDisplay { - minDisplay = n - } + displays = append(displays, n) } - if minDisplay < 0 { + if len(displays) == 0 { return false } - display := ":" + strconv.Itoa(minDisplay) + if len(displays) > 1 { + log.Warnf("found %d X sockets in %s and no way to tell which serves the console; not attaching to any", + len(displays), x11SocketDir) + return false + } + + display := ":" + strconv.Itoa(displays[0]) os.Setenv(envDisplay, display) - auth := findXorgAuthFromPS() + auth := findXorgAuthFromPS(display) if auth != "" { os.Setenv(envXAuthority, auth) log.Infof("auto-detected DISPLAY=%s (from socket) XAUTHORITY=%s (from ps)", display, auth) @@ -281,8 +325,11 @@ func detectX11FromSockets() bool { return true } -// findXorgAuthFromPS runs ps to find Xorg and extract its -auth argument. -func findXorgAuthFromPS() string { +// findXorgAuthFromPS runs ps to find the X server serving display and extracts +// its -auth argument. The display has to match: on a host running more than one +// X server the first ps hit is not necessarily the one being attached to, and +// handing that server's cookie to a different display fails the connection. +func findXorgAuthFromPS(display string) string { out, err := exec.Command("ps", "auxww").Output() if err != nil { return "" @@ -292,6 +339,9 @@ func findXorgAuthFromPS() string { continue } fields := strings.Fields(line) + if !slices.Contains(fields, display) { + continue + } for i, f := range fields { if f == "-auth" && i+1 < len(fields) { return fields[i+1] @@ -385,6 +435,11 @@ func NewX11Capturer(display, cookieHex string) (*X11Capturer, error) { } screen := setup.Roots[0] + if err := checkPixmapFormat(setup, screen.RootDepth); err != nil { + conn.Close() + return nil, err + } + c := &X11Capturer{ conn: conn, screen: &screen, @@ -400,6 +455,28 @@ func NewX11Capturer(display, cookieHex string) (*X11Capturer, error) { return c, nil } +// checkPixmapFormat rejects a screen whose pixels this capturer cannot decode. +// GetImage returns ZPixmap data in the server's pixmap format for the screen's +// depth, and both the SHM and GetImage paths read it as 8-bit-per-channel BGRA. +// A 16-bpp screen, a packed 24-bpp one, or a 30-bit deep-colour one would +// otherwise pass startup and fail on every frame instead. +func checkPixmapFormat(setup *xproto.SetupInfo, depth byte) error { + if depth != 24 && depth != 32 { + return fmt.Errorf("unsupported X11 root depth %d, need 24 or 32", depth) + } + for _, f := range setup.PixmapFormats { + if f.Depth != depth { + continue + } + if f.BitsPerPixel != 32 { + return fmt.Errorf("unsupported X11 pixmap format for depth %d: %d bits per pixel, need 32", + depth, f.BitsPerPixel) + } + return nil + } + return fmt.Errorf("no X11 pixmap format for root depth %d", depth) +} + // initSHM is implemented in capture_x11_shm_linux.go (requires SysV SHM). // On platforms without SysV SHM (FreeBSD), a stub returns an error and // the capturer falls back to GetImage. diff --git a/client/vnc/server/capture_x11_select_test.go b/client/vnc/server/capture_x11_select_test.go index b9cd183f6..2b8ed8625 100644 --- a/client/vnc/server/capture_x11_select_test.go +++ b/client/vnc/server/capture_x11_select_test.go @@ -26,9 +26,27 @@ func TestPickXorgCandidate(t *testing.T) { wantOutcome: x11NotFound, }, { - name: "single server is used whatever its VT", + // The console is on tty2 and the only X server serves tty9, so it + // belongs to another seat. Being the sole candidate does not make + // it the right one to hand a remote user. + name: "a lone server on an inactive VT is refused", candidates: []xorgCandidate{{display: ":3", vt: 9}}, activeVT: 2, + wantOutcome: x11Ambiguous, + }, + { + name: "a lone VT-less server is used when the active VT matches nothing", + candidates: []xorgCandidate{{display: ":3", vt: -1}}, + activeVT: 2, + want: ":3", + wantOutcome: x11Detected, + }, + { + // No active-VT signal at all, so a single server is the host's only + // one and there is no other seat it could belong to. + name: "a lone server is used when the active VT is unknown", + candidates: []xorgCandidate{{display: ":3", vt: 9}}, + activeVT: -1, want: ":3", wantOutcome: x11Detected, }, diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index a71b7e82d..59868869f 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -789,6 +789,7 @@ func (s *Server) Start(ctx context.Context, addr netip.AddrPort, network netip.P s.stopping = false s.handlersDrained = nil s.handlersMu.Unlock() + s.resetServiceAgent() s.ctx, s.cancel = context.WithCancel(ctx) s.vmgr = s.platformSessionManager() diff --git a/client/vnc/server/service_agent.go b/client/vnc/server/service_agent.go index f4715779b..66f397bf0 100644 --- a/client/vnc/server/service_agent.go +++ b/client/vnc/server/service_agent.go @@ -16,6 +16,17 @@ type sessionAgent interface { Release() } +// resetServiceAgent reopens the latch stopServiceAgent closed, so a restarted +// server can build a manager again. Without it every accept loop of the new +// lifecycle keeps getting a nil agent and service mode stays dead for the rest +// of the process. Owned by Start, mirroring the other stop-time latches it +// clears. +func (s *Server) resetServiceAgent() { + s.serviceAgentMu.Lock() + defer s.serviceAgentMu.Unlock() + s.serviceAgentStopped = false +} + // stopServiceAgent tears down the shared manager, if one was ever built, and // latches the server so a still-draining accept loop cannot build another. // Owned by Stop rather than by an accept loop: the loops share the manager, so diff --git a/client/vnc/server/xauth_x11.go b/client/vnc/server/xauth_x11.go index 79a71fa6d..aca3dec10 100644 --- a/client/vnc/server/xauth_x11.go +++ b/client/vnc/server/xauth_x11.go @@ -65,18 +65,38 @@ func writeXAuthFile(path, hostname, display string, cookie []byte, uid, gid uint appendField([]byte(xauthMITMagic)) appendField(cookie) - tmp := path + ".tmp" - if err := os.WriteFile(tmp, buf, 0600); err != nil { + // The runtime dir is shared and writable by other local accounts, so a + // fixed temp name is something an occupant can pre-create as a symlink: + // the daemon would then write this cookie wherever it points and chown the + // target away. CreateTemp opens O_EXCL under an unpredictable name, which + // fails rather than follows, and the mode and owner are set through the + // descriptor so no path is resolved a second time. + f, err := os.CreateTemp(dir, ".Xauthority-*") + if err != nil { + return fmt.Errorf("create xauth tmp: %w", err) + } + tmp := f.Name() + defer func() { + if tmp != "" { + _ = os.Remove(tmp) + } + }() + + if _, err := f.Write(buf); err != nil { + f.Close() return fmt.Errorf("write xauth tmp: %w", err) } - if err := os.Chown(tmp, int(uid), int(gid)); err != nil { - _ = os.Remove(tmp) + if err := f.Chown(int(uid), int(gid)); err != nil { + f.Close() return fmt.Errorf("chown xauth tmp: %w", err) } + if err := f.Close(); err != nil { + return fmt.Errorf("close xauth tmp: %w", err) + } if err := os.Rename(tmp, path); err != nil { - _ = os.Remove(tmp) return fmt.Errorf("rename xauth: %w", err) } + tmp = "" return nil } diff --git a/management/internals/network_map_db/sqlite/policy_test.go b/management/internals/network_map_db/sqlite/policy_test.go new file mode 100644 index 000000000..5a60be5b8 --- /dev/null +++ b/management/internals/network_map_db/sqlite/policy_test.go @@ -0,0 +1,32 @@ +package networkmap_sqlite_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + networkmap_sqlite "github.com/netbirdio/netbird/management/internals/network_map_db/sqlite" + "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/management/server/types" +) + +// GetPoliciesQuery names its columns explicitly, while the schema they live in +// comes from AutoMigrate over types.PolicyRule. Nothing makes the compiler +// relate the two, so a column added to the query without a matching model field +// only fails at runtime, on every policy read. Run the real query against a +// real migrated store. +func TestGetPolicies_QueryMatchesMigratedSchema(t *testing.T) { + dir := t.TempDir() + t.Setenv("NETBIRD_STORE_ENGINE", string(types.SqliteStoreEngine)) + + _, cleanup, err := store.NewTestStoreFromSQL(context.Background(), "", dir) + require.NoError(t, err) + t.Cleanup(cleanup) + + conn, err := networkmap_sqlite.NewSqliteStore("store.db", dir) + require.NoError(t, err) + + _, _, _, err = conn.UsingConn().GetPolicies(context.Background(), "nonexistent-account") + require.NoError(t, err, "every column GetPoliciesQuery selects must exist in the migrated schema") +}