[client] Validate the saved service parameters and pin the netsh lookup (#7584)

* [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 <stateDir>/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

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

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

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

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

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"
-}

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"
-}

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.
 //

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

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