Merge branch 'main' into profile-ownership

client/cmd/service.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"net/http"
 	"runtime"
+	"slices"
 	"strings"
 	"sync"
 
@@ -26,6 +27,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
@@ -146,8 +171,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"
 )
 
@@ -50,10 +51,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
@@ -199,10 +223,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
 	}
@@ -221,13 +251,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)
 }
@@ -250,6 +280,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/dns/host_windows.go

@@ -124,19 +124,9 @@ func newHostManager(wgInterface WGIface) (*registryConfigurator, error) {
 		return nil, err
 	}
 
-	var useGPO bool
-	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
-	if err != nil {
-		log.Debugf("failed to open GPO DNS policy root: %v", err)
-	} else {
-		closer(k)
-		useGPO = true
-		log.Infof("detected GPO DNS policy configuration, using policy store")
-	}
-
 	configurator := &registryConfigurator{
 		guid: guid,
-		gpo:  useGPO,
+		gpo:  useGPOPolicyStore(),
 	}
 
 	origNameservers, err := configurator.captureOriginalNameservers()
@@ -576,14 +566,22 @@ func (r *registryConfigurator) setInterfaceRegistryKeyStringValue(key, value str
 	return nil
 }
 
+// deleteInterfaceRegistryKeyProperty removes a value from the interface key.
+// A value that is already gone, or an interface key that is, is not an error:
+// the caller asked for the value not to be there, and a cleanup that runs twice
+// has to reach its later steps on the second run as well.
 func (r *registryConfigurator) deleteInterfaceRegistryKeyProperty(propertyKey string) error {
 	regKey, err := r.getInterfaceRegistryKey()
-	if err != nil {
+	switch {
+	case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND):
+		log.Debugf("interface key of %s does not exist, nothing to delete %s from", r.guid, propertyKey)
+		return nil
+	case err != nil:
 		return fmt.Errorf("get interface registry key: %w", err)
 	}
 	defer closer(regKey)
 
-	if err := regKey.DeleteValue(propertyKey); err != nil {
+	if err := regKey.DeleteValue(propertyKey); err != nil && !errors.Is(err, registry.ErrNotExist) {
 		return fmt.Errorf("delete registry key %s: %w", propertyKey, err)
 	}
 	return nil
@@ -612,7 +610,12 @@ func (r *registryConfigurator) restoreHostDNS() error {
 
 	go r.flushDNSCache()
 
-	return nil
+	// Last, and only on the way out, once no rule of ours is left: during a
+	// session the store is where the rules of this run live, and emptying it
+	// mid-session would have the next rule recreate it anyway. Propagated so a
+	// failure keeps the shutdown state for the next run to retry, rather than
+	// leaving the store to hold up every rule change from here on.
+	return removeEmptyGPOPolicyStore()
 }
 
 // removeDNSMatchPolicies deletes every NRPT rule this client may have created,
@@ -651,6 +654,73 @@ func (r *registryConfigurator) restoreUncleanShutdownDNS() error {
 	return r.restoreHostDNS()
 }
 
+// useGPOPolicyStore reports whether NRPT rules have to go into the group policy
+// store, and clears an empty one out of the way first.
+//
+// The order is the point. A store left empty by an earlier run would otherwise
+// decide this run too, sending its rules somewhere the resolver only reads when
+// the policy engine next applies DNS client policy. Removing it before the
+// choice is made leaves the local store authoritative for the whole session,
+// including the first one after an upgrade.
+func useGPOPolicyStore() bool {
+	if err := removeEmptyGPOPolicyStore(); err != nil {
+		// Nothing to retry against here: the worst case is the run going
+		// through the group policy store, which is where it would have gone
+		// before this check existed.
+		log.Warnf("%v", err)
+	}
+
+	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
+	if err != nil {
+		log.Debugf("failed to open GPO DNS policy root: %v", err)
+		return false
+	}
+	closer(k)
+
+	log.Infof("detected GPO DNS policy configuration, using policy store")
+	return true
+}
+
+// removeEmptyGPOPolicyStore deletes the group policy DnsPolicyConfig key once
+// nothing is left in it. The key survives the deletion of the last rule it
+// held, and the client treats its presence as "group policy configures the
+// NRPT", so an empty one left behind keeps every later run writing rules there.
+// Rules in that store reach the resolver only when the policy engine next
+// applies DNS client policy, and a rule this client writes belongs to no GPO,
+// so nothing schedules that application: both adding and removing a rule are
+// held up by a minute or more, and for a removal that is a catch-all rule
+// resolving every name over an interface that no longer exists. With the store
+// absent the local one is authoritative and a change applies at once.
+//
+// A store that still holds rules, values or subkeys of somebody else's is left
+// alone.
+func removeEmptyGPOPolicyStore() error {
+	k, err := registry.OpenKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.QUERY_VALUE)
+	switch {
+	case errors.Is(err, registry.ErrNotExist), errors.Is(err, syscall.ERROR_PATH_NOT_FOUND):
+		return nil
+	case err != nil:
+		return fmt.Errorf("open HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	info, err := k.Stat()
+	closer(k)
+	if err != nil {
+		return fmt.Errorf("stat HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	if info.SubKeyCount != 0 || info.ValueCount != 0 {
+		return nil
+	}
+
+	if err := registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot); err != nil {
+		return fmt.Errorf("delete empty HKEY_LOCAL_MACHINE\\%s: %w", GPODNSPolicyConfigRoot, err)
+	}
+
+	log.Infof("removed the empty GPO DNS policy store, leaving the local one authoritative")
+	return nil
+}
+
 // listNRPTRuleKeys returns the names of our NRPT rule keys under a policy store
 // root. An absent root holds nothing to clean up, which is the normal state of
 // the GPO store on a machine without DNS Client policy.

client/internal/dns/host_windows_test.go

@@ -8,6 +8,8 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sys/windows/registry"
+
+	"github.com/netbirdio/netbird/client/internal/winregistry"
 )
 
 // TestNRPTEntriesCleanupOnConfigChange tests that old NRPT entries are properly cleaned up
@@ -405,3 +407,130 @@ func TestNRPTDomainBatching(t *testing.T) {
 		})
 	}
 }
+
+// TestRemoveEmptyGPOPolicyStore verifies that cleanup takes the GPO policy
+// store itself with it once our rules are gone, since the store existing keeps
+// the local one from being applied, and that a store with somebody else's rule
+// in it is left alone.
+func TestRemoveEmptyGPOPolicyStore(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	t.Cleanup(func() { cleanupRegistryKeys(t) })
+	cleanupRegistryKeys(t)
+
+	testIP := netip.MustParseAddr("100.64.0.1")
+	cfg := &registryConfigurator{gpo: true}
+
+	// a store holding a rule of ours is kept, because the rule is still applied
+	require.NoError(t, cfg.addDNSMatchPolicy([]string{".example.com"}, testIP))
+	exists, err := registryKeyExists(gpoDnsPolicyConfigMatchPath + "-0")
+	require.NoError(t, err)
+	require.True(t, exists, "Should write the rule to the GPO policy store")
+
+	require.NoError(t, removeEmptyGPOPolicyStore())
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a policy store that still holds a rule")
+
+	// once the rules are gone the store goes with them
+	require.NoError(t, cfg.removeDNSMatchPolicies())
+	require.NoError(t, removeEmptyGPOPolicyStore())
+
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.False(t, exists, "Should remove the GPO policy store once it is empty")
+
+	// A store is not ours to remove while somebody else has a rule in it. The
+	// rule is written volatile like our own: the rules above created the parent
+	// chain volatile, and Windows refuses a stable subkey under a volatile
+	// parent.
+	foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}`
+	foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE)
+	require.NoError(t, err, "Should create a foreign GPO rule")
+	foreignKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule)
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot)
+	})
+
+	require.NoError(t, cfg.removeDNSMatchPolicies())
+	require.NoError(t, removeEmptyGPOPolicyStore())
+
+	exists, err = registryKeyExists(foreignRule)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should not remove a foreign rule")
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a policy store that still holds a foreign rule")
+}
+
+// TestDeleteInterfaceRegistryKeyPropertyTwice verifies that removing a value
+// that is already gone, or one on an interface key that is, reports success.
+// Teardown runs again after a failed cleanup, and the steps that follow this
+// one have to be reached on that second run.
+func TestDeleteInterfaceRegistryKeyPropertyTwice(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	testGUID := "{12345678-1234-1234-1234-123456789ABC}"
+	interfacePath := InterfaceConfigPath + `\` + testGUID
+	testKey, _, err := registry.CreateKey(registry.LOCAL_MACHINE, interfacePath, registry.SET_VALUE)
+	require.NoError(t, err, "Should create test interface registry key")
+	testKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath)
+	})
+
+	cfg := &registryConfigurator{guid: testGUID}
+
+	require.NoError(t, cfg.setInterfaceRegistryKeyStringValue(interfaceConfigSearchListKey, "example.com"))
+	require.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey))
+	assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey),
+		"Should report success for a value that is already gone")
+
+	// and with the interface key itself gone, as it is once the adapter is
+	require.NoError(t, registry.DeleteKey(registry.LOCAL_MACHINE, interfacePath))
+	assert.NoError(t, cfg.deleteInterfaceRegistryKeyProperty(interfaceConfigSearchListKey),
+		"Should report success when the interface key does not exist")
+}
+
+// TestUseGPOPolicyStoreClearsEmptyStore verifies that the store is cleared
+// before it is consulted, so an empty one left by an earlier run does not send
+// this run's rules to the group policy store. A store somebody else has a rule
+// in still decides where the rules go.
+func TestUseGPOPolicyStoreClearsEmptyStore(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping registry integration test in short mode")
+	}
+
+	t.Cleanup(func() { cleanupRegistryKeys(t) })
+	cleanupRegistryKeys(t)
+
+	// the leftover an earlier run used to keep, which the client read as
+	// "group policy configures the NRPT" for every run after it
+	emptyStore, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot, registry.SET_VALUE)
+	require.NoError(t, err, "Should create the GPO policy store")
+	emptyStore.Close()
+
+	assert.False(t, useGPOPolicyStore(), "An empty store should not decide where the rules go")
+	exists, err := registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.False(t, exists, "Should clear the empty store before consulting it")
+
+	foreignRule := GPODNSPolicyConfigRoot + `\{2A3B4C5D-6E7F-4041-8283-84858687888A}`
+	foreignKey, _, err := winregistry.CreateVolatileKey(registry.LOCAL_MACHINE, foreignRule, registry.SET_VALUE)
+	require.NoError(t, err, "Should create a foreign GPO rule")
+	foreignKey.Close()
+	t.Cleanup(func() {
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, foreignRule)
+		_ = registry.DeleteKey(registry.LOCAL_MACHINE, GPODNSPolicyConfigRoot)
+	})
+
+	assert.True(t, useGPOPolicyStore(), "A store holding a rule should decide where the rules go")
+	exists, err = registryKeyExists(GPODNSPolicyConfigRoot)
+	require.NoError(t, err)
+	assert.True(t, exists, "Should keep a store that holds a rule")
+}

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

@@ -1061,7 +1061,11 @@ func (e *Engine) handleSync(update *mgmProto.SyncResponse) error {
 			// back to empty if the FQDN doesn't have the expected shape.
 			dnsName = extractDNSDomainFromFQDN(pc.GetFqdn())
 		}
-		result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName)
+		// With the firewall disabled there is no ACL manager to program, so
+		// RoutesFirewallRules would be built and then dropped. On a peer that
+		// routes many network resources that is the single most expensive
+		// step of the sync.
+		result, err := nbnetworkmap.EnvelopeToNetworkMap(e.ctx, envelope, localKey, dnsName, e.config.DisableFirewall)
 		if err != nil {
 			return fmt.Errorf("decode network map envelope: %w", err)
 		}

client/internal/peer/conn.go

@@ -135,9 +135,10 @@ type Conn struct {
 	// used to store the remote Rosenpass key for Relayed connection in case of connection update from ice
 	rosenpassRemoteKey []byte
 
-	wgProxyICE   wgproxy.Proxy
-	wgProxyRelay wgproxy.Proxy
-	handshaker   *Handshaker
+	wgProxyICE     wgproxy.Proxy
+	wgProxyRelay   wgproxy.Proxy
+	relayedConnRef *relayClient.Conn
+	handshaker     *Handshaker
 
 	guard *guard.Guard
 	wg    sync.WaitGroup
@@ -560,7 +561,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
 
-	if conn.ctx.Err() != nil {
+	if conn.ctx.Err() != nil || rci.relayedConn.Context().Err() != nil {
 		if err := rci.relayedConn.Close(); err != nil {
 			conn.Log.Warnf("failed to close unnecessary relayed connection: %v", err)
 		}
@@ -575,7 +576,9 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 		conn.Log.Errorf("failed to add relayed net.Conn to local proxy: %v", err)
 		return
 	}
-	wgProxy.SetDisconnectListener(conn.onRelayDisconnected)
+	wgProxy.SetDisconnectListener(func() {
+		conn.onRelayDisconnected(rci.relayedConn)
+	})
 
 	conn.dumpState.NewLocalProxy()
 
@@ -583,7 +586,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 
 	if conn.isICEActive() {
 		conn.Log.Debugf("do not switch to relay because current priority is: %s", conn.currentConnPriority.String())
-		conn.setRelayedProxy(wgProxy)
+		conn.setRelayedProxy(wgProxy, rci.relayedConn)
 		conn.statusRelay.SetConnected()
 		conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, time.Now())
 		return
@@ -614,15 +617,26 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
 	conn.rosenpassRemoteKey = rci.rosenpassPubKey
 	conn.currentConnPriority = conntype.Relay
 	conn.statusRelay.SetConnected()
-	conn.setRelayedProxy(wgProxy)
+	conn.setRelayedProxy(wgProxy, rci.relayedConn)
 	conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, updateTime)
 	conn.Log.Infof("start to communicate with peer via relay")
 	conn.doOnConnected(rci.rosenpassPubKey, rci.rosenpassAddr, updateTime)
 }
 
-func (conn *Conn) onRelayDisconnected() {
+// onRelayDisconnected reports the teardown of a relayed connection. relayedConn
+// names the connection the signal belongs to, so a signal that arrives after
+// its connection was replaced is ignored instead of tearing down its successor.
+// A nil relayedConn means the caller does not track generations and the current
+// connection is always torn down.
+func (conn *Conn) onRelayDisconnected(relayedConn *relayClient.Conn) {
 	conn.mu.Lock()
 	defer conn.mu.Unlock()
+
+	if relayedConn != nil && conn.relayedConnRef != relayedConn {
+		conn.Log.Debugf("ignoring relay disconnect of a superseded connection")
+		return
+	}
+
 	conn.handleRelayDisconnectedLocked()
 }
 
@@ -646,6 +660,7 @@ func (conn *Conn) handleRelayDisconnectedLocked() {
 		_ = conn.wgProxyRelay.CloseConn()
 		conn.wgProxyRelay = nil
 	}
+	conn.relayedConnRef = nil
 
 	changed := conn.statusRelay.Get() != worker.StatusDisconnected
 	if changed {
@@ -930,13 +945,14 @@ func (conn *Conn) logTraceConnState() {
 	}
 }
 
-func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy) {
+func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy, relayedConn *relayClient.Conn) {
 	if conn.wgProxyRelay != nil {
 		if err := conn.wgProxyRelay.CloseConn(); err != nil {
 			conn.Log.Warnf("failed to close deprecated wg proxy conn: %v", err)
 		}
 	}
 	conn.wgProxyRelay = proxy
+	conn.relayedConnRef = relayedConn
 }
 
 // onWGHandshakeSuccess is called when the first WireGuard handshake is detected

client/internal/peer/worker_relay.go

@@ -3,7 +3,6 @@ package peer
 import (
 	"context"
 	"errors"
-	"net"
 	"net/netip"
 	"sync"
 	"sync/atomic"
@@ -14,7 +13,7 @@ import (
 )
 
 type RelayConnInfo struct {
-	relayedConn     net.Conn
+	relayedConn     *relayClient.Conn
 	rosenpassPubKey []byte
 	rosenpassAddr   string
 }
@@ -27,7 +26,7 @@ type WorkerRelay struct {
 	conn         *Conn
 	relayManager *relayClient.Manager
 
-	relayedConn net.Conn
+	relayedConn *relayClient.Conn
 	relayLock   sync.Mutex
 
 	relaySupportedOnRemotePeer atomic.Bool
@@ -80,12 +79,7 @@ func (w *WorkerRelay) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
 	w.relayedConn = relayedConn
 	w.relayLock.Unlock()
 
-	err = w.relayManager.AddCloseListener(srv, w.onRelayClientDisconnected)
-	if err != nil {
-		log.Errorf("failed to add close listener: %s", err)
-		_ = relayedConn.Close()
-		return
-	}
+	go w.watchRelayedConn(relayedConn)
 
 	w.log.Debugf("peer conn opened via Relay: %s", srv)
 	go w.conn.onRelayConnectionIsReady(RelayConnInfo{
@@ -109,12 +103,15 @@ func (w *WorkerRelay) RelayIsSupportedLocally() bool {
 
 func (w *WorkerRelay) CloseConn() {
 	w.relayLock.Lock()
-	defer w.relayLock.Unlock()
-	if w.relayedConn == nil {
+	conn := w.relayedConn
+	w.relayedConn = nil
+	w.relayLock.Unlock()
+
+	if conn == nil {
 		return
 	}
 
-	if err := w.relayedConn.Close(); err != nil {
+	if err := conn.Close(); err != nil {
 		w.log.Warnf("failed to close relay connection: %v", err)
 	}
 }
@@ -133,6 +130,8 @@ func (w *WorkerRelay) preferredRelayServer(myRelayAddress, remoteRelayAddress st
 	return remoteRelayAddress
 }
 
-func (w *WorkerRelay) onRelayClientDisconnected() {
-	go w.conn.onRelayDisconnected()
+func (w *WorkerRelay) watchRelayedConn(relayedConn *relayClient.Conn) {
+	<-relayedConn.Context().Done()
+
+	w.conn.onRelayDisconnected(relayedConn)
 }

client/internal/profilemanager/active_state_test.go

@@ -0,0 +1,76 @@
+package profilemanager
+
+import (
+	"fmt"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// Regression test: a concurrent Get and Set of the ActiveProfileState will
+// fail on Windows since the write is a temp file renamed over an open file.
+// Windows will refuse to replace a file another handle holds open by default.
+func TestActiveProfileState_ReadsDoNotBreakAConcurrentWrite(t *testing.T) {
+	withTempConfigDir(t, func(configDir string) {
+		withPatchedGlobals(t, configDir, func() {
+			sm := &ServiceManager{}
+			require.NoError(t, sm.CreateDefaultProfile())
+			require.NoError(t, sm.SetActiveProfileStateToDefault())
+
+			const switched = ID("0123456789abcdef0123456789abcdef")
+			const rounds = 50
+
+			var wg sync.WaitGroup
+			errs := make(chan error, 128)
+
+			for i := 0; i < 8; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					for r := 0; r < rounds; r++ {
+						state, err := sm.GetActiveProfileState()
+						if err != nil {
+							errs <- fmt.Errorf("read: %w", err)
+							return
+						}
+						if state.ID != defaultProfileName && state.ID != switched {
+							errs <- fmt.Errorf("read: active profile is %q, which no writer wrote", state.ID)
+							return
+						}
+					}
+				}()
+			}
+
+			for i := 0; i < 2; i++ {
+				wg.Add(1)
+				go func() {
+					defer wg.Done()
+					for r := 0; r < rounds; r++ {
+						id := switched
+						if r%2 == 0 {
+							id = defaultProfileName
+						}
+						if err := sm.SetActiveProfileState(&ActiveProfileState{ID: id, Username: "testuser"}); err != nil {
+							errs <- fmt.Errorf("switch: %w", err)
+							return
+						}
+					}
+				}()
+			}
+
+			wg.Wait()
+			close(errs)
+
+			for err := range errs {
+				assert.NoError(t, err, "a switch and a read of the active profile state must not collide")
+			}
+
+			state, err := sm.GetActiveProfileState()
+			require.NoError(t, err)
+			assert.Contains(t, []ID{defaultProfileName, switched}, state.ID,
+				"the file holds whichever switch landed last, not a mix of the two")
+		})
+	})
+}

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