Apply the socket restriction before serving and fail closed on an unusable one

client/cmd/service.go

@@ -72,11 +72,12 @@ func init() {
 	serviceCmd.PersistentFlags().StringVar(&jsonSocket, "json-socket", defaultJSONSocket, "HTTP/JSON API socket address [unix|tcp]://[path|host:port]. Requires --enable-json-socket to serve. To persist, use: netbird service install --enable-json-socket --json-socket")
 
 	allowGroupDesc := `Restricts the daemon control socket and the JSON socket to the given group. ` +
-		`Local accounts outside it cannot connect at all, so nothing the daemon exposes is reachable from them. ` +
+		`Accounts outside it cannot connect at all, so nothing the daemon exposes is reachable from them. ` +
+		`On Windows the daemon's own account, LocalSystem and elevated administrators keep access regardless. ` +
 		`Takes a group name, or a numeric GID on Unix and a SID on Windows; ` +
 		`Unix accepts a single group, Windows a comma-separated list of groups or accounts. ` +
 		`Names are resolved when the service is installed, LDAP, SSSD and Active Directory groups included. ` +
-		`To persist, use: netbird service install --allow-group`
+		`To persist, use: netbird service install --allow-group <group>`
 	serviceCmd.PersistentFlags().StringSliceVar(&allowGroups, "allow-group", nil, allowGroupDesc)
 
 	rootCmd.PersistentFlags().StringVarP(&serviceName, "service", "s", defaultServiceName, "Netbird system service name")

client/cmd/service_allow_group.go

@@ -68,8 +68,18 @@ func resolveAllowGroups(values []string) ([]string, error) {
 // meant to be locked down.
 func daemonSocketPrincipals(policy *mdm.Policy) ([]string, string, error) {
 	values, source := allowGroups, "--allow-group"
-	if managed, ok := policy.GetStringSlice(mdm.KeyAllowGroups); ok {
-		values, source = managed, "MDM policy "+mdm.KeyAllowGroups
+
+	if policy.HasKey(mdm.KeyAllowGroups) {
+		source = "MDM policy " + mdm.KeyAllowGroups
+		managed, ok := policy.GetStringSlice(mdm.KeyAllowGroups)
+		if !ok {
+			// The key is managed but holds something that is not a list of
+			// strings. Falling back to the install-time value, or to no
+			// restriction at all, would apply an access rule the administrator
+			// did not write.
+			return nil, source, fmt.Errorf("%s: managed value is not a list of principals", source)
+		}
+		values = managed
 	}
 
 	resolved, err := resolveAllowGroups(values)

client/cmd/service_allow_group_test.go

@@ -98,6 +98,27 @@ func TestDaemonSocketPrincipals(t *testing.T) {
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), mdm.KeyAllowGroups)
 	})
+
+	// A managed key holding something that is not a list of principals must not
+	// read as "no policy set" and quietly hand the decision back to the
+	// install-time value.
+	t.Run("a malformed MDM value is an error, not a fallback", func(t *testing.T) {
+		allowGroups = []string{testAllowGroupPrincipal}
+
+		for name, value := range map[string]any{
+			"number": 42,
+			"bool":   true,
+			"map":    map[string]any{"group": "x"},
+		} {
+			t.Run(name, func(t *testing.T) {
+				policy := mdm.NewPolicy(map[string]any{mdm.KeyAllowGroups: value})
+
+				_, source, err := daemonSocketPrincipals(policy)
+				require.Error(t, err)
+				assert.Contains(t, source, mdm.KeyAllowGroups, "the managed key must be named as the source that failed")
+			})
+		}
+	})
 }
 
 // A TCP listener can express neither a socket mode nor a security descriptor,

client/cmd/service_allow_group_unix.go

@@ -5,7 +5,9 @@ package cmd
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strconv"
+	"syscall"
 
 	"github.com/netbirdio/netbird/client/internal/getent"
 )
@@ -61,6 +63,13 @@ func checkAllowGroupSet(principals []string) error {
 // before the mode is widened, so the window between the two is one where the
 // group has no access rather than one where it has access it should not.
 func applySocketAccess(path string, principals []string) error {
+	// The listener just bound this path, so anything else standing there now is
+	// something another account substituted. Checked before either call below,
+	// neither of which should ever act on a name the daemon did not create.
+	if err := requireSocketFile(path); err != nil {
+		return err
+	}
+
 	if len(principals) == 0 {
 		if err := os.Chmod(path, openSocketMode); err != nil {
 			return fmt.Errorf("set mode %#o: %w", openSocketMode, err)
@@ -77,7 +86,14 @@ func applySocketAccess(path string, principals []string) error {
 		return err
 	}
 
-	if err := os.Chown(path, -1, gid); err != nil {
+	if err := requireTrustedSocketDir(filepath.Dir(path)); err != nil {
+		return err
+	}
+
+	// Lchown, not Chown: chown follows symlinks, so a daemon running as root
+	// would otherwise hand an arbitrary target away to the configured group if
+	// the name were swapped between the check above and here.
+	if err := os.Lchown(path, -1, gid); err != nil {
 		return fmt.Errorf("set group to gid %d: %w", gid, err)
 	}
 	if err := os.Chmod(path, restrictedSocketMode); err != nil {
@@ -86,17 +102,71 @@ func applySocketAccess(path string, principals []string) error {
 	return nil
 }
 
+// requireSocketFile reports whether path is the socket the listener created,
+// without following a symlink standing in its place.
+func requireSocketFile(path string) error {
+	info, err := os.Lstat(path)
+	if err != nil {
+		return fmt.Errorf("stat socket: %w", err)
+	}
+	if info.Mode()&os.ModeSocket == 0 {
+		return fmt.Errorf("%s is not a socket (mode %s), refusing to change its access", path, info.Mode())
+	}
+	return nil
+}
+
+// requireTrustedSocketDir refuses to apply a restriction inside a directory
+// where another account could swap the socket for something else between the
+// check and the change. That is only true of a directory some other account can
+// write to: a sticky directory is fine, since only the owner of an entry may
+// replace it there.
+//
+// The default locations are root-owned, so this rejects nothing an ordinary
+// install does. It exists because the socket paths are configurable.
+func requireTrustedSocketDir(dir string) error {
+	info, err := os.Stat(dir)
+	if err != nil {
+		return fmt.Errorf("stat socket directory: %w", err)
+	}
+
+	mode := info.Mode()
+	if mode.Perm()&0o022 != 0 && mode&os.ModeSticky == 0 {
+		return fmt.Errorf("socket directory %s is writable by other accounts (mode %s), refusing to restrict a socket that they can replace", dir, mode.Perm())
+	}
+
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		return fmt.Errorf("cannot read ownership of socket directory %s", dir)
+	}
+	if stat.Uid != 0 && stat.Uid != uint32(os.Geteuid()) {
+		return fmt.Errorf("socket directory %s is owned by uid %d, which is neither root nor this daemon", dir, stat.Uid)
+	}
+	return nil
+}
+
+// gidPrincipal renders a GID as a principal in its canonical decimal form, so
+// that spellings of the same group ("gid:01" and "gid:1") produce one principal
+// rather than two that later look like a request to use two groups.
 func gidPrincipal(gid string) (string, error) {
-	if _, err := parseGID(gid); err != nil {
+	parsed, err := parseGID(gid)
+	if err != nil {
 		return "", err
 	}
-	return allowGroupKindGID + ":" + gid, nil
+	return allowGroupKindGID + ":" + strconv.Itoa(parsed), nil
 }
 
+// unchangedGID is the value chown reads as "leave the group alone". A
+// configured GID that lands on it would silently keep whatever group the socket
+// already had, so it is rejected rather than applied.
+const unchangedGID = 1<<32 - 1
+
 func parseGID(value string) (int, error) {
 	gid, err := strconv.ParseUint(value, 10, 32)
 	if err != nil {
 		return 0, fmt.Errorf("parse gid %q: %w", value, err)
 	}
+	if gid == unchangedGID {
+		return 0, fmt.Errorf("gid %d is not a usable group", gid)
+	}
 	return int(gid), nil
 }

client/cmd/service_allow_group_unix_test.go

@@ -43,6 +43,14 @@ func TestResolveAllowGroup_ByName(t *testing.T) {
 	assert.Equal(t, "gid:"+gid, principal)
 }
 
+// Spellings of the same GID must collapse to one principal, otherwise
+// checkAllowGroupSet reads them as a request for two groups and refuses.
+func TestResolveAllowGroups_CanonicalisesGIDs(t *testing.T) {
+	resolved, err := resolveAllowGroups([]string{"gid:01", "gid:1", "1"})
+	require.NoError(t, err)
+	assert.Equal(t, []string{"gid:1"}, resolved)
+}
+
 func TestResolveAllowGroup_Rejects(t *testing.T) {
 	tests := []struct {
 		name  string
@@ -53,6 +61,9 @@ func TestResolveAllowGroup_Rejects(t *testing.T) {
 		{name: "non-numeric gid", value: "gid:wheel"},
 		{name: "negative gid", value: "gid:-1"},
 		{name: "unknown group", value: "no-such-group-08b1f0c4"},
+		// chown reads this as "leave the group alone", so applying it would
+		// leave the socket on whatever group it already had.
+		{name: "the unchanged-gid sentinel", value: "gid:4294967295"},
 	}
 
 	for _, tc := range tests {
@@ -101,6 +112,49 @@ func TestApplySocketAccess(t *testing.T) {
 
 		require.Error(t, applySocketAccess(path, []string{"gid:wheel"}))
 	})
+
+	// A symlink standing where the listener put its socket is something another
+	// account substituted, and chowning it as root would hand its target away.
+	t.Run("a path that is not a socket is refused", func(t *testing.T) {
+		dir, err := os.MkdirTemp("", "nb-sock")
+		require.NoError(t, err)
+		t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
+
+		target := filepath.Join(dir, "target")
+		require.NoError(t, os.WriteFile(target, []byte("not a socket"), 0600))
+		link := filepath.Join(dir, "d.sock")
+		require.NoError(t, os.Symlink(target, link))
+
+		gid := strconv.Itoa(os.Getgid())
+		require.Error(t, applySocketAccess(link, []string{"gid:" + gid}))
+		require.Error(t, applySocketAccess(link, nil), "the open path must not follow it either")
+
+		// The substituted target keeps the mode it was created with.
+		info, err := os.Stat(target)
+		require.NoError(t, err)
+		assert.Equal(t, os.FileMode(0600), info.Mode().Perm())
+	})
+
+	// Restricting a socket in a directory other accounts can write to is
+	// refused: they can replace the entry between the check and the change.
+	t.Run("an untrusted socket directory is refused", func(t *testing.T) {
+		dir, err := os.MkdirTemp("", "nb-sock")
+		require.NoError(t, err)
+		t.Cleanup(func() { assert.NoError(t, os.RemoveAll(dir)) })
+		require.NoError(t, os.Chmod(dir, 0777))
+
+		path := filepath.Join(dir, "d.sock")
+		listener, err := net.Listen("unix", path)
+		require.NoError(t, err)
+		t.Cleanup(func() { assert.NoError(t, listener.Close()) })
+
+		gid := strconv.Itoa(os.Getgid())
+		require.Error(t, applySocketAccess(path, []string{"gid:" + gid}))
+
+		// Leaving it unrestricted is still allowed: that is the historical
+		// behaviour and grants nothing the mode did not already grant.
+		assert.NoError(t, applySocketAccess(path, nil))
+	})
 }
 
 func listenTestSocket(t *testing.T) string {

client/cmd/service_allow_group_windows_test.go

@@ -103,9 +103,11 @@ func TestAllowedPipeSDDL(t *testing.T) {
 	})
 }
 
-// TestListenNamedPipe_RestrictedDescriptor covers the descriptor reaching the
-// listener, which is the only part of the restriction that cannot be asserted
-// from allowedPipeSDDL alone.
+// TestListenNamedPipe_RestrictedDescriptor covers only that a restricted
+// descriptor is accepted by ListenPipe and the pipe is created. Whether the
+// descriptor actually denies an outside principal is not asserted here: that
+// needs a second account and a connect attempt, so it is covered by the
+// allowedPipeSDDL assertions above plus manual testing.
 func TestListenNamedPipe_RestrictedDescriptor(t *testing.T) {
 	listener, path, err := listenNamedPipe("netbird-test-"+t.Name(), []string{"sid:" + sidAdministrators})
 	require.NoError(t, err)

client/cmd/service_controller.go

@@ -90,7 +90,7 @@ func (p *program) Start(svc service.Service) error {
 	)
 	p.serv = grpc.NewServer(opts...)
 
-	allowed, source, err := daemonSocketPrincipals(mdm.LoadPolicy())
+	daemonListener, jsonListener, err := p.listenRestricted()
 	if err != nil {
 		// Logged as well as returned: the service manager is the only other
 		// place this surfaces, and it reports a service that will not start
@@ -99,23 +99,72 @@ func (p *program) Start(svc service.Service) error {
 		log.Errorf("failed to apply the daemon socket restriction, not serving: %v", err)
 		return err
 	}
+
+	go func() {
+		// Fatal here rather than inside serve, so serve's deferred listener
+		// closes run before the process exits.
+		if err := p.serve(daemonListener, jsonListener); err != nil {
+			log.Fatalf("failed to %v", err)
+		}
+	}()
+	return nil
+}
+
+// listenRestricted opens the daemon sockets and applies the configured access
+// restriction to them before returning, so no caller can reach a socket that is
+// still open to everybody. Both listeners are closed again if the restriction
+// cannot be applied, and the error is returned rather than handled later, so the
+// service manager sees a start that failed instead of one that succeeded and
+// then died.
+//
+// An unreadable MDM source is an error here for the same reason a bad value is:
+// on a managed host it may be carrying the restriction, and treating it as
+// absent would serve every local account instead.
+func (p *program) listenRestricted() (*socketListener, *socketListener, error) {
+	policy, err := mdm.LoadPolicyWithError()
+	if err != nil {
+		return nil, nil, err
+	}
+
+	allowed, source, err := daemonSocketPrincipals(policy)
+	if err != nil {
+		return nil, nil, err
+	}
 	if len(allowed) > 0 {
 		log.Infof("daemon sockets are restricted to %v by %s", allowed, source)
 	}
 
 	daemonListener, jsonListener, err := listenDaemonSockets(allowed)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 
-	go func() {
-		// Fatal here rather than inside serve, so serve's deferred listener
-		// closes run before the process exits.
-		if err := p.serve(daemonListener, jsonListener, allowed); err != nil {
-			log.Fatalf("failed to %v", err)
+	if err := restrictListeners(daemonListener, jsonListener, allowed); err != nil {
+		closeListeners(daemonListener, jsonListener)
+		return nil, nil, err
+	}
+
+	return daemonListener, jsonListener, nil
+}
+
+// restrictListeners applies the access restriction to both sockets. restrict is
+// a no-op for a nil listener, which is what a disabled JSON socket is.
+func restrictListeners(daemonListener, jsonListener *socketListener, allowed []string) error {
+	if err := daemonListener.restrict("daemon", allowed); err != nil {
+		return err
+	}
+	return jsonListener.restrict("daemon JSON", allowed)
+}
+
+func closeListeners(listeners ...*socketListener) {
+	for _, l := range listeners {
+		if l == nil {
+			continue
 		}
-	}()
-	return nil
+		if err := l.Close(); err != nil {
+			log.Debugf("close daemon listener: %v", err)
+		}
+	}
 }
 
 // listenDaemonSockets opens the daemon control socket and, when it is enabled, the
@@ -145,29 +194,17 @@ func listenDaemonSockets(allowed []string) (*socketListener, *socketListener, er
 	return daemonListener, jsonListener, nil
 }
 
-// serve brings up the daemon server on an already-open control socket and blocks
-// until it stops. jsonListener is nil when the JSON socket is disabled. A returned
-// error means the daemon cannot run at all and the caller is expected to exit; the
-// failures it recovers from on its own are logged here.
-func (p *program) serve(daemonListener, jsonListener *socketListener, allowed []string) error {
+// serve brings up the daemon server on listeners that are already open and
+// already restricted, and blocks until it stops. jsonListener is nil when the
+// JSON socket is disabled. A returned error means the daemon cannot run at all
+// and the caller is expected to exit; the failures it recovers from on its own
+// are logged here.
+func (p *program) serve(daemonListener, jsonListener *socketListener) error {
 	defer daemonListener.Close()
 	if jsonListener != nil {
 		defer jsonListener.Close()
 	}
 
-	// Returned rather than logged: a socket whose access could not be set is
-	// either open to accounts that must not reach the daemon, or unreachable by
-	// the ones that must. Both are worse than the caller's fatal exit, and
-	// swallowing this would leave a service the manager still reports as running
-	// with no usable socket. restrict is a no-op for a nil listener, which is
-	// what a disabled JSON socket is.
-	if err := daemonListener.restrict("daemon", allowed); err != nil {
-		return err
-	}
-	if err := jsonListener.restrict("daemon JSON", allowed); err != nil {
-		return err
-	}
-
 	serverInstance := server.New(p.ctx, util.FindFirstLogPath(logFiles), configPath, profilesDisabled, updateSettingsDisabled, captureEnabled, networksDisabled)
 	p.authzGate.SetState(serverInstance)
 	if err := serverInstance.Start(); err != nil {