From c32efd9118bb8ba63ae5729653d1eaeeaad52149 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Mon, 10 Jun 2024 22:05:15 -0500 Subject: [PATCH] various: create a catch-all NRPT rule when "Override local DNS" is enabled on Windows Without this rule, Windows 8.1 and newer devices issue parallel DNS requests to DNS servers associated with all network adapters, even when "Override local DNS" is enabled and/or a Mullvad exit node is being used, resulting in DNS leaks. This also adds "disable-local-dns-override-via-nrpt" nodeAttr that can be used to disable the new behavior if needed. Fixes tailscale/corp#20718 Signed-off-by: Nick Khyl --- cmd/tailscaled/tailscaled.go | 4 ++-- control/controlknobs/controlknobs.go | 12 ++++++++++++ net/dns/manager.go | 2 +- net/dns/manager_darwin.go | 6 +++++- net/dns/manager_default.go | 6 +++++- net/dns/manager_freebsd.go | 6 +++++- net/dns/manager_linux.go | 6 +++++- net/dns/manager_openbsd.go | 6 +++++- net/dns/manager_windows.go | 26 ++++++++++++++++++++++---- net/dns/manager_windows_test.go | 4 ++-- tailcfg/tailcfg.go | 12 +++++++++++- 11 files changed, 75 insertions(+), 15 deletions(-) diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index 713a8d441..a2ab7271c 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -698,7 +698,7 @@ func tryEngine(logf logger.Logf, sys *tsd.System, name string) (onlyNetstack boo // configuration being unavailable (from the noop // manager). More in Issue 4017. // TODO(bradfitz): add a Synology-specific DNS manager. - conf.DNS, err = dns.NewOSConfigurator(logf, sys.HealthTracker(), "") // empty interface name + conf.DNS, err = dns.NewOSConfigurator(logf, sys.HealthTracker(), sys.ControlKnobs(), "") // empty interface name if err != nil { return false, fmt.Errorf("dns.NewOSConfigurator: %w", err) } @@ -726,7 +726,7 @@ func tryEngine(logf logger.Logf, sys *tsd.System, name string) (onlyNetstack boo return false, fmt.Errorf("creating router: %w", err) } - d, err := dns.NewOSConfigurator(logf, sys.HealthTracker(), devName) + d, err := dns.NewOSConfigurator(logf, sys.HealthTracker(), sys.ControlKnobs(), devName) if err != nil { dev.Close() r.Close() diff --git a/control/controlknobs/controlknobs.go b/control/controlknobs/controlknobs.go index f4cef7b41..9ff12c76f 100644 --- a/control/controlknobs/controlknobs.go +++ b/control/controlknobs/controlknobs.go @@ -90,6 +90,15 @@ type Knobs struct { // This is for now (2024-06-06) an iOS-specific battery life optimization, // and this knob allows us to disable the optimization remotely if needed. DisableSplitDNSWhenNoCustomResolvers atomic.Bool + + // DisableLocalDNSOverrideViaNRPT indicates that the node's DNS manager should not + // create a default (catch-all) Windows NRPT rule when "Override local DNS" is enabled. + // Without this rule, Windows 8.1 and newer devices issue parallel DNS requests to DNS servers + // associated with all network adapters, even when "Override local DNS" is enabled and/or + // a Mullvad exit node is being used, resulting in DNS leaks. + // We began creating this rule on 2024-06-14, and this knob + // allows us to disable the new behavior remotely if needed. + DisableLocalDNSOverrideViaNRPT atomic.Bool } // UpdateFromNodeAttributes updates k (if non-nil) based on the provided self @@ -117,6 +126,7 @@ func (k *Knobs) UpdateFromNodeAttributes(capMap tailcfg.NodeCapMap) { appCStoreRoutes = has(tailcfg.NodeAttrStoreAppCRoutes) userDialUseRoutes = has(tailcfg.NodeAttrUserDialUseRoutes) disableSplitDNSWhenNoCustomResolvers = has(tailcfg.NodeAttrDisableSplitDNSWhenNoCustomResolvers) + disableLocalDNSOverrideViaNRPT = has(tailcfg.NodeAttrDisableLocalDNSOverrideViaNRPT) ) if has(tailcfg.NodeAttrOneCGNATEnable) { @@ -142,6 +152,7 @@ func (k *Knobs) UpdateFromNodeAttributes(capMap tailcfg.NodeCapMap) { k.AppCStoreRoutes.Store(appCStoreRoutes) k.UserDialUseRoutes.Store(userDialUseRoutes) k.DisableSplitDNSWhenNoCustomResolvers.Store(disableSplitDNSWhenNoCustomResolvers) + k.DisableLocalDNSOverrideViaNRPT.Store(disableLocalDNSOverrideViaNRPT) } // AsDebugJSON returns k as something that can be marshalled with json.Marshal @@ -168,5 +179,6 @@ func (k *Knobs) AsDebugJSON() map[string]any { "AppCStoreRoutes": k.AppCStoreRoutes.Load(), "UserDialUseRoutes": k.UserDialUseRoutes.Load(), "DisableSplitDNSWhenNoCustomResolvers": k.DisableSplitDNSWhenNoCustomResolvers.Load(), + "DisableLocalDNSOverrideViaNRPT": k.DisableLocalDNSOverrideViaNRPT.Load(), } } diff --git a/net/dns/manager.go b/net/dns/manager.go index b955e5041..5bffc56b8 100644 --- a/net/dns/manager.go +++ b/net/dns/manager.go @@ -485,7 +485,7 @@ func (m *Manager) FlushCaches() error { // in case the Tailscale daemon terminated without closing the router. // No other state needs to be instantiated before this runs. func CleanUp(logf logger.Logf, netMon *netmon.Monitor, interfaceName string) { - oscfg, err := NewOSConfigurator(logf, nil, interfaceName) + oscfg, err := NewOSConfigurator(logf, nil, nil, interfaceName) if err != nil { logf("creating dns cleanup: %v", err) return diff --git a/net/dns/manager_darwin.go b/net/dns/manager_darwin.go index 3ccdd6103..7e4d40320 100644 --- a/net/dns/manager_darwin.go +++ b/net/dns/manager_darwin.go @@ -8,12 +8,16 @@ import ( "os" "go4.org/mem" + "tailscale.com/control/controlknobs" "tailscale.com/health" "tailscale.com/types/logger" "tailscale.com/util/mak" ) -func NewOSConfigurator(logf logger.Logf, health *health.Tracker, ifName string) (OSConfigurator, error) { +// NewOSConfigurator creates a new OS configurator. +// +// The health tracker and the knobs may be nil and are ignored on this platform. +func NewOSConfigurator(logf logger.Logf, _ *health.Tracker, _ *controlknobs.Knobs, ifName string) (OSConfigurator, error) { return &darwinConfigurator{logf: logf, ifName: ifName}, nil } diff --git a/net/dns/manager_default.go b/net/dns/manager_default.go index 838d0761c..11dea5ca8 100644 --- a/net/dns/manager_default.go +++ b/net/dns/manager_default.go @@ -6,10 +6,14 @@ package dns import ( + "tailscale.com/control/controlknobs" "tailscale.com/health" "tailscale.com/types/logger" ) -func NewOSConfigurator(logger.Logf, *health.Tracker, string) (OSConfigurator, error) { +// NewOSConfigurator creates a new OS configurator. +// +// The health tracker and the knobs may be nil and are ignored on this platform. +func NewOSConfigurator(logger.Logf, *health.Tracker, *controlknobs.Knobs, string) (OSConfigurator, error) { return NewNoopManager() } diff --git a/net/dns/manager_freebsd.go b/net/dns/manager_freebsd.go index ac4567a6f..1ec9ea841 100644 --- a/net/dns/manager_freebsd.go +++ b/net/dns/manager_freebsd.go @@ -7,11 +7,15 @@ import ( "fmt" "os" + "tailscale.com/control/controlknobs" "tailscale.com/health" "tailscale.com/types/logger" ) -func NewOSConfigurator(logf logger.Logf, health *health.Tracker, _ string) (OSConfigurator, error) { +// NewOSConfigurator creates a new OS configurator. +// +// The health tracker may be nil; the knobs may be nil and are ignored on this platform. +func NewOSConfigurator(logf logger.Logf, health *health.Tracker, _ *controlknobs.Knobs, _ string) (OSConfigurator, error) { bs, err := os.ReadFile("/etc/resolv.conf") if os.IsNotExist(err) { return newDirectManager(logf, health), nil diff --git a/net/dns/manager_linux.go b/net/dns/manager_linux.go index 7ce0494ee..3ba3022b6 100644 --- a/net/dns/manager_linux.go +++ b/net/dns/manager_linux.go @@ -14,6 +14,7 @@ import ( "time" "github.com/godbus/dbus/v5" + "tailscale.com/control/controlknobs" "tailscale.com/health" "tailscale.com/net/netaddr" "tailscale.com/types/logger" @@ -31,7 +32,10 @@ func (kv kv) String() string { var publishOnce sync.Once -func NewOSConfigurator(logf logger.Logf, health *health.Tracker, interfaceName string) (ret OSConfigurator, err error) { +// NewOSConfigurator created a new OS configurator. +// +// The health tracker may be nil; the knobs may be nil and are ignored on this platform. +func NewOSConfigurator(logf logger.Logf, health *health.Tracker, _ *controlknobs.Knobs, interfaceName string) (ret OSConfigurator, err error) { env := newOSConfigEnv{ fs: directFS{}, dbusPing: dbusPing, diff --git a/net/dns/manager_openbsd.go b/net/dns/manager_openbsd.go index c55efa509..1a1c4390c 100644 --- a/net/dns/manager_openbsd.go +++ b/net/dns/manager_openbsd.go @@ -8,6 +8,7 @@ import ( "fmt" "os" + "tailscale.com/control/controlknobs" "tailscale.com/health" "tailscale.com/types/logger" ) @@ -20,7 +21,10 @@ func (kv kv) String() string { return fmt.Sprintf("%s=%s", kv.k, kv.v) } -func NewOSConfigurator(logf logger.Logf, health *health.Tracker, interfaceName string) (OSConfigurator, error) { +// NewOSConfigurator created a new OS configurator. +// +// The health tracker may be nil; the knobs may be nil and are ignored on this platform. +func NewOSConfigurator(logf logger.Logf, health *health.Tracker, _ *controlknobs.Knobs, interfaceName string) (OSConfigurator, error) { return newOSConfigurator(logf, health, interfaceName, newOSConfigEnv{ rcIsResolvd: rcIsResolvd, diff --git a/net/dns/manager_windows.go b/net/dns/manager_windows.go index 56bdbbe06..e4d703257 100644 --- a/net/dns/manager_windows.go +++ b/net/dns/manager_windows.go @@ -22,6 +22,7 @@ import ( "golang.org/x/sys/windows/registry" "golang.zx2c4.com/wireguard/windows/tunnel/winipcfg" "tailscale.com/atomicfile" + "tailscale.com/control/controlknobs" "tailscale.com/envknob" "tailscale.com/health" "tailscale.com/types/logger" @@ -38,6 +39,7 @@ var configureWSL = envknob.RegisterBool("TS_DEBUG_CONFIGURE_WSL") type windowsManager struct { logf logger.Logf guid string + knobs *controlknobs.Knobs // or nil nrptDB *nrptRuleDatabase wslManager *wslManager @@ -45,10 +47,14 @@ type windowsManager struct { closing bool } -func NewOSConfigurator(logf logger.Logf, health *health.Tracker, interfaceName string) (OSConfigurator, error) { +// NewOSConfigurator created a new OS configurator. +// +// The health tracker and the knobs may be nil. +func NewOSConfigurator(logf logger.Logf, health *health.Tracker, knobs *controlknobs.Knobs, interfaceName string) (OSConfigurator, error) { ret := &windowsManager{ logf: logf, guid: interfaceName, + knobs: knobs, wslManager: newWSLManager(logf, health), } @@ -288,6 +294,10 @@ func (m *windowsManager) setPrimaryDNS(resolvers []netip.Addr, domains []dnsname return nil } +func (m *windowsManager) disableLocalDNSOverrideViaNRPT() bool { + return m.knobs != nil && m.knobs.DisableLocalDNSOverrideViaNRPT.Load() +} + func (m *windowsManager) SetDNS(cfg OSConfig) error { // We can configure Windows DNS in one of two ways: // @@ -322,7 +332,17 @@ func (m *windowsManager) SetDNS(cfg OSConfig) error { } if len(cfg.MatchDomains) == 0 { - if err := m.setSplitDNS(nil, nil); err != nil { + var resolvers []netip.Addr + var domains []dnsname.FQDN + if !m.disableLocalDNSOverrideViaNRPT() { + // Create a default catch-all rule to make ourselves the actual primary resolver. + // Without this rule, Windows 8.1 and newer devices issue parallel DNS requests to DNS servers + // associated with all network adapters, even when "Override local DNS" is enabled and/or + // a Mullvad exit node is being used, resulting in DNS leaks. + resolvers = cfg.Nameservers + domains = []dnsname.FQDN{"."} + } + if err := m.setSplitDNS(resolvers, domains); err != nil { return err } if err := m.setHosts(nil); err != nil { @@ -331,8 +351,6 @@ func (m *windowsManager) SetDNS(cfg OSConfig) error { if err := m.setPrimaryDNS(cfg.Nameservers, cfg.SearchDomains); err != nil { return err } - } else if m.nrptDB == nil { - return errors.New("cannot set per-domain resolvers on Windows 7") } else { if err := m.setSplitDNS(cfg.Nameservers, cfg.MatchDomains); err != nil { return err diff --git a/net/dns/manager_windows_test.go b/net/dns/manager_windows_test.go index 917738ec0..183ab1177 100644 --- a/net/dns/manager_windows_test.go +++ b/net/dns/manager_windows_test.go @@ -84,7 +84,7 @@ func TestManagerWindowsGPCopy(t *testing.T) { } defer delIfKey() - cfg, err := NewOSConfigurator(logf, nil, fakeInterface.String()) + cfg, err := NewOSConfigurator(logf, nil, nil, fakeInterface.String()) if err != nil { t.Fatalf("NewOSConfigurator: %v\n", err) } @@ -213,7 +213,7 @@ func runTest(t *testing.T, isLocal bool) { } defer delIfKey() - cfg, err := NewOSConfigurator(logf, nil, fakeInterface.String()) + cfg, err := NewOSConfigurator(logf, nil, nil, fakeInterface.String()) if err != nil { t.Fatalf("NewOSConfigurator: %v\n", err) } diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index be3abeb2e..4460eb374 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -139,7 +139,8 @@ type CapabilityVersion int // - 96: 2024-05-29: Client understands NodeAttrSSHBehaviorV1 // - 97: 2024-06-06: Client understands NodeAttrDisableSplitDNSWhenNoCustomResolvers // - 98: 2024-06-13: iOS/tvOS clients may provide serial number as part of posture information -const CurrentCapabilityVersion CapabilityVersion = 98 +// - 99: 2024-06-14: Client understands NodeAttrDisableLocalDNSOverrideViaNRPT +const CurrentCapabilityVersion CapabilityVersion = 99 type StableID string @@ -2306,6 +2307,15 @@ const ( // and this node attribute allows us to disable the optimization remotely // if needed. NodeAttrDisableSplitDNSWhenNoCustomResolvers NodeCapability = "disable-split-dns-when-no-custom-resolvers" + + // NodeAttrDisableLocalDNSOverrideViaNRPT indicates that the node's DNS manager should not + // create a default (catch-all) Windows NRPT rule when "Override local DNS" is enabled. + // Without this rule, Windows 8.1 and newer devices issue parallel DNS requests to DNS servers + // associated with all network adapters, even when "Override local DNS" is enabled and/or + // a Mullvad exit node is being used, resulting in DNS leaks. + // We began creating this rule on 2024-06-14, and this node attribute + // allows us to disable the new behavior remotely if needed. + NodeAttrDisableLocalDNSOverrideViaNRPT NodeCapability = "disable-local-dns-override-via-nrpt" ) // SetDNSRequest is a request to add a DNS record.