From 6e17f50040dcf3203178f5536020850c711091ab Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:47:48 +0200 Subject: [PATCH] [client] Validate the saved service parameters and pin the netsh lookup (#7584) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [client] Export the only-owner-writable path check from elevate Pure refactor, no behavior change: the existing checkOnlyOwnerWritable gets a thin exported wrapper so callers outside the elevation path can reuse it. No call site changes here. * [client] Validate the saved service parameters before applying them The install reads /service.json and applies it to the service it then registers: its arguments, its config path and its environment. The restricted ACL that saveServiceParams puts on the state directory is applied when the file is written, which is not necessarily before the file is first read, so the install now checks the file rather than assuming it. A file whose ownership or permissions are not the ones saveServiceParams produces is treated as absent, and the install proceeds with its defaults. The check covers the directories above the file as well, so what is checked is what is read. * [client] Restrict which environment variables the service is registered with --service-env, and the service.json it persists to, accepted any name. A small set of them decides how a process resolves the executables and libraries it loads, and the daemon needs none of those: it now refuses them when they are passed explicitly, and drops them with a warning when they come back from a service.json written by an older version, so an upgrade does not fail over a variable nobody needs. * [client] Resolve netsh by absolute path The lookup consulted PATH first and fell back to System32, in both the copy the userspace firewall uses and the one that tears the interface down. It now asks Windows for the system directory, so the resolution no longer depends on the environment the service happens to be started with. * [client] Move the System32 lookup into a package both callers share Pure refactor, no behavior change: client/iface and client/firewall/uspfilter carried a copy each of the same function, and neither imports the other, so the body moves to client/internal/wincmd — alongside winregistry, which is where the client's other Windows-only helper already lives. Both call sites now read wincmd.System32("netsh"). * [client] Cover the System32 lookup with a test Asserts what the previous commits changed: the lookup is absolute, and neither PATH nor %SystemRoot% moves it. * [client] Refuse the loader environment families by prefix Review follow-up on the previous commit: - LD_* and DYLD_* are now refused whole rather than name by name. Their members differ per platform and libc and grow with new OS releases, so a list of them is out of date as soon as it is written — DYLD_FALLBACK_LIBRARY_PATH and DYLD_FALLBACK_FRAMEWORK_PATH were already missing from it. - The names are folded to upper case only on Windows, where a variable is the same one however it is spelled. Elsewhere the environment is case-sensitive, so Path and PATH are two variables and only the exact spelling is the one that is read; the fold refused the wrong one. - TEMP and TMP stay in the denylist, but the rationale and the message now say what they actually decide: where the service writes, not what it loads. --- client/cmd/service.go | 50 ++++++++++++++++ client/cmd/service_params.go | 56 ++++++++++++++++-- client/cmd/service_params_test.go | 54 ++++++++++++++++++ client/cmd/service_params_trust_test.go | 57 +++++++++++++++++++ .../uspfilter/interface_allower_windows.go | 20 ++----- client/iface/iface_destroy_windows.go | 17 +----- client/internal/elevate/trusted.go | 11 ++++ client/internal/wincmd/system32_windows.go | 30 ++++++++++ .../internal/wincmd/system32_windows_test.go | 31 ++++++++++ 9 files changed, 289 insertions(+), 37 deletions(-) create mode 100644 client/cmd/service_params_trust_test.go create mode 100644 client/internal/wincmd/system32_windows.go create mode 100644 client/internal/wincmd/system32_windows_test.go diff --git a/client/cmd/service.go b/client/cmd/service.go index 7410d60ea..2a558e6d5 100644 --- a/client/cmd/service.go +++ b/client/cmd/service.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "runtime" + "slices" "strings" "sync" @@ -25,6 +26,30 @@ var serviceCmd = &cobra.Command{ const defaultJSONSocket = "unix:///var/run/netbird-http.sock" +// forbiddenServiceEnvVars are the environment variables the service is never +// registered with, keyed in upper case since these are Windows names. Each one +// decides where the daemon resolves something it then uses with the privileges +// of the account it runs under — LocalSystem on Windows, root elsewhere: the +// executables it runs (PATH, PATHEXT, COMSPEC, SystemRoot, windir) or the +// directory it writes temporary files in (TEMP, TMP). The daemon needs none of +// them, and the utilities it shells out to are resolved by absolute path. +var forbiddenServiceEnvVars = map[string]struct{}{ + "PATH": {}, + "PATHEXT": {}, + "SYSTEMROOT": {}, + "WINDIR": {}, + "COMSPEC": {}, + "TEMP": {}, + "TMP": {}, +} + +// forbiddenServiceEnvPrefixes are the dynamic-loader families, refused whole +// rather than by name: LD_PRELOAD, DYLD_INSERT_LIBRARIES and their siblings all +// reach the loader of the process, the set differs per platform and libc, and +// new members arrive with new OS releases. Listing them one by one is a list +// that is wrong the moment it is written. +var forbiddenServiceEnvPrefixes = []string{"LD_", "DYLD_"} + var ( serviceName string serviceEnvVars []string @@ -127,8 +152,33 @@ func parseServiceEnvVars(envVars []string) (map[string]string, error) { return nil, fmt.Errorf("empty environment variable key in: %s", env) } + if isForbiddenServiceEnvVar(key) { + return nil, fmt.Errorf("environment variable %s cannot be set on the service: it decides where the service resolves the executables, libraries or temporary files it uses", key) + } + envMap[key] = value } return envMap, nil } + +// isForbiddenServiceEnvVar reports whether name is one the service must not be +// registered with. +// +// The names are matched case-insensitively only on Windows, where they are the +// same variable however they are spelled. Elsewhere the environment is +// case-sensitive, so Path and PATH are two different variables and only the +// exact spelling is the one the loader reads. +func isForbiddenServiceEnvVar(name string) bool { + if runtime.GOOS == "windows" { + name = strings.ToUpper(name) + } + + if _, forbidden := forbiddenServiceEnvVars[name]; forbidden { + return true + } + + return slices.ContainsFunc(forbiddenServiceEnvPrefixes, func(prefix string) bool { + return strings.HasPrefix(name, prefix) + }) +} diff --git a/client/cmd/service_params.go b/client/cmd/service_params.go index 750b22ae6..6e2dbec40 100644 --- a/client/cmd/service_params.go +++ b/client/cmd/service_params.go @@ -14,6 +14,7 @@ import ( "github.com/netbirdio/netbird/client/configs" "github.com/netbirdio/netbird/client/internal/daemonaddr" + "github.com/netbirdio/netbird/client/internal/elevate" "github.com/netbirdio/netbird/util" ) @@ -43,10 +44,33 @@ func serviceParamsPath() string { // loadServiceParams reads saved service parameters from disk. // Returns nil with no error if the file does not exist. +// +// The file is read by an elevated install and decides the arguments and the +// environment of the service it then registers, so it is used only when its +// ownership and permissions are the ones saveServiceParams leaves behind. That +// restricted ACL is applied when the file is written, which is not necessarily +// before it is first read, so this is checked rather than assumed. A file that +// fails the check is treated as absent, and the install proceeds with its +// defaults. func loadServiceParams() (*serviceParams, error) { path := serviceParamsPath() - data, err := os.ReadFile(path) + // Resolve links first so the checks apply to the file that is actually read. + // Since the check covers every directory above it as well, nobody who fails + // it can swap the file between here and the read below. + resolved, err := filepath.EvalSymlinks(path) + if err != nil { + if os.IsNotExist(err) { + return nil, nil //nolint:nilnil + } + return nil, fmt.Errorf("resolve service params %s: %w", path, err) + } + + if err := elevate.CheckOnlyOwnerWritable(resolved); err != nil { + return nil, fmt.Errorf("refusing to read service params from %s: %w", resolved, err) + } + + data, err := os.ReadFile(resolved) if err != nil { if os.IsNotExist(err) { return nil, nil //nolint:nilnil @@ -182,10 +206,16 @@ func applyServiceParams(cmd *cobra.Command, params *serviceParams) { // If --service-env was explicitly set to empty, all saved env vars are cleared. // If --service-env was not set, saved env vars are used entirely. func applyServiceEnvParams(cmd *cobra.Command, params *serviceParams) { + // A forbidden name explicitly passed on the command line is an error the + // operator is told about, but one restored from a file written by an older + // version is dropped: an install that refuses to run would leave the host + // without a daemon over a variable nobody is asking for any more. + saved := dropForbiddenServiceEnvVars(cmd, params.ServiceEnvVars) + if !cmd.Flags().Changed("service-env") { - if len(params.ServiceEnvVars) > 0 { + if len(saved) > 0 { // No explicit env vars: rebuild serviceEnvVars from saved params. - serviceEnvVars = envMapToSlice(params.ServiceEnvVars) + serviceEnvVars = envMapToSlice(saved) } return } @@ -204,13 +234,13 @@ func applyServiceEnvParams(cmd *cobra.Command, params *serviceParams) { return } - if len(params.ServiceEnvVars) == 0 { + if len(saved) == 0 { return } // Merge saved values underneath explicit ones. - merged := make(map[string]string, len(params.ServiceEnvVars)+len(explicit)) - maps.Copy(merged, params.ServiceEnvVars) + merged := make(map[string]string, len(saved)+len(explicit)) + maps.Copy(merged, saved) maps.Copy(merged, explicit) // explicit wins on conflict serviceEnvVars = envMapToSlice(merged) } @@ -233,6 +263,20 @@ var resetParamsCmd = &cobra.Command{ }, } +// dropForbiddenServiceEnvVars returns the saved entries that may still be +// registered on the service, reporting every one it leaves behind. +func dropForbiddenServiceEnvVars(cmd *cobra.Command, saved map[string]string) map[string]string { + kept := make(map[string]string, len(saved)) + for key, value := range saved { + if isForbiddenServiceEnvVar(key) { + cmd.PrintErrf("Warning: ignoring saved service environment variable %s: it decides where the service resolves the executables, libraries or temporary files it uses\n", key) + continue + } + kept[key] = value + } + return kept +} + // envMapToSlice converts a map of env vars to a KEY=VALUE slice. func envMapToSlice(m map[string]string) []string { s := make([]string, 0, len(m)) diff --git a/client/cmd/service_params_test.go b/client/cmd/service_params_test.go index 94f98a0ce..1f83374cb 100644 --- a/client/cmd/service_params_test.go +++ b/client/cmd/service_params_test.go @@ -9,6 +9,7 @@ import ( "go/token" "os" "path/filepath" + "runtime" "strings" "testing" @@ -353,6 +354,59 @@ func TestApplyServiceEnvParams_NotChanged(t *testing.T) { assert.Equal(t, map[string]string{"FROM_SAVED": "val"}, result) } +func TestParseServiceEnvVars_RejectsForbiddenNames(t *testing.T) { + for _, env := range []string{"PATH=C:\\somewhere", "LD_PRELOAD=/tmp/lib.so", "DYLD_FALLBACK_LIBRARY_PATH=/tmp"} { + _, err := parseServiceEnvVars([]string{"KEEP=me", env}) + require.Errorf(t, err, "%s selects what the service resolves and must be refused", env) + } +} + +func TestIsForbiddenServiceEnvVar(t *testing.T) { + // The loader families are matched by prefix, so a name nobody has heard of + // yet is refused too. + for _, name := range []string{ + "PATH", "PATHEXT", "COMSPEC", "SYSTEMROOT", "WINDIR", "TEMP", "TMP", + "LD_PRELOAD", "LD_AUDIT", "DYLD_INSERT_LIBRARIES", "DYLD_FALLBACK_FRAMEWORK_PATH", + } { + assert.Truef(t, isForbiddenServiceEnvVar(name), "%s must be refused", name) + } + + // The prefix must not swallow names that merely start with the same letters. + for _, name := range []string{"NB_LOG_LEVEL", "NB_WG_DEBUG", "HTTPS_PROXY", "LDAP_URL", "DYLDX"} { + assert.Falsef(t, isForbiddenServiceEnvVar(name), "%s has no reason to be refused", name) + } + + // On Windows a variable is the same one however it is spelled; elsewhere + // Path and PATH are two variables and only the exact one is read. + if runtime.GOOS == "windows" { + assert.True(t, isForbiddenServiceEnvVar("Path")) + assert.True(t, isForbiddenServiceEnvVar("ld_preload")) + } else { + assert.False(t, isForbiddenServiceEnvVar("Path")) + assert.False(t, isForbiddenServiceEnvVar("ld_preload")) + } +} + +func TestApplyServiceEnvParams_DropsForbiddenSavedNames(t *testing.T) { + origServiceEnvVars := serviceEnvVars + t.Cleanup(func() { serviceEnvVars = origServiceEnvVars }) + + serviceEnvVars = nil + + cmd := &cobra.Command{} + cmd.Flags().StringSlice("service-env", nil, "") + + saved := &serviceParams{ + ServiceEnvVars: map[string]string{"PATH": "C:\\attacker", "NB_LOG_FORMAT": "json"}, + } + + applyServiceEnvParams(cmd, saved) + + result, err := parseServiceEnvVars(serviceEnvVars) + require.NoError(t, err, "a saved PATH must be dropped rather than fail the install") + assert.Equal(t, map[string]string{"NB_LOG_FORMAT": "json"}, result) +} + func TestApplyServiceEnvParams_ExplicitEmptyClears(t *testing.T) { origServiceEnvVars := serviceEnvVars t.Cleanup(func() { serviceEnvVars = origServiceEnvVars }) diff --git a/client/cmd/service_params_trust_test.go b/client/cmd/service_params_trust_test.go new file mode 100644 index 000000000..1cf564445 --- /dev/null +++ b/client/cmd/service_params_trust_test.go @@ -0,0 +1,57 @@ +//go:build !windows && !ios && !android + +package cmd + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/client/configs" +) + +// The Windows equivalent of this is the ACL check in +// elevate.CheckOnlyOwnerWritable, covered by that package's own tests; here the +// point is that loadServiceParams asks the question at all. +func TestLoadServiceParams_RefusesWorldWritableFile(t *testing.T) { + tmpDir := t.TempDir() + + original := configs.StateDir + t.Cleanup(func() { configs.StateDir = original }) + configs.StateDir = tmpDir + + path := filepath.Join(tmpDir, serviceParamsFile) + require.NoError(t, os.WriteFile(path, []byte(`{"log_level":"debug"}`), 0o666)) + // WriteFile is subject to the umask, so set the bits that matter explicitly. + require.NoError(t, os.Chmod(path, 0o666)) + + params, err := loadServiceParams() + require.Error(t, err, "a service.json anyone can rewrite must not be trusted") + assert.Nil(t, params) + + require.NoError(t, os.Chmod(path, 0o600)) + params, err = loadServiceParams() + require.NoError(t, err) + require.NotNil(t, params) + assert.Equal(t, "debug", params.LogLevel) +} + +func TestLoadServiceParams_RefusesWorldWritableDirectory(t *testing.T) { + tmpDir := t.TempDir() + stateDir := filepath.Join(tmpDir, "state") + require.NoError(t, os.Mkdir(stateDir, 0o777)) + require.NoError(t, os.Chmod(stateDir, 0o777)) + + original := configs.StateDir + t.Cleanup(func() { configs.StateDir = original }) + configs.StateDir = stateDir + + require.NoError(t, os.WriteFile(filepath.Join(stateDir, serviceParamsFile), []byte(`{}`), 0o600)) + + params, err := loadServiceParams() + require.Error(t, err, "a service.json in a directory anyone can replace entries in must not be trusted") + assert.Nil(t, params) +} diff --git a/client/firewall/uspfilter/interface_allower_windows.go b/client/firewall/uspfilter/interface_allower_windows.go index 7f525e28c..4cd0fe969 100644 --- a/client/firewall/uspfilter/interface_allower_windows.go +++ b/client/firewall/uspfilter/interface_allower_windows.go @@ -9,6 +9,7 @@ import ( log "github.com/sirupsen/logrus" nberrors "github.com/netbirdio/netbird/client/errors" + "github.com/netbirdio/netbird/client/internal/wincmd" ) type action string @@ -91,7 +92,7 @@ func manageFirewallRule(ruleName string, action action, extraArgs ...string) err if action == addRule { args = append(args, extraArgs...) } - netshCmd := GetSystem32Command("netsh") + netshCmd := wincmd.System32("netsh") cmd := exec.Command(netshCmd, args...) cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} return cmd.Run() @@ -100,7 +101,7 @@ func manageFirewallRule(ruleName string, action action, extraArgs ...string) err func isWindowsFirewallReachable() bool { args := []string{"advfirewall", "show", "allprofiles", "state"} - netshCmd := GetSystem32Command("netsh") + netshCmd := wincmd.System32("netsh") cmd := exec.Command(netshCmd, args...) cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} @@ -117,23 +118,10 @@ func isWindowsFirewallReachable() bool { func isFirewallRuleActive(ruleName string) bool { args := []string{"advfirewall", "firewall", "show", "rule", "name=" + ruleName} - netshCmd := GetSystem32Command("netsh") + netshCmd := wincmd.System32("netsh") cmd := exec.Command(netshCmd, args...) cmd.SysProcAttr = &syscall.SysProcAttr{HideWindow: true} _, err := cmd.Output() return err == nil } - -// GetSystem32Command checks if a command can be found in the system path and returns it. In case it can't find it -// in the path it will return the full path of a command assuming C:\windows\system32 as the base path. -func GetSystem32Command(command string) string { - _, err := exec.LookPath(command) - if err == nil { - return command - } - - log.Tracef("Command %s not found in PATH, using C:\\windows\\system32\\%s.exe path", command, command) - - return "C:\\windows\\system32\\" + command + ".exe" -} diff --git a/client/iface/iface_destroy_windows.go b/client/iface/iface_destroy_windows.go index 0bfa4e211..54c0014c4 100644 --- a/client/iface/iface_destroy_windows.go +++ b/client/iface/iface_destroy_windows.go @@ -6,27 +6,14 @@ import ( "fmt" "os/exec" - log "github.com/sirupsen/logrus" + "github.com/netbirdio/netbird/client/internal/wincmd" ) func (w *WGIface) Destroy() error { - netshCmd := GetSystem32Command("netsh") + netshCmd := wincmd.System32("netsh") out, err := exec.Command(netshCmd, "interface", "set", "interface", w.Name(), "admin=disable").CombinedOutput() if err != nil { return fmt.Errorf("failed to remove interface %s: %w - %s", w.Name(), err, out) } return nil } - -// GetSystem32Command checks if a command can be found in the system path and returns it. In case it can't find it -// in the path it will return the full path of a command assuming C:\windows\system32 as the base path. -func GetSystem32Command(command string) string { - _, err := exec.LookPath(command) - if err == nil { - return command - } - - log.Tracef("Command %s not found in PATH, using C:\\windows\\system32\\%s.exe path", command, command) - - return "C:\\windows\\system32\\" + command + ".exe" -} diff --git a/client/internal/elevate/trusted.go b/client/internal/elevate/trusted.go index c11054c45..98e05fde5 100644 --- a/client/internal/elevate/trusted.go +++ b/client/internal/elevate/trusted.go @@ -6,6 +6,17 @@ import ( "path/filepath" ) +// CheckOnlyOwnerWritable reports an error unless path, and every directory +// leading to it, is owned by an account that can already act with the privileges +// the caller holds, and is writable by nobody else. +// +// Exported for callers outside elevation that read a file while privileged and +// then act on what it says: the same question this package asks of an +// executable, asked of a configuration file. +func CheckOnlyOwnerWritable(path string) error { + return checkOnlyOwnerWritable(path) +} + // trustedSelf returns the path of this executable, provided it is one we are // willing to have run as root. // diff --git a/client/internal/wincmd/system32_windows.go b/client/internal/wincmd/system32_windows.go new file mode 100644 index 000000000..36aa258b5 --- /dev/null +++ b/client/internal/wincmd/system32_windows.go @@ -0,0 +1,30 @@ +// Package wincmd locates the Windows utilities the client shells out to. +package wincmd + +import ( + "path/filepath" + + log "github.com/sirupsen/logrus" + "golang.org/x/sys/windows" +) + +// defaultSystem32Dir is where the system directory is on every supported +// install, used only when the API that reports it fails. +const defaultSystem32Dir = `C:\Windows\System32` + +// System32 returns the full path of a Windows utility under the system +// directory. +// +// PATH is deliberately not consulted. The daemon runs as LocalSystem with an +// environment of its own, so whoever can place an entry in that PATH chooses +// which binary runs with those privileges. The system directory is read from +// the API rather than from %SystemRoot% for the same reason. +func System32(command string) string { + sysDir, err := windows.GetSystemDirectory() + if err != nil { + log.Warnf("Failed to locate the Windows system directory, falling back to %s: %v", defaultSystem32Dir, err) + sysDir = defaultSystem32Dir + } + + return filepath.Join(sysDir, command+".exe") +} diff --git a/client/internal/wincmd/system32_windows_test.go b/client/internal/wincmd/system32_windows_test.go new file mode 100644 index 000000000..0d31d7ee7 --- /dev/null +++ b/client/internal/wincmd/system32_windows_test.go @@ -0,0 +1,31 @@ +package wincmd + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSystem32IgnoresPATH(t *testing.T) { + // A directory holding something that would win a PATH lookup, in front of + // everything else: the daemon runs as LocalSystem, so a PATH entry must not + // be able to decide what it executes. + planted := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(planted, "netsh.exe"), []byte("not really netsh"), 0o600)) + t.Setenv("PATH", planted+string(os.PathListSeparator)+os.Getenv("PATH")) + + got := System32("netsh") + + assert.True(t, filepath.IsAbs(got), "the path must be absolute, got %q", got) + assert.NotContains(t, got, planted, "a PATH entry must not be consulted") + assert.True(t, strings.EqualFold(filepath.Base(got), "netsh.exe"), "unexpected file name in %q", got) + + // The system directory is what Windows reports it to be, not %SystemRoot%, + // which the same caller could have set alongside PATH. + t.Setenv("SystemRoot", planted) + assert.Equal(t, got, System32("netsh"), "%SystemRoot% must not move the lookup") +}