diff --git a/client/cmd/service.go b/client/cmd/service.go index 30436418e..b917cb949 100644 --- a/client/cmd/service.go +++ b/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 ` serviceCmd.PersistentFlags().StringSliceVar(&allowGroups, "allow-group", nil, allowGroupDesc) rootCmd.PersistentFlags().StringVarP(&serviceName, "service", "s", defaultServiceName, "Netbird system service name") diff --git a/client/cmd/service_allow_group.go b/client/cmd/service_allow_group.go index 5e18c47f6..3e217b57b 100644 --- a/client/cmd/service_allow_group.go +++ b/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) diff --git a/client/cmd/service_allow_group_test.go b/client/cmd/service_allow_group_test.go index 9f0479761..ca901e2cf 100644 --- a/client/cmd/service_allow_group_test.go +++ b/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, diff --git a/client/cmd/service_allow_group_unix.go b/client/cmd/service_allow_group_unix.go index f5e3b2391..63d724ae9 100644 --- a/client/cmd/service_allow_group_unix.go +++ b/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 } diff --git a/client/cmd/service_allow_group_unix_test.go b/client/cmd/service_allow_group_unix_test.go index 7e5dde264..0a7ea1bde 100644 --- a/client/cmd/service_allow_group_unix_test.go +++ b/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 { diff --git a/client/cmd/service_allow_group_windows_test.go b/client/cmd/service_allow_group_windows_test.go index 419b57a6f..a69111f1a 100644 --- a/client/cmd/service_allow_group_windows_test.go +++ b/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) diff --git a/client/cmd/service_controller.go b/client/cmd/service_controller.go index 1e35287e3..478f1b4a2 100644 --- a/client/cmd/service_controller.go +++ b/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 { diff --git a/client/mdm/policy.go b/client/mdm/policy.go index a23b92e51..125d1a255 100644 --- a/client/mdm/policy.go +++ b/client/mdm/policy.go @@ -9,6 +9,7 @@ package mdm import ( + "fmt" "sort" "strconv" "strings" @@ -142,20 +143,41 @@ func NewPolicy(values map[string]any) *Policy { // - source present, zero keys: info "MDM enrolled (no managed keys)" // - source present, N keys: info "MDM enrolled with N managed keys: [...]" func LoadPolicy() *Policy { - values, err := loadPlatformPolicy() + policy, err := LoadPolicyWithError() if err != nil { log.Tracef("MDM policy load: %v", err) return &Policy{values: map[string]any{}} } + return policy +} + +// LoadPolicyWithError reads the platform-native MDM configuration and reports a +// source that is present but could not be read, which LoadPolicy hides behind an +// empty Policy. +// +// A caller that enforces a managed security setting must use this and fail +// closed: an unreadable source is not the same as an absent one, and treating +// it as absent silently drops whatever the administrator configured. A caller +// that only decides a default, such as the UI's autostart checkbox, is better +// served by LoadPolicy. +// +// An absent source is not an error. The platform loaders return the +// documented (nil, nil) sentinel for "not enrolled", so only a real read +// failure surfaces here. +func LoadPolicyWithError() (*Policy, error) { + values, err := loadPlatformPolicy() + if err != nil { + return nil, fmt.Errorf("read MDM policy source: %w", err) + } if values == nil { - return &Policy{values: map[string]any{}} + return &Policy{values: map[string]any{}}, nil } if len(values) == 0 { log.Info("MDM enrolled (no managed keys)") } else { log.Infof("MDM enrolled with %d managed key(s): %v", len(values), sortedKeys(values)) } - return &Policy{values: values} + return &Policy{values: values}, nil } // IsEmpty reports whether the Policy has no managed keys. diff --git a/docs/netbird.adml b/docs/netbird.adml index 22a4f14cb..1e00f06cd 100644 --- a/docs/netbird.adml +++ b/docs/netbird.adml @@ -57,7 +57,7 @@ Split tunnel Restrict the NetBird tunnel to or from a chosen list of application package names. Choose either the allow mode (only the listed apps route through NetBird) or the disallow mode (the listed apps bypass NetBird; everything else routes through). The mode is mutually exclusive — only one can be active at a time. Android-only at the daemon level; Windows/macOS/iOS clients ignore this policy. Restrict the daemon sockets - Restrict the NetBird daemon control pipe, and the JSON socket where it is enabled, to the listed principals. Accounts outside them cannot connect at all, so nothing the daemon exposes is reachable from them; LocalSystem and elevated administrators are never locked out. Enter security identifiers in sid:S-1-5-21-... form, comma-separated, because the daemon reads this while starting and must not wait on a domain controller to resolve a name. An empty value lifts a restriction that was set when the service was installed. Windows and macOS only; on macOS the entries are Unix group IDs in gid:1001 form. + Restrict the NetBird daemon control pipe, and the JSON socket where it is enabled, to the listed principals. Accounts outside them cannot connect at all, so nothing the daemon exposes is reachable from them; LocalSystem and elevated administrators are never locked out. Enter security identifiers in sid:S-1-5-21-... form, comma-separated, because the daemon reads this while starting and must not wait on a domain controller to resolve a name. An empty value lifts a restriction that was set when the service was installed. Allow only listed apps (everything else bypasses) Disallow listed apps (everything else routes)