net/dns/manager_darwin.go

@@ -5,7 +5,10 @@ package dns
 
 import (
 	"bytes"
+	"fmt"
+	"io/fs"
 	"os"
+	"strings"
 
 	"go4.org/mem"
 	"tailscale.com/control/controlknobs"
@@ -22,15 +25,22 @@ import (
 //
 // The health tracker, bus and the knobs may be nil and are ignored on this platform.
 func NewOSConfigurator(logf logger.Logf, _ *health.Tracker, _ *eventbus.Bus, _ policyclient.Client, _ *controlknobs.Knobs, ifName string) (OSConfigurator, error) {
-	return &darwinConfigurator{logf: logf, ifName: ifName}, nil
+	return &darwinConfigurator{
+		logf:           logf,
+		ifName:         ifName,
+		resolverDir:    "/etc/resolver",
+		resolvConfPath: "/etc/resolv.conf",
+	}, nil
 }
 
 // darwinConfigurator is the tailscaled-on-macOS DNS OS configurator that
 // maintains the Split DNS nameserver entries pointing MagicDNS DNS suffixes
 // to 100.100.100.100 using the macOS /etc/resolver/$SUFFIX files.
 type darwinConfigurator struct {
-	logf   logger.Logf
+	logf           logger.Logf
-	ifName string
+	ifName         string
+	resolverDir    string // default "/etc/resolver"
+	resolvConfPath string // default "/etc/resolv.conf"
 }
 
 func (c *darwinConfigurator) Close() error {
@@ -51,10 +61,16 @@ func (c *darwinConfigurator) SetDNS(cfg OSConfig) error {
 		buf.WriteString("\n")
 	}
 
-	if err := os.MkdirAll("/etc/resolver", 0755); err != nil {
+	if err := os.MkdirAll(c.resolverDir, 0755); err != nil {
 		return err
 	}
 
+	root, err := os.OpenRoot(c.resolverDir)
+	if err != nil {
+		return err
+	}
+	defer root.Close()
+
 	var keep map[string]bool
 
 	// Add a dummy file to /etc/resolver with a "search ..." directive if we have
@@ -70,7 +86,7 @@ func (c *darwinConfigurator) SetDNS(cfg OSConfig) error {
 			sbuf.WriteString(string(d.WithoutTrailingDot()))
 		}
 		sbuf.WriteString("\n")
-		if err := os.WriteFile("/etc/resolver/"+searchFile, sbuf.Bytes(), 0644); err != nil {
+		if err := root.WriteFile(searchFile, sbuf.Bytes(), 0644); err != nil {
 			return err
 		}
 	}
@@ -78,15 +94,34 @@ func (c *darwinConfigurator) SetDNS(cfg OSConfig) error {
 	for _, d := range cfg.MatchDomains {
 		fileBase := string(d.WithoutTrailingDot())
 		mak.Set(&keep, fileBase, true)
-		fullPath := "/etc/resolver/" + fileBase
 
-		if err := os.WriteFile(fullPath, buf.Bytes(), 0644); err != nil {
+		if !isValidResolverFileName(fileBase) {
+			c.logf("[unexpected] invalid resolver domain %q with slashes or colons", fileBase)
+			return fmt.Errorf("invalid resolver domain %q: must not contain slashes or colons", fileBase)
+		}
+
+		if err := root.WriteFile(fileBase, buf.Bytes(), 0644); err != nil {
 			return err
 		}
 	}
 	return c.removeResolverFiles(func(domain string) bool { return !keep[domain] })
 }
 
+func isValidResolverFileName(name string) bool {
+	// Verify that the filename doesn't contain any characters that
+	// might cause issues when used as a filename; os.Root is a
+	// defense against path traversal, but prefer a nice error here
+	// if we can. These aren't valid for domain names anyway.
+	if strings.Contains(name, "/") || strings.Contains(name, "\\") {
+		return false
+	}
+
+	if strings.Contains(name, ":") {
+		return false
+	}
+	return true
+}
+
 // GetBaseConfig returns the current OS DNS configuration, extracting it from /etc/resolv.conf.
 // We should really be using the SystemConfiguration framework to get this information, as this
 // is not a stable public API, and is provided mostly as a compatibility effort with Unix
@@ -95,9 +130,9 @@ func (c *darwinConfigurator) SetDNS(cfg OSConfig) error {
 func (c *darwinConfigurator) GetBaseConfig() (OSConfig, error) {
 	cfg := OSConfig{}
 
-	resolvConf, err := resolvconffile.ParseFile("/etc/resolv.conf")
+	resolvConf, err := resolvconffile.ParseFile(c.resolvConfPath)
 	if err != nil {
-		c.logf("failed to parse /etc/resolv.conf: %v", err)
+		c.logf("failed to parse %s: %v", c.resolvConfPath, err)
 		return cfg, ErrGetBaseConfigNotSupported
 	}
 
@@ -113,7 +148,7 @@ func (c *darwinConfigurator) GetBaseConfig() (OSConfig, error) {
 
 	if len(cfg.Nameservers) == 0 {
 		// Log a warning in case we couldn't find any nameservers in /etc/resolv.conf.
-		c.logf("no nameservers found in /etc/resolv.conf, DNS resolution might fail")
+		c.logf("no nameservers found in %s, DNS resolution might fail", c.resolvConfPath)
 	}
 
 	return cfg, nil
@@ -124,13 +159,19 @@ const macResolverFileHeader = "# Added by tailscaled\n"
 // removeResolverFiles deletes all files in /etc/resolver for which the shouldDelete
 // func returns true.
 func (c *darwinConfigurator) removeResolverFiles(shouldDelete func(domain string) bool) error {
-	dents, err := os.ReadDir("/etc/resolver")
+	root, err := os.OpenRoot(c.resolverDir)
 	if os.IsNotExist(err) {
 		return nil
 	}
 	if err != nil {
 		return err
 	}
+	defer root.Close()
+
+	dents, err := fs.ReadDir(root.FS(), ".")
+	if err != nil {
+		return err
+	}
 	for _, de := range dents {
 		if !de.Type().IsRegular() {
 			continue
@@ -139,8 +180,7 @@ func (c *darwinConfigurator) removeResolverFiles(shouldDelete func(domain string
 		if !shouldDelete(name) {
 			continue
 		}
-		fullPath := "/etc/resolver/" + name
+		contents, err := root.ReadFile(name)
-		contents, err := os.ReadFile(fullPath)
 		if err != nil {
 			if os.IsNotExist(err) { // race?
 				continue
@@ -150,7 +190,7 @@ func (c *darwinConfigurator) removeResolverFiles(shouldDelete func(domain string
 		if !mem.HasPrefix(mem.B(contents), mem.S(macResolverFileHeader)) {
 			continue
 		}
-		if err := os.Remove(fullPath); err != nil {
+		if err := root.Remove(name); err != nil {
 			return err
 		}
 	}

net/dns/manager_darwin_test.go

@@ -0,0 +1,182 @@
+// Copyright (c) Tailscale Inc & contributors
+// SPDX-License-Identifier: BSD-3-Clause
+
+package dns
+
+import (
+	"errors"
+	"maps"
+	"net/netip"
+	"os"
+	"path/filepath"
+	"slices"
+	"testing"
+
+	"tailscale.com/types/logger"
+	"tailscale.com/util/dnsname"
+)
+
+func newTestConfigurator(t *testing.T) *darwinConfigurator {
+	t.Helper()
+	dir := t.TempDir()
+
+	resolvConf := filepath.Join(dir, "resolv.conf")
+	if err := os.WriteFile(resolvConf, []byte("nameserver 8.8.8.8\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	resolverDir := filepath.Join(dir, "resolvers")
+	if err := os.Mkdir(resolverDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+
+	return &darwinConfigurator{
+		logf:           logger.Discard,
+		ifName:         "utun99",
+		resolverDir:    resolverDir,
+		resolvConfPath: resolvConf,
+	}
+}
+
+func TestSetDNS(t *testing.T) {
+	c := newTestConfigurator(t)
+
+	tests := []struct {
+		name         string
+		cfg          OSConfig
+		fileContents map[string]string // path -> expected file contents
+	}{
+		{
+			name: "basic",
+			cfg: OSConfig{
+				Nameservers:  []netip.Addr{netip.MustParseAddr("100.100.100.100")},
+				MatchDomains: []dnsname.FQDN{"example.com.", "ts.net."},
+			},
+			fileContents: map[string]string{
+				"example.com": macResolverFileHeader + "nameserver 100.100.100.100\n",
+				"ts.net":      macResolverFileHeader + "nameserver 100.100.100.100\n",
+			},
+		},
+		{
+			name: "SearchDomains",
+			cfg: OSConfig{
+				Nameservers:   []netip.Addr{netip.MustParseAddr("100.100.100.100")},
+				SearchDomains: []dnsname.FQDN{"tail1234.ts.net."},
+				MatchDomains:  []dnsname.FQDN{"ts.net."},
+			},
+			fileContents: map[string]string{
+				"ts.net":           macResolverFileHeader + "nameserver 100.100.100.100\n",
+				"search.tailscale": macResolverFileHeader + "search tail1234.ts.net\n",
+			},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if err := c.SetDNS(tt.cfg); err != nil {
+				t.Fatalf("SetDNS failed: %v", err)
+			}
+
+			// We want only the expected files in the resolverDir,
+			// and nothing else.
+			files, err := os.ReadDir(c.resolverDir)
+			if err != nil {
+				t.Fatalf("reading resolver directory: %v", err)
+			}
+
+			var fileNames []string
+			for _, f := range files {
+				fileNames = append(fileNames, f.Name())
+			}
+
+			if len(files) != len(tt.fileContents) {
+				t.Fatalf("expected %d resolver files, got %d\ngot:  %v\nwant: %v",
+					len(tt.fileContents), len(files),
+					fileNames, slices.Collect(maps.Keys(tt.fileContents)),
+				)
+			}
+
+			// Check each file's contents.
+			for domain, expected := range tt.fileContents {
+				path := filepath.Join(c.resolverDir, domain)
+				data, err := os.ReadFile(path)
+				if err != nil {
+					t.Fatalf("reading resolver file %q: %v", domain, err)
+				}
+				if string(data) != expected {
+					t.Errorf("resolver file %q contents mismatch:\ngot:  %q\nwant: %q", domain, string(data), expected)
+				}
+			}
+		})
+	}
+}
+
+func TestSetDNS_PathTraversal(t *testing.T) {
+	c := newTestConfigurator(t)
+
+	// Use a simple path traversal that tries to escape the resolver
+	// directory. With the previously-vulnerable code (os.WriteFile with string
+	// concatenation), this writes to the parent directory. With the
+	// fix (os.Root), this is rejected.
+	traversals := []dnsname.FQDN{
+		"../evil.",
+		"../../evil.",
+		"sub/../../evil.",
+	}
+
+	for _, traversal := range traversals {
+		cfg := OSConfig{
+			Nameservers:  []netip.Addr{netip.MustParseAddr("100.100.100.100")},
+			MatchDomains: []dnsname.FQDN{traversal},
+		}
+
+		if err := c.SetDNS(cfg); err == nil {
+			t.Errorf("SetDNS with MatchDomain %q should have failed, but succeeded", traversal)
+		}
+	}
+
+	// Verify no file named "evil" was written in the parent of resolverDir.
+	parent := filepath.Dir(c.resolverDir)
+	if fileExists(filepath.Join(parent, "evil")) {
+		t.Fatal("file 'evil' was written to parent directory via path traversal")
+	}
+}
+
+func TestRemoveResolverFiles(t *testing.T) {
+	c := newTestConfigurator(t)
+
+	// Write a tailscale-managed file.
+	managed := filepath.Join(c.resolverDir, "ts.net")
+	if err := os.WriteFile(managed, []byte(macResolverFileHeader+"nameserver 100.100.100.100\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Write a non-tailscale file that should be left alone.
+	unmanaged := filepath.Join(c.resolverDir, "other.conf")
+	if err := os.WriteFile(unmanaged, []byte("# not ours\nnameserver 8.8.8.8\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+
+	// Remove all resolver files and verify that only the managed one is removed.
+	if err := c.removeResolverFiles(func(domain string) bool { return true }); err != nil {
+		t.Fatal(err)
+	}
+
+	if fileExists(managed) {
+		t.Error("managed file should have been removed")
+	}
+	if !fileExists(unmanaged) {
+		t.Error("unmanaged file should still exist")
+	}
+}
+
+func fileExists(path string) bool {
+	_, err := os.Stat(path)
+	if errors.Is(err, os.ErrNotExist) {
+		return false
+	} else if err == nil {
+		return true
+	}
+
+	panic("unexpected error checking file existence: " + err.Error())
+}