Close view-only input, approval-responder and inbound-block gaps in the VNC path

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()

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 {

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)
+		})
+	}
+}

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)))

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
 }