From 071bd94d14810d7a8bbba9e62ae940989423dddf Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Thu, 27 Aug 2026 21:07:03 +0200 Subject: [PATCH] Close view-only input, approval-responder and inbound-block gaps in the VNC path --- client/server/server.go | 20 +++++++- client/server/ssh_gate.go | 36 +++++++++++++ client/server/ssh_gate_test.go | 70 ++++++++++++++++++++++++++ client/vnc/server/server.go | 10 ++++ client/vnc/server/session_clipboard.go | 14 +++++- 5 files changed, 148 insertions(+), 2 deletions(-) diff --git a/client/server/server.go b/client/server/server.go index ee6cbd587..9d1f9bec3 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -25,6 +25,7 @@ import ( "github.com/netbirdio/netbird/client/internal/expose" "github.com/prometheus/client_golang/prometheus" + "github.com/netbirdio/netbird/client/internal/ipcauth" "github.com/netbirdio/netbird/client/internal/localmetrics" "github.com/netbirdio/netbird/client/internal/profilemanager" sleephandler "github.com/netbirdio/netbird/client/internal/sleep/handler" @@ -2080,10 +2081,27 @@ func (s *Server) ExposeService(req *proto.ExposeServiceRequest, srv proto.Daemon // approval prompt to the engine's broker. Unknown or already-resolved // request_ids are silently no-op'd so a slow UI cannot deny a prompt the // user already handled (or that already timed out). -func (s *Server) RespondApproval(_ context.Context, msg *proto.RespondApprovalRequest) (*proto.RespondApprovalResponse, error) { +// +// The answer is the whole of the consent the prompt asks for, so a caller the +// daemon cannot name is refused: on a control channel that carries no identity +// (loopback TCP) anything that can open a socket could accept a session on the +// console user's behalf. An identified local caller is accepted and logged. +// Restricting it further, to the user who owns the console, needs an +// owner-gated IPC channel that does not exist yet, and on a multi-seat Linux +// host there is no single such user to check against. +func (s *Server) RespondApproval(ctx context.Context, msg *proto.RespondApprovalRequest) (*proto.RespondApprovalResponse, error) { if msg.GetRequestId() == "" { return nil, gstatus.Errorf(codes.InvalidArgument, "request_id is required") } + id, ok := ipcauth.CallerIdentity(ctx) + if !ok { + log.Warnf("refusing approval response for %s: the caller's identity cannot be verified on this control channel", msg.GetRequestId()) + return nil, gstatus.Errorf(codes.PermissionDenied, + "answering a connection approval requires a control channel that carries the caller's identity. "+ + "Reinstall the service on a socket that does: %s", reinstallCommand()) + } + log.Infof("approval response for %s from caller %s: accept=%t view_only=%t", + msg.GetRequestId(), id, msg.GetAccept(), msg.GetViewOnly()) s.mutex.Lock() connectClient := s.connectClient s.mutex.Unlock() diff --git a/client/server/ssh_gate.go b/client/server/ssh_gate.go index f9b75a521..bbc920f52 100644 --- a/client/server/ssh_gate.go +++ b/client/server/ssh_gate.go @@ -35,6 +35,10 @@ import ( // including which keys and users are accepted, to whoever controls that // identity. Changing the management URL and deregistering the peer are both // ways to do that. +// - Unblocking inbound connections while a remote-access server is enabled +// starts that server: the engine keeps SSH and VNC down for as long as +// inbound traffic is blocked, so clearing the block is another way to hand +// out a shell or a desktop. // - Binding the local metrics endpoint to a non-loopback address publishes // peer names and connectivity state to the network without authentication. // @@ -53,6 +57,7 @@ type privilegedConfigChange struct { disableSSHAuth *bool serverVNCAllowed *bool disableVNCApproval *bool + blockInbound *bool enableLocalMetrics *bool localMetricsAddress *string } @@ -65,6 +70,7 @@ func privilegedChangeFromSetConfig(msg *proto.SetConfigRequest) privilegedConfig disableSSHAuth: msg.DisableSSHAuth, serverVNCAllowed: msg.ServerVNCAllowed, disableVNCApproval: msg.DisableVNCApproval, + blockInbound: msg.BlockInbound, enableLocalMetrics: msg.EnableLocalMetrics, localMetricsAddress: msg.LocalMetricsAddress, } @@ -78,6 +84,7 @@ func privilegedChangeFromLogin(msg *proto.LoginRequest) privilegedConfigChange { disableSSHAuth: msg.DisableSSHAuth, serverVNCAllowed: msg.ServerVNCAllowed, disableVNCApproval: msg.DisableVNCApproval, + blockInbound: msg.BlockInbound, enableLocalMetrics: msg.EnableLocalMetrics, localMetricsAddress: msg.LocalMetricsAddress, } @@ -127,6 +134,15 @@ func requirePrivilegeForConfigChange(ctx context.Context, stored *profilemanager ipcauth.UpCommand("-m "+change.managementURL)) } + // The engine keeps a remote-access server down while inbound connections + // are blocked, so lifting the block on a profile that has one enabled + // starts it. That is the same privilege as enabling it in the first place. + if disables(storedBlockInbound(stored), change.blockInbound) { + return denyPrivileged(ctx, + fmt.Sprintf("unblocking inbound connections while the NetBird %s server is enabled", server), + ipcauth.UpCommand("--block-inbound=false")) + } + return nil } @@ -243,6 +259,26 @@ func enables(stored, requested *bool) bool { return stored == nil || !*stored } +// disables reports whether requested turns a flag off that is currently on. The +// mirror of enables, for a flag whose privileged direction is being cleared. +func disables(stored, requested *bool) bool { + if requested == nil || *requested { + return false + } + return stored != nil && *stored +} + +// storedBlockInbound reads the inbound-block flag from the stored config. It +// defaults to off, which is the engine's own default, so a config written +// before the flag existed is not read as blocking. +func storedBlockInbound(cfg *profilemanager.Config) *bool { + if cfg == nil { + return nil + } + blocked := cfg.BlockInbound + return &blocked +} + // storedFlag reads a flag from the stored config, tolerating a config that does // not exist yet. func storedFlag(cfg *profilemanager.Config, get func(*profilemanager.Config) *bool) *bool { diff --git a/client/server/ssh_gate_test.go b/client/server/ssh_gate_test.go index d71cd86ef..11818b06d 100644 --- a/client/server/ssh_gate_test.go +++ b/client/server/ssh_gate_test.go @@ -444,3 +444,73 @@ func TestRequirePrivilegeForDeregistration(t *testing.T) { // directly have no transport credentials, and the privileged-change gate refuses // a caller it cannot identify. func privilegedTestCtx() context.Context { return rootCtx() } + +// The engine keeps SSH and VNC down while inbound connections are blocked, so +// clearing that block on a profile with one enabled starts the server. An +// unprivileged caller must not be able to do it by that route either. +func TestRequirePrivilegeForConfigChange_UnblockInbound(t *testing.T) { + tests := []struct { + name string + stored *profilemanager.Config + change privilegedConfigChange + privileged bool + wantDeny bool + }{ + { + name: "clearing the block with the SSH server enabled", + stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true}, + change: privilegedConfigChange{blockInbound: boolPtr(false)}, + wantDeny: true, + }, + { + name: "clearing the block with only the VNC server enabled", + stored: &profilemanager.Config{ + ServerSSHAllowed: boolPtr(false), + ServerVNCAllowed: boolPtr(true), + BlockInbound: true, + }, + change: privilegedConfigChange{blockInbound: boolPtr(false)}, + wantDeny: true, + }, + { + name: "an administrator may clear it", + stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true}, + change: privilegedConfigChange{blockInbound: boolPtr(false)}, + privileged: true, + }, + { + name: "restating the stored block is not a change", + stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: true}, + change: privilegedConfigChange{blockInbound: boolPtr(true)}, + }, + { + name: "turning the block on is never privileged", + stored: &profilemanager.Config{ServerSSHAllowed: boolPtr(true), BlockInbound: false}, + change: privilegedConfigChange{blockInbound: boolPtr(true)}, + }, + { + name: "with no remote-access server enabled it is the user's own business", + stored: &profilemanager.Config{ + ServerSSHAllowed: boolPtr(false), + ServerVNCAllowed: boolPtr(false), + BlockInbound: true, + }, + change: privilegedConfigChange{blockInbound: boolPtr(false)}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := userCtx() + if tt.privileged { + ctx = rootCtx() + } + err := requirePrivilegeForConfigChange(ctx, tt.stored, tt.change) + if tt.wantDeny { + assertDenied(t, err) + return + } + assertAllowed(t, err) + }) + } +} diff --git a/client/vnc/server/server.go b/client/vnc/server/server.go index 8fe586b48..79a71a541 100644 --- a/client/vnc/server/server.go +++ b/client/vnc/server/server.go @@ -1039,6 +1039,16 @@ func (s *Server) acquireVirtualSession(conn net.Conn, header *connectionHeader, (*connLog).Warn("session rejected: no username provided") return nil, nil, nil, false } + // The requested geometry comes off the wire and is handed straight to the X + // server, which allocates a framebuffer for it. The cap the rest of the + // pipeline enforces is only checked once the capturer is up, which is too + // late to stop a peer asking for 65535x65535. Zero means "use the default". + if header.width > maxFramebufferDim || header.height > maxFramebufferDim { + rejectConnection(conn, codeMessage(RejectCodeBadRequest, + fmt.Sprintf("requested geometry out of range: %dx%d", header.width, header.height))) + (*connLog).Warnf("session rejected: requested %dx%d exceeds cap %d", header.width, header.height, maxFramebufferDim) + return nil, nil, nil, false + } vs, err := s.vmgr.GetOrCreate(header.username, header.width, header.height) if err != nil { rejectConnection(conn, codeMessage(RejectCodeSessionError, fmt.Sprintf("create virtual session: %v", err))) diff --git a/client/vnc/server/session_clipboard.go b/client/vnc/server/session_clipboard.go index 5a47c3dc6..0b80d1248 100644 --- a/client/vnc/server/session_clipboard.go +++ b/client/vnc/server/session_clipboard.go @@ -68,6 +68,12 @@ func (s *session) handleCutText() error { if _, err := io.ReadFull(s.conn, buf); err != nil { return fmt.Errorf("read CutText payload: %w", err) } + // Writing the host clipboard changes host state, so a view-only session + // must not do it either. The payload is read first regardless, to leave the + // stream positioned at the next message. + if s.viewOnly { + return nil + } s.injector.SetClipboard(latin1ToUTF8(buf)) return nil } @@ -184,7 +190,7 @@ func (s *session) handleExtClipProvide(flags uint32, payload []byte) { s.log.Debugf("parse ext clipboard provide: %v", err) return } - if text != "" { + if text != "" && !s.viewOnly { s.injector.SetClipboard(text) } } @@ -254,6 +260,12 @@ func (s *session) handleTypeText() error { if _, err := io.ReadFull(s.conn, buf); err != nil { return fmt.Errorf("read TypeText payload: %w", err) } + // Synthesized keystrokes are input like any other, so a view-only session + // must not deliver them. The payload is read first regardless, to leave the + // stream positioned at the next message. + if s.viewOnly { + return nil + } s.injector.TypeText(string(buf)) return nil }