From d196b23de673bfd34157e9c21dbd38cff35438f9 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Sat, 29 Aug 2026 10:29:17 +0200 Subject: [PATCH] Stop narrowing the shared runtime dir, close the injector on stop, allowlist VNC metrics --- client/internal/engine_vnc_console_freebsd.go | 20 +++-- client/internal/metrics/infra/ingest/main.go | 20 +++++ client/vnc/server/server.go | 7 ++ client/vnc/server/xauth_x11.go | 33 ++++++++- client/vnc/server/xauth_x11_test.go | 73 +++++++++++++++++++ 5 files changed, 138 insertions(+), 15 deletions(-) create mode 100644 client/vnc/server/xauth_x11_test.go diff --git a/client/internal/engine_vnc_console_freebsd.go b/client/internal/engine_vnc_console_freebsd.go index 0d6b3a687..3acf5736a 100644 --- a/client/internal/engine_vnc_console_freebsd.go +++ b/client/internal/engine_vnc_console_freebsd.go @@ -10,11 +10,13 @@ import ( vncserver "github.com/netbirdio/netbird/client/vnc/server" ) -// newConsoleVNC builds the FreeBSD console fallback: vt(4) framebuffer -// for capture, /dev/uinput for input. The uinput device requires the -// `uinput` kernel module (`kldload uinput`); without it, input init -// fails and we drop to a stub injector so the user still gets a -// view-only screen mirror. +// newConsoleVNC builds the FreeBSD console fallback: the vt(4) framebuffer for +// capture, and no input. +// +// Input injection is not implemented on FreeBSD: the uinput injector is a +// Linux-only implementation built on UI_DEV_CREATE and friends, so this backend +// mirrors the console read-only. It is offered anyway because a view-only +// console is still worth more than nothing on a box with no X server. func newConsoleVNC() (vncserver.ScreenCapturer, vncserver.InputInjector, error) { poller := vncserver.NewFBPoller("") w, h := poller.Width(), poller.Height() @@ -22,10 +24,6 @@ func newConsoleVNC() (vncserver.ScreenCapturer, vncserver.InputInjector, error) poller.Close() return nil, nil, fmt.Errorf("vt framebuffer init failed (vt may not allow mmap on this driver)") } - if inj, err := vncserver.NewUInputInjector(w, h); err == nil { - return poller, inj, nil - } else { - log.Infof("VNC console: uinput unavailable (%v); view-only mode. Run `kldload uinput` to enable input.", err) - return poller, &vncserver.StubInputInjector{}, nil - } + log.Info("VNC console: FreeBSD has no input backend, serving the console view-only") + return poller, &vncserver.StubInputInjector{}, nil } diff --git a/client/internal/metrics/infra/ingest/main.go b/client/internal/metrics/infra/ingest/main.go index 91405b85f..100c70011 100644 --- a/client/internal/metrics/infra/ingest/main.go +++ b/client/internal/metrics/infra/ingest/main.go @@ -85,6 +85,26 @@ var allowedMeasurements = map[string]measurementSpec{ "peer_id": true, }, }, + // Emitted per VNC session tick by influxDBMetrics.RecordVNCSessionTick. + "netbird_vnc_traffic": { + allowedFields: map[string]bool{ + "period_seconds": true, + "bytes_out": true, + "writes": true, + "fbus": true, + "max_fbu_bytes": true, + "max_fbu_rects": true, + "max_write_bytes": true, + "write_time_seconds": true, + }, + allowedTags: map[string]bool{ + "deployment_type": true, + "version": true, + "os": true, + "arch": true, + "peer_id": true, + }, + }, } func main() { diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index b68ee0e0c..8aef5e9b3 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -761,6 +761,13 @@ func (s *Server) Stop() error { if c, ok := s.capturer.(interface{ Close() }); ok { c.Close() } + // The injector owns OS resources of its own: the uinput backend holds a + // /dev/uinput descriptor and a registered virtual device, the X11 one an + // X connection. Leaving them open leaks one of each every time the VNC + // server is stopped and started again. + if i, ok := s.injector.(interface{ Close() }); ok { + i.Close() + } if listenerErr != nil { return fmt.Errorf("close VNC listener: %w", listenerErr) diff --git a/client/vnc/server/xauth_x11.go b/client/vnc/server/xauth_x11.go index 0914ef8fa..79a71fa6d 100644 --- a/client/vnc/server/xauth_x11.go +++ b/client/vnc/server/xauth_x11.go @@ -81,8 +81,15 @@ func writeXAuthFile(path, hostname, display string, cookie []byte, uid, gid uint } // ensureTraversable walks up from dir to configs.RuntimeDir (inclusive) and -// sets mode 0711 on each component. A dir outside the runtime dir is refused -// before anything is chmodded, so it never touches /var/run or /run. +// makes each component traversable by the session's user, so the X server can +// reach its Xauthority file. A dir outside the runtime dir is refused before +// anything is changed, so it never touches /var/run or /run. +// +// Only the execute bits are added, never a whole mode. The runtime dir is +// shared: the daemon advertises its socket there and unprivileged CLI and UI +// clients list it to find one, so setting it to 0711 would take away the read +// bit they need and break socket discovery. Execute alone grants traversal +// without exposing a listing. func ensureTraversable(dir string) error { root := filepath.Clean(configs.RuntimeDir) if root == "" { @@ -93,8 +100,8 @@ func ensureTraversable(dir string) error { return fmt.Errorf("xauth dir %s is outside the runtime dir %s", cur, root) } for { - if err := os.Chmod(cur, 0711); err != nil { - return fmt.Errorf("chmod %s: %w", cur, err) + if err := addTraversalBits(cur); err != nil { + return err } if cur == root { return nil @@ -107,6 +114,24 @@ func ensureTraversable(dir string) error { } } +// addTraversalBits ORs group and other execute onto dir's mode, leaving every +// other bit as it was. A directory that is already traversable is not touched. +func addTraversalBits(dir string) error { + info, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("stat %s: %w", dir, err) + } + const traversal = 0o011 + mode := info.Mode().Perm() + if mode&traversal == traversal { + return nil + } + if err := os.Chmod(dir, mode|traversal); err != nil { + return fmt.Errorf("chmod %s: %w", dir, err) + } + return nil +} + // dialXUnixWithCookie opens an xgb connection to display over AF_UNIX, // authenticating with the supplied hex cookie instead of XAUTHORITY env. func dialXUnixWithCookie(display, cookieHex string) (*xgb.Conn, error) { diff --git a/client/vnc/server/xauth_x11_test.go b/client/vnc/server/xauth_x11_test.go new file mode 100644 index 000000000..a707e2bfc --- /dev/null +++ b/client/vnc/server/xauth_x11_test.go @@ -0,0 +1,73 @@ +//go:build (linux && !android) || freebsd + +package server + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/configs" +) + +// withRuntimeDir points configs.RuntimeDir at a temp tree for the test. +func withRuntimeDir(t *testing.T) string { + t.Helper() + root := t.TempDir() + prev := configs.RuntimeDir + configs.RuntimeDir = root + t.Cleanup(func() { configs.RuntimeDir = prev }) + return root +} + +func permOf(t *testing.T, dir string) os.FileMode { + t.Helper() + info, err := os.Stat(dir) + require.NoError(t, err) + return info.Mode().Perm() +} + +// The runtime dir is shared: the daemon advertises its socket there and +// unprivileged clients list it to find one. Making the xauth dir traversable +// must not cost the read bit that listing needs. +func TestEnsureTraversableKeepsTheRuntimeDirReadable(t *testing.T) { + root := withRuntimeDir(t) + require.NoError(t, os.Chmod(root, 0o755)) + + sub := filepath.Join(root, vncXAuthSubdir) + require.NoError(t, os.Mkdir(sub, 0o700)) + + require.NoError(t, ensureTraversable(sub)) + + assert.Equal(t, os.FileMode(0o755), permOf(t, root), "an already-traversable shared dir must be left alone") + assert.Equal(t, os.FileMode(0o711), permOf(t, sub), "the xauth dir gains traversal, keeping its own bits") +} + +// A runtime dir that is not traversable gains execute, and nothing else: no +// read bit is handed out that was not there before. +func TestEnsureTraversableAddsOnlyExecute(t *testing.T) { + root := withRuntimeDir(t) + require.NoError(t, os.Chmod(root, 0o700)) + + sub := filepath.Join(root, vncXAuthSubdir) + require.NoError(t, os.Mkdir(sub, 0o700)) + + require.NoError(t, ensureTraversable(sub)) + + assert.Equal(t, os.FileMode(0o711), permOf(t, root)) + assert.Equal(t, os.FileMode(0o711), permOf(t, sub)) +} + +// A path outside the runtime dir is refused before anything is modified. +func TestEnsureTraversableRefusesOutsidePaths(t *testing.T) { + withRuntimeDir(t) + + outside := t.TempDir() + require.NoError(t, os.Chmod(outside, 0o700)) + + require.Error(t, ensureTraversable(outside)) + assert.Equal(t, os.FileMode(0o700), permOf(t, outside), "a refused path must not be chmodded") +}