diff --git a/client/status/status.go b/client/status/status.go index 84a2a28c5..eb106c8bd 100644 --- a/client/status/status.go +++ b/client/status/status.go @@ -448,7 +448,7 @@ func (o *OutputOverview) YAML() (string, error) { } // GeneralSummary returns a general summary of the status overview. -func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameServers bool, showSSHSessions bool) string { +func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameServers bool, showSessions bool) string { var managementConnString string if o.ManagementState.Connected { managementConnString = "Connected" @@ -573,7 +573,7 @@ func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameS sshServerStatus = "Enabled" } - if showSSHSessions && sessionCount > 0 { + if showSessions && sessionCount > 0 { for _, session := range o.SSHServerState.Sessions { var sessionDisplay string if session.JWTUsername != "" { @@ -611,7 +611,7 @@ func (o *OutputOverview) GeneralSummary(showURL bool, showRelays bool, showNameS vncServerStatus = "Enabled" } - if showSSHSessions && vncSessionCount > 0 { + if showSessions && vncSessionCount > 0 { for _, sess := range o.VNCServerState.Sessions { vncServerStatus += "\n " + formatVNCSessionLine(sess) } diff --git a/client/vnc/server/agent_windows.go b/client/vnc/server/agent_windows.go index 911a021a7..bab6035f5 100644 --- a/client/vnc/server/agent_windows.go +++ b/client/vnc/server/agent_windows.go @@ -800,17 +800,16 @@ func relogAgentOutput(pipe windows.Handle) { // each call site, while still capturing diagnostic info when the OS reports // a failure. func logCleanupCall(name string, proc *windows.LazyProc) { - r, _, err := proc.Call() - if r == 0 && err != nil && err != windows.NTE_OP_OK { - log.Tracef("%s: %v", name, err) - } + logCleanupCallArgs(name, proc) } -// logCleanupCallArgs is logCleanupCall with one argument; common pattern for +// logCleanupCallArgs is logCleanupCall with arguments; common pattern for // release-by-handle syscalls. func logCleanupCallArgs(name string, proc *windows.LazyProc, args ...uintptr) { r, _, err := proc.Call(args...) - if r == 0 && err != nil && err != windows.NTE_OP_OK { + // LazyProc.Call always returns a non-nil error carrying the thread's last + // error code, so a zero code is what "the call did not fail" looks like. + if r == 0 && !errors.Is(err, windows.ERROR_SUCCESS) { log.Tracef("%s: %v", name, err) } } diff --git a/client/vnc/server/capture_x11_shm_linux.go b/client/vnc/server/capture_x11_shm_linux.go index 24edff4df..466318fa5 100644 --- a/client/vnc/server/capture_x11_shm_linux.go +++ b/client/vnc/server/capture_x11_shm_linux.go @@ -31,25 +31,27 @@ func (c *X11Capturer) initSHM() error { return fmt.Errorf("shmat: %w", err) } - if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil { - log.Debugf("shmctl IPC_RMID: %v", err) - } - seg, err := shm.NewSegId(c.conn) if err != nil { - if detachErr := unix.SysvShmDetach(addr); detachErr != nil { - log.Debugf("shmdt on new-seg failure: %v", detachErr) - } + releaseShmSegment(id, addr) return fmt.Errorf("new SHM seg: %w", err) } + // The X server attaches before the segment is marked for deletion: since + // Linux 3.10 a shmat() against an IPC_RMID'd segment fails with EIDRM, so + // marking it first would push us onto the slow non-SHM path. if err := shm.AttachChecked(c.conn, seg, uint32(id), false).Check(); err != nil { - if detachErr := unix.SysvShmDetach(addr); detachErr != nil { - log.Debugf("shmdt on attach-checked failure: %v", detachErr) - } + releaseShmSegment(id, addr) return fmt.Errorf("SHM attach to X: %w", err) } + // Both ends hold the segment at this point, so marking it for deletion + // frees it as soon as the last of them detaches, even if this process + // dies without cleaning up. + if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil { + log.Debugf("shmctl IPC_RMID: %v", err) + } + c.shmID = id c.shmAddr = addr c.shmSeg = uint32(seg) @@ -57,6 +59,18 @@ func (c *X11Capturer) initSHM() error { return nil } +// releaseShmSegment gives a segment back on a setup path that failed after +// attaching it: detach this process, then mark it for deletion so it does not +// linger in the kernel's IPC table for the life of the host. +func releaseShmSegment(id int, addr []byte) { + if err := unix.SysvShmDetach(addr); err != nil { + log.Debugf("shmdt on setup failure: %v", err) + } + if _, err := unix.SysvShmCtl(id, unix.IPC_RMID, nil); err != nil { + log.Debugf("shmctl IPC_RMID on setup failure: %v", err) + } +} + func (c *X11Capturer) captureSHM() (*image.RGBA, error) { if err := c.fillSHM(); err != nil { return nil, err diff --git a/client/vnc/server/cursor_windows.go b/client/vnc/server/cursor_windows.go index 51815b7db..ed390f0dd 100644 --- a/client/vnc/server/cursor_windows.go +++ b/client/vnc/server/cursor_windows.go @@ -120,28 +120,15 @@ func (s *cursorSampler) sample() (*cursorSnapshot, error) { // cursor and stay armed for the next handle change rather than // treating this as a hard failure that would latch us off for // the session. - if s.lastHandle == hiddenHandle { - s.snapshot.posX = int(ci.PtPos.X) - s.snapshot.posY = int(ci.PtPos.Y) - s.snapshot.hasPos = true - return s.snapshot, nil + if s.lastHandle == hiddenHandle && s.snapshot != nil { + return s.publish(*s.snapshot, ci), nil } s.lastHandle = hiddenHandle s.serial++ - s.snapshot = &cursorSnapshot{ - img: transparentCursorImage(), - posX: int(ci.PtPos.X), - posY: int(ci.PtPos.Y), - hasPos: true, - serial: s.serial, - } - return s.snapshot, nil + return s.publish(cursorSnapshot{img: transparentCursorImage(), serial: s.serial}, ci), nil } if ci.Cursor == s.lastHandle && s.snapshot != nil { - s.snapshot.posX = int(ci.PtPos.X) - s.snapshot.posY = int(ci.PtPos.Y) - s.snapshot.hasPos = true - return s.snapshot, nil + return s.publish(*s.snapshot, ci), nil } img, hotX, hotY, err := decodeCursor(ci.Cursor) if err != nil { @@ -149,16 +136,21 @@ func (s *cursorSampler) sample() (*cursorSnapshot, error) { } s.lastHandle = ci.Cursor s.serial++ - s.snapshot = &cursorSnapshot{ - img: img, - hotX: hotX, - hotY: hotY, - posX: int(ci.PtPos.X), - posY: int(ci.PtPos.Y), - hasPos: true, - serial: s.serial, - } - return s.snapshot, nil + return s.publish(cursorSnapshot{img: img, hotX: hotX, hotY: hotY, serial: s.serial}, ci), nil +} + +// publish stamps the cursor's current position onto snap and stores it as the +// sampler's latest snapshot. A fresh value every time, never an update in +// place: the session encoder reads the snapshot the sampler last handed out, +// and must not see a position that is halfway between two samples. The sprite +// fields are carried over by value, so a snapshot the encoder still holds keeps +// pointing at the same immutable image. +func (s *cursorSampler) publish(snap cursorSnapshot, ci winCursorInfo) *cursorSnapshot { + snap.posX = int(ci.PtPos.X) + snap.posY = int(ci.PtPos.Y) + snap.hasPos = true + s.snapshot = &snap + return s.snapshot } // decodeCursor extracts the sprite at hCur as RGBA along with the hotspot. diff --git a/client/vnc/server/extclipboard_test.go b/client/vnc/server/extclipboard_test.go index 70a106af9..d82999a03 100644 --- a/client/vnc/server/extclipboard_test.go +++ b/client/vnc/server/extclipboard_test.go @@ -3,6 +3,8 @@ package server import ( + "bytes" + "compress/zlib" "encoding/binary" "strings" "testing" @@ -93,10 +95,43 @@ func TestExtClipProvideRoundTripLarge(t *testing.T) { assert.Equal(t, original, text) } -func TestParseExtClipProvideTextRejectsOversized(t *testing.T) { +func TestParseExtClipProvideTextRejectsMalformedStream(t *testing.T) { var fakePayload [4]byte // 4 bytes of zlib-compressed garbage won't decode; we want to ensure we // don't panic, not that we accept it. _, err := parseExtClipProvideText(extClipActionProvide|extClipFormatText, fakePayload[:]) assert.Error(t, err) } + +// The size caps are what keep a peer from making us allocate a record of its +// choosing, on either side of the wire. +func TestParseExtClipProvideTextRejectsOversizedRecord(t *testing.T) { + // A well-formed stream whose declared record size is past the cap: the + // bytes behind it are never read, so the guard is the only thing that + // stops the allocation. + var body bytes.Buffer + var lenBuf [4]byte + binary.BigEndian.PutUint32(lenBuf[:], uint32(extClipMaxText)+1) + body.Write(lenBuf[:]) + + var compressed bytes.Buffer + zw := zlib.NewWriter(&compressed) + _, err := zw.Write(body.Bytes()) + require.NoError(t, err) + require.NoError(t, zw.Close()) + + _, err = parseExtClipProvideText(extClipActionProvide|extClipFormatText, compressed.Bytes()) + require.Error(t, err) + assert.Contains(t, err.Error(), "record too large") +} + +func TestBuildExtClipProvideTextRejectsOversizedText(t *testing.T) { + // extClipMaxText itself is already one over: the builder appends a NUL + // terminator, and the length it writes counts it. + _, err := buildExtClipProvideText(strings.Repeat("a", extClipMaxText)) + require.Error(t, err) + assert.Contains(t, err.Error(), "exceeds extClipMaxText") + + _, err = buildExtClipProvideText(strings.Repeat("a", extClipMaxText-1)) + require.NoError(t, err, "one byte under the cap must still build") +} diff --git a/client/vnc/server/input_x11.go b/client/vnc/server/input_x11.go index a3eecb95c..3c9432dfc 100644 --- a/client/vnc/server/input_x11.go +++ b/client/vnc/server/input_x11.go @@ -32,9 +32,11 @@ type X11InputInjector struct { // NewX11InputInjector connects to the X11 display and initializes XTest. // Empty cookieHex/authFile fall back to XAUTHORITY env lookup. func NewX11InputInjector(display, cookieHex, authFile string) (*X11InputInjector, error) { - detectX11Display() - + // Only probe for a display when the caller named none: detection writes + // DISPLAY and XAUTHORITY into this process's environment, which has no + // business changing when the caller already knows which display to use. if display == "" { + detectX11Display() display = os.Getenv(envDisplay) } if display == "" { diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index ee06fa694..1cd04c05c 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -459,6 +459,16 @@ func (s *Server) trackConn(c net.Conn) { s.sessionsMu.Unlock() } +// retrackConn replaces a tracked raw connection with the wrapper its handler +// will actually hold, so shutdown and the handler's own untrackConn agree on +// which object is registered. +func (s *Server) retrackConn(raw, wrapped net.Conn) { + s.sessionsMu.Lock() + delete(s.acceptedConns, raw) + s.acceptedConns[wrapped] = struct{}{} + s.sessionsMu.Unlock() +} + // untrackConn forgets a connection once its handler is returning. func (s *Server) untrackConn(c net.Conn) { s.sessionsMu.Lock() @@ -1046,9 +1056,19 @@ func (s *Server) acquireVirtualSession(conn net.Conn, header *connectionHeader, return nil, nil, nil, false } vs.ClientConnect() + // GetOrCreate checks the session is alive, but nothing stops it being torn + // down between that check and here, and a nil capturer would only surface + // as a panic in the encoder. + capturer, injector := vs.Capturer(), vs.Injector() + if capturer == nil { + vs.ClientDisconnect() + rejectConnection(conn, codeMessage(RejectCodeSessionError, "virtual session stopped")) + (*connLog).Warnf("virtual session for %s stopped before the client attached", header.username) + return nil, nil, nil, false + } *connLog = (*connLog).WithField("vnc_user", header.username) (*connLog).Infof("session mode: user=%s display=%s", header.username, vs.Display()) - return vs.Capturer(), vs.Injector(), vs.ClientDisconnect, true + return capturer, injector, vs.ClientDisconnect, true } // acquireAttachSession bumps the shared capturer's per-session refcount diff --git a/client/vnc/server/server_darwin.go b/client/vnc/server/server_darwin.go index 055066049..906c77854 100644 --- a/client/vnc/server/server_darwin.go +++ b/client/vnc/server/server_darwin.go @@ -49,19 +49,24 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) { continue } + // Track before any early-reject path so a concurrent Stop's + // closeActiveSessions snapshot can never miss a just-accepted + // socket and let it survive shutdown. + s.trackConn(conn) if !s.tryAcquireConnSlot() { + s.untrackConn(conn) s.log.Warnf("rejecting VNC connection from %s: %d concurrent connections in flight", conn.RemoteAddr(), maxConcurrentVNCConns) _ = conn.Close() continue } enableTCPKeepAlive(conn, s.log) - conn = newMetricsConn(conn, s.sessionRecorder) - s.trackConn(conn) + metered := newMetricsConn(conn, s.sessionRecorder) + s.retrackConn(conn, metered) go func(c net.Conn) { defer s.releaseConnSlot() defer s.untrackConn(c) s.handleServiceConnection(c, mgr) - }(conn) + }(metered) } } diff --git a/client/vnc/server/server_windows.go b/client/vnc/server/server_windows.go index 84145376c..38fcb7563 100644 --- a/client/vnc/server/server_windows.go +++ b/client/vnc/server/server_windows.go @@ -336,19 +336,24 @@ func (s *Server) serviceAcceptLoop(ln net.Listener) { continue } + // Track before any early-reject path so a concurrent Stop's + // closeActiveSessions snapshot can never miss a just-accepted + // socket and let it survive shutdown. + s.trackConn(conn) if !s.tryAcquireConnSlot() { + s.untrackConn(conn) s.log.Warnf("rejecting VNC connection from %s: %d concurrent connections in flight", conn.RemoteAddr(), maxConcurrentVNCConns) _ = conn.Close() continue } enableTCPKeepAlive(conn, s.log) - conn = newMetricsConn(conn, s.sessionRecorder) - s.trackConn(conn) + metered := newMetricsConn(conn, s.sessionRecorder) + s.retrackConn(conn, metered) go func(c net.Conn) { defer s.releaseConnSlot() defer s.untrackConn(c) s.handleServiceConnection(c, sm) - }(conn) + }(metered) } } diff --git a/client/vnc/server/session.go b/client/vnc/server/session.go index 1d87947bb..b534b814c 100644 --- a/client/vnc/server/session.go +++ b/client/vnc/server/session.go @@ -4,6 +4,7 @@ package server import ( "encoding/binary" + "errors" "fmt" "image" "io" @@ -201,7 +202,9 @@ func (s *session) serve() { <-encoderDone }() - if err := s.messageLoop(); err != nil && err != io.EOF { + // messageLoop only ever returns an error, so the interesting question is + // which one: a clean client disconnect is io.EOF and not worth a warning. + if err := s.messageLoop(); !errors.Is(err, io.EOF) { s.log.Warnf("client %s disconnected: %v", s.addr(), err) } else { s.log.Infof("client disconnected: %s", s.addr()) diff --git a/client/vnc/server/session_clipboard.go b/client/vnc/server/session_clipboard.go index c31b0b8e4..5a47c3dc6 100644 --- a/client/vnc/server/session_clipboard.go +++ b/client/vnc/server/session_clipboard.go @@ -7,6 +7,7 @@ import ( "fmt" "io" "time" + "unicode/utf8" ) // clipboardPoll periodically checks the server-side clipboard and sends @@ -192,8 +193,10 @@ func (s *session) handleExtClipProvide(flags uint32, payload []byte) { // host clipboard contents, capped to extClipMaxText. func (s *session) sendExtClipProvideText() error { text := s.injector.GetClipboard() - if len(text) > extClipMaxText { - text = text[:extClipMaxText] + // One byte short of the cap: buildExtClipProvideText appends a NUL + // terminator, which counts against extClipMaxText. + if len(text) > extClipMaxText-1 { + text = trimPartialRune(text[:extClipMaxText-1]) } payload, err := buildExtClipProvideText(text) if err != nil { @@ -202,6 +205,20 @@ func (s *session) sendExtClipProvideText() error { return s.writeExtClipMessage(payload) } +// trimPartialRune drops the trailing bytes a byte-length cut left halfway +// through a UTF-8 rune, so the client is never handed invalid UTF-8. A real +// U+FFFD in the text decodes as three bytes and is kept. +func trimPartialRune(s string) string { + for s != "" { + r, size := utf8.DecodeLastRuneInString(s) + if r != utf8.RuneError || size > 1 { + return s + } + s = s[:len(s)-1] + } + return s +} + // writeExtClipMessage frames an ExtendedClipboard payload as a ServerCutText // message with a negative length, then writes it under writeMu. func (s *session) writeExtClipMessage(payload []byte) error { diff --git a/client/vnc/server/virtual_x11.go b/client/vnc/server/virtual_x11.go index 2068265fa..54af33346 100644 --- a/client/vnc/server/virtual_x11.go +++ b/client/vnc/server/virtual_x11.go @@ -231,13 +231,23 @@ func (vs *VirtualSession) isAlive() bool { return true } -// Capturer returns the screen capturer for this virtual session. +// Capturer returns the screen capturer for this virtual session, or nil once +// Stop has torn it down. Read under vs.mu, which is what Stop writes it under. func (vs *VirtualSession) Capturer() ScreenCapturer { + vs.mu.Lock() + defer vs.mu.Unlock() + + if vs.poller == nil { + return nil + } return vs.poller } // Injector returns the input injector for this virtual session. func (vs *VirtualSession) Injector() InputInjector { + vs.mu.Lock() + defer vs.mu.Unlock() + return vs.injector } diff --git a/client/vnc/server/xauth_x11.go b/client/vnc/server/xauth_x11.go index 66855aa4c..0914ef8fa 100644 --- a/client/vnc/server/xauth_x11.go +++ b/client/vnc/server/xauth_x11.go @@ -81,14 +81,17 @@ 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. Stops once it leaves the runtime dir so -// it never touches /var/run or /run. +// 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. func ensureTraversable(dir string) error { root := filepath.Clean(configs.RuntimeDir) if root == "" { return nil } cur := filepath.Clean(dir) + if cur != root && !strings.HasPrefix(cur, root+string(os.PathSeparator)) { + 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) @@ -97,7 +100,7 @@ func ensureTraversable(dir string) error { return nil } parent := filepath.Dir(cur) - if parent == cur || !strings.HasPrefix(cur, root+string(os.PathSeparator)) { + if parent == cur { return nil } cur = parent 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 5bbe6592e..4319d36a0 100644 --- a/management/server/http/handlers/peers/temporary_access_permission_test.go +++ b/management/server/http/handlers/peers/temporary_access_permission_test.go @@ -8,8 +8,8 @@ import ( "net/http/httptest" "testing" - "github.com/golang/mock/gomock" "github.com/gorilla/mux" + "go.uber.org/mock/gomock" nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/permissions" diff --git a/management/server/types/legacynmap/networkmap_components.go b/management/server/types/legacynmap/networkmap_components.go index 1d4a44ae9..b210f134f 100644 --- a/management/server/types/legacynmap/networkmap_components.go +++ b/management/server/types/legacynmap/networkmap_components.go @@ -10,10 +10,10 @@ import ( "sync" "time" - auth "github.com/netbirdio/netbird/shared/sessionauth" nbdns "github.com/netbirdio/netbird/dns" "github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/shared/management/domain" + auth "github.com/netbirdio/netbird/shared/sessionauth" ) type NetworkMapComponents struct { diff --git a/management/server/types/legacynmap/proto_legacy.go b/management/server/types/legacynmap/proto_legacy.go index 79841df64..558b7ddb3 100644 --- a/management/server/types/legacynmap/proto_legacy.go +++ b/management/server/types/legacynmap/proto_legacy.go @@ -7,7 +7,6 @@ import ( "net/url" "strings" - auth "github.com/netbirdio/netbird/shared/sessionauth" nbconfig "github.com/netbirdio/netbird/management/internals/server/config" nbpeer "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/types" @@ -15,6 +14,7 @@ import ( "github.com/netbirdio/netbird/shared/management/networkmap" "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/netiputil" + auth "github.com/netbirdio/netbird/shared/sessionauth" ) func ToProtocolRoutes(routes []*nbroute.Route) []*proto.Route { diff --git a/shared/sessionauth/auth.go b/shared/sessionauth/auth.go index 9628a6b0d..f69e1869b 100644 --- a/shared/sessionauth/auth.go +++ b/shared/sessionauth/auth.go @@ -104,7 +104,7 @@ func (a *Authorizer) Update(config *Config) { a.machineUsers = make(map[string][]uint32) a.sessionPubKeys = make(map[[sessionPubKeyLen]byte]sshuserhash.UserIDHash) a.sessionDisplayNames = make(map[[sessionPubKeyLen]byte]string) - log.Info("SSH authorization cleared") + log.Info("session authorization cleared") return } @@ -139,7 +139,7 @@ func (a *Authorizer) Update(config *Config) { continue } if existing, ok := sessionPubKeys[key]; ok && existing != e.UserIDHash { - log.Warnf("SSH auth: session pubkey bound to conflicting user hashes; dropping binding") + log.Warn("session auth: session pubkey bound to conflicting user hashes; dropping binding") delete(sessionPubKeys, key) delete(sessionDisplayNames, key) conflicted[key] = struct{}{} @@ -153,7 +153,7 @@ func (a *Authorizer) Update(config *Config) { a.sessionPubKeys = sessionPubKeys a.sessionDisplayNames = sessionDisplayNames - log.Debugf("SSH auth: updated with %d authorized users, %d machine user mappings, %d session pubkeys", + log.Debugf("session auth: updated with %d authorized users, %d machine user mappings, %d session pubkeys", len(config.AuthorizedUsers), len(machineUsers), len(sessionPubKeys)) } diff --git a/util/capture/text.go b/util/capture/text.go index 6ffdebef0..4ceafedd5 100644 --- a/util/capture/text.go +++ b/util/capture/text.go @@ -538,11 +538,13 @@ func matchRFBSecurityFailure(p []byte) (string, bool) { if len(p) < 5 || p[0] != 0 { return "", false } - reasonLen := int(p[1])<<24 | int(p[2])<<16 | int(p[3])<<8 | int(p[4]) - if reasonLen <= 0 || reasonLen > 4096 || 5+reasonLen != len(p) { + // Kept as uint32: converting to int first would wrap to a negative value + // on a 32-bit build and lose the annotation to the length check below. + reasonLen := binary.BigEndian.Uint32(p[1:5]) + if reasonLen == 0 || reasonLen > 4096 || 5+int(reasonLen) != len(p) { return "", false } - return string(p[5 : 5+reasonLen]), true + return string(p[5 : 5+int(reasonLen)]), true } // vncRejectCodes mirrors the RejectCode* constants in