From 25ce4b7c845914726cff58fcc499161de049c216 Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 10:29:33 +0100 Subject: [PATCH 1/7] Improve search scope validation and add configuration support --- resolver/resolvers.go | 95 ++++++++++++++++++++++++-------------- resolver/resolvers_test.go | 60 +++++++++++++----------- 2 files changed, 93 insertions(+), 62 deletions(-) diff --git a/resolver/resolvers.go b/resolver/resolvers.go index 7c26cb88..53762b80 100644 --- a/resolver/resolvers.go +++ b/resolver/resolvers.go @@ -9,6 +9,8 @@ import ( "strings" "sync" + "github.com/safing/portbase/utils" + "golang.org/x/net/publicsuffix" "github.com/safing/portbase/log" @@ -16,6 +18,8 @@ import ( "github.com/safing/portmaster/network/netutils" ) +const maxSearchDomains = 100 + // Scope defines a domain scope and which resolvers can resolve it. type Scope struct { Domain string @@ -153,22 +157,43 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { UpstreamBlockDetection: blockType, } + searchDomains := query.Get("search") + if searchDomains != "" { + err = configureSearchDomains(newResolver, strings.Split(searchDomains, ","), true) + if err != nil { + return nil, false, err + } + } + newResolver.Conn = resolverConnFactory(newResolver) return newResolver, false, nil } -func configureSearchDomains(resolver *Resolver, searches []string) { - // only allow searches for local resolvers - for _, value := range searches { +func configureSearchDomains(resolver *Resolver, searches []string, hardfail bool) error { + // Check all search domains. + for i, value := range searches { trimmedDomain := strings.Trim(value, ".") - if checkSearchScope(trimmedDomain) { - resolver.Search = append(resolver.Search, fmt.Sprintf(".%s.", strings.Trim(value, "."))) + err := checkSearchScope(trimmedDomain) + if err != nil { + if hardfail { + return fmt.Errorf("failed to validate search domain #%d: %w", i+1, err) + } + log.Warningf("resolver: skipping invalid search domain for resolver %s: %s", resolver, utils.SafeFirst16Chars(value)) + } else { + resolver.Search = append(resolver.Search, fmt.Sprintf(".%s.", trimmedDomain)) } } - // cap to mitigate exploitation via malicious local resolver - if len(resolver.Search) > 100 { - resolver.Search = resolver.Search[:100] + + // Limit search domains to mitigate exploitation via malicious local resolver. + if len(resolver.Search) > maxSearchDomains { + if hardfail { + return fmt.Errorf("too many search domains, for security reasons only %d search domains are supported", maxSearchDomains) + } + log.Warningf("resolver: limiting search domains for resolver %s to %d for security reasons", resolver, maxSearchDomains) + resolver.Search = resolver.Search[:maxSearchDomains] } + + return nil } func getConfiguredResolvers(list []string) (resolvers []*Resolver) { @@ -204,7 +229,7 @@ func getSystemResolvers() (resolvers []*Resolver) { } if resolver.Info.IPScope.IsLAN() { - configureSearchDomains(resolver, nameserver.Search) + configureSearchDomains(resolver, nameserver.Search, false) } resolvers = append(resolvers, resolver) @@ -355,46 +380,48 @@ func setScopedResolvers(resolvers []*Resolver) { ) } -func checkSearchScope(searchDomain string) (ok bool) { - // sanity check +func checkSearchScope(searchDomain string) error { + // Sanity check the input. if len(searchDomain) == 0 || searchDomain[0] == '.' || searchDomain[len(searchDomain)-1] == '.' { - return false + return fmt.Errorf("invalid (1) search domain: %s", searchDomain) } - // add more subdomains to use official publicsuffix package for our cause - searchDomain = "*.*.*.*.*." + searchDomain + // Domains that are specifically listed in the special service domains may be + // diverted completely. + if domainInScope("."+searchDomain+".", specialServiceDomains) { + return nil + } - // get suffix - suffix, icann := publicsuffix.PublicSuffix(searchDomain) - // sanity check + // In order to check if the search domain is too high up in the hierarchy, we + // need to add some more subdomains. + augmentedSearchDomain := "*.*.*.*.*." + searchDomain + + // Get the public suffix of the search domain and if the TLD is managed by ICANN. + suffix, icann := publicsuffix.PublicSuffix(augmentedSearchDomain) + // Sanity check the result. if len(suffix) == 0 { - return false + return fmt.Errorf("invalid (2) search domain: %s", searchDomain) } - // inexistent (custom) tlds are okay - // this will include special service domains! (.onion, .bit, ...) + + // TLDs that are not managed by ICANN (ie. are unofficial) may be + // diverted completely. + // This might include special service domains like ".onion" and ".bit". if !icann && !strings.Contains(suffix, ".") { - return true + return nil } - // check if suffix is a special service domain (may be handled fully by local nameserver) - if domainInScope("."+suffix+".", specialServiceDomains) { - return true - } + // Build the eTLD+1 domain, which is the highest that may be diverted. + split := len(augmentedSearchDomain) - len(suffix) - 1 + eTLDplus1 := augmentedSearchDomain[1+strings.LastIndex(augmentedSearchDomain[:split], "."):] - // build eTLD+1 - split := len(searchDomain) - len(suffix) - 1 - eTLDplus1 := searchDomain[1+strings.LastIndex(searchDomain[:split], "."):] - - // scope check - //nolint:gosimple // want comment + // Check if the scope is being violated by checking if the eTLD+1 contains a wildcard. if strings.Contains(eTLDplus1, "*") { - // oops, search domain is too high up the hierarchy - return false + return fmt.Errorf(`search domain "%s" is dangerously high up the hierarchy, stay at or below "%s"`, searchDomain, eTLDplus1) } - return true + return nil } // IsResolverAddress returns whether the given ip and port match a configured resolver. diff --git a/resolver/resolvers_test.go b/resolver/resolvers_test.go index 29697440..4c529a6e 100644 --- a/resolver/resolvers_test.go +++ b/resolver/resolvers_test.go @@ -1,40 +1,44 @@ package resolver -import "testing" +import ( + "testing" + + "github.com/stretchr/testify/assert" +) func TestCheckResolverSearchScope(t *testing.T) { t.Parallel() - test := func(t *testing.T, domain string, expectedResult bool) { - t.Helper() - - if checkSearchScope(domain) != expectedResult { - if expectedResult { - t.Errorf("domain %s failed scope test", domain) - } else { - t.Errorf("domain %s should fail scope test", domain) - } - } - } - // should fail (invalid) - test(t, ".", false) - test(t, ".com.", false) - test(t, "com.", false) - test(t, ".com", false) + assert.Error(t, checkSearchScope(".")) + assert.Error(t, checkSearchScope(".com.")) + assert.Error(t, checkSearchScope("com.")) + assert.Error(t, checkSearchScope(".com")) + + // should fail (too high scope) + assert.Error(t, checkSearchScope("com")) + assert.Error(t, checkSearchScope("net")) + assert.Error(t, checkSearchScope("org")) + assert.Error(t, checkSearchScope("pvt.k12.ma.us")) // should succeed - test(t, "a.com", true) - test(t, "b.a.com", true) - test(t, "c.b.a.com", true) + assert.NoError(t, checkSearchScope("a.com")) + assert.NoError(t, checkSearchScope("b.a.com")) + assert.NoError(t, checkSearchScope("c.b.a.com")) + assert.NoError(t, checkSearchScope("test.pvt.k12.ma.us")) - test(t, "onion", true) - test(t, "a.onion", true) - test(t, "b.a.onion", true) - test(t, "c.b.a.onion", true) + assert.NoError(t, checkSearchScope("onion")) + assert.NoError(t, checkSearchScope("a.onion")) + assert.NoError(t, checkSearchScope("b.a.onion")) + assert.NoError(t, checkSearchScope("c.b.a.onion")) - test(t, "bit", true) - test(t, "a.bit", true) - test(t, "b.a.bit", true) - test(t, "c.b.a.bit", true) + assert.NoError(t, checkSearchScope("bit")) + assert.NoError(t, checkSearchScope("a.bit")) + assert.NoError(t, checkSearchScope("b.a.bit")) + assert.NoError(t, checkSearchScope("c.b.a.bit")) + + assert.NoError(t, checkSearchScope("doesnotexist")) + assert.NoError(t, checkSearchScope("a.doesnotexist")) + assert.NoError(t, checkSearchScope("b.a.doesnotexist")) + assert.NoError(t, checkSearchScope("c.b.a.doesnotexist")) } From 2a930b6362c1f4d7bc5170c30b6814cb7b9cdd4d Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 10:30:48 +0100 Subject: [PATCH 2/7] Add support for search-only resolvers --- resolver/resolver.go | 1 + resolver/resolvers.go | 14 ++++++++++++++ resolver/scopes.go | 6 ++++++ 3 files changed, 21 insertions(+) diff --git a/resolver/resolver.go b/resolver/resolver.go index 0aaab91b..56d4b6cc 100644 --- a/resolver/resolver.go +++ b/resolver/resolver.go @@ -63,6 +63,7 @@ type Resolver struct { // Special Options VerifyDomain string Search []string + SearchOnly bool // logic interface Conn ResolverConn `json:"-"` diff --git a/resolver/resolvers.go b/resolver/resolvers.go index 53762b80..09b152e3 100644 --- a/resolver/resolvers.go +++ b/resolver/resolvers.go @@ -1,6 +1,7 @@ package resolver import ( + "errors" "fmt" "net" "net/url" @@ -157,6 +158,7 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { UpstreamBlockDetection: blockType, } + // Parse search domains. searchDomains := query.Get("search") if searchDomains != "" { err = configureSearchDomains(newResolver, strings.Split(searchDomains, ","), true) @@ -165,6 +167,18 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { } } + // Check if searchOnly is set and valid. + if query.Has("searchOnly") { + newResolver.SearchOnly = true + + if query.Get("searchOnly") != "" { + return nil, false, errors.New("searchOnly may only be used as an empty parameter") + } + if len(newResolver.Search) == 0 { + return nil, false, errors.New("cannot use searchOnly without search scopes") + } + } + newResolver.Conn = resolverConnFactory(newResolver) return newResolver, false, nil } diff --git a/resolver/scopes.go b/resolver/scopes.go index ee9f396e..e7fd7f9d 100644 --- a/resolver/scopes.go +++ b/resolver/scopes.go @@ -179,6 +179,7 @@ var ( errInsecureProtocol = errors.New("insecure protocols disabled") errAssignedServer = errors.New("assigned (dhcp) nameservers disabled") errMulticastDNS = errors.New("multicast DNS disabled") + errOutOfScope = errors.New("query out of scope for resolver") ) func (q *Query) checkCompliance() error { @@ -236,5 +237,10 @@ func (resolver *Resolver) checkCompliance(_ context.Context, q *Query) error { } } + // Check if the resolver should only be used for the search scopes. + if resolver.SearchOnly && !domainInScope(q.dotPrefixedFQDN, resolver.Search) { + return errOutOfScope + } + return nil } From 9300d0b6eab4939679e8383d9cb262183189ad32 Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 10:31:35 +0100 Subject: [PATCH 3/7] Expand special service TLDs --- resolver/scopes.go | 53 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/resolver/scopes.go b/resolver/scopes.go index e7fd7f9d..54757a43 100644 --- a/resolver/scopes.go +++ b/resolver/scopes.go @@ -91,11 +91,62 @@ var ( // Special-Service Domain Names // Handling: Send to nameservers with matching search scope, then local and system assigned nameservers. specialServiceDomains = []string{ - // RFC7686: Tor Hidden Services + // RFC7686: Tor Hidden Services, https://www.torproject.org/ ".onion.", + // I2P: Fully encrypted private network layer, https://geti2p.net/ + ".i2p.", + + // Lokinet: Internal services on the decentralised network, https://lokinet.org/ + ".loki.", + // Namecoin: Blockchain based nameservice, https://www.namecoin.org/ ".bit.", + + // Ethereum Name Service (ENS): naming system based on the Ethereum blockchain, https://ens.domains/ + ".eth.", + + // Unstoppable Domains: NFT based domain names, https://unstoppabledomains.com/ + ".888.", + ".bitcoin.", + ".coin.", + ".crypto.", + ".dao.", + ".nft.", + ".wallet.", + ".x.", + ".zil.", + + // EmerDNS: Domain name registraion on EmerCoin, https://emercoin.com/en/emerdns/ + ".bazar.", + ".coin.", + ".emc.", + ".lib.", + + // OpenNIC TLDs: Democratic alternative to ICANN, https://www.opennic.org/ + ".bbs.", + ".chan.", + ".dyn.", + ".free.", + ".fur.", + ".geek.", + ".glue.", + ".gopher.", + ".indy.", + ".libre.", + ".neo.", + ".null.", + ".o.", + ".oss.", + ".oz.", + ".parody.", + ".pirate.", + + // NewNations: TLDs for countries/regions without a ccTLD, http://new-nations.net/ + ".ku.", + ".te.", + ".ti.", + ".uu.", } ) From 52506ffb1ee6a1415cfd971da569ae95ca51bd75 Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 11:28:10 +0100 Subject: [PATCH 4/7] Add documentation for new parameters --- resolver/config.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/resolver/config.go b/resolver/config.go index a69c9419..b274f119 100644 --- a/resolver/config.go +++ b/resolver/config.go @@ -104,6 +104,8 @@ The format is: "protocol://ip:port?parameter=value¶meter=value" - "empty": server replies with NXDomain status, but without any other record in any section - "refused": server replies with Refused status - "zeroip": server replies with an IP address, but it is zero + - "search": specify prioritized domains/TLDs for this resolver (delimited by ",") + - "search-only": use this resolver for domains in the "search" parameter only (no value) `, `"`, "`"), OptType: config.OptTypeStringArray, ExpertiseLevel: config.ExpertiseLevelExpert, From 75b3c40ffc0a520d362f55e9c4955d597f66f8db Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 11:29:16 +0100 Subject: [PATCH 5/7] Validate resolver config for unknown parameters --- resolver/resolvers.go | 47 ++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/resolver/resolvers.go b/resolver/resolvers.go index 09b152e3..e76e1be4 100644 --- a/resolver/resolvers.go +++ b/resolver/resolvers.go @@ -27,6 +27,14 @@ type Scope struct { Resolvers []*Resolver } +const ( + parameterName = "name" + parameterVerify = "verify" + parameterBlockedIf = "blockedif" + parameterSearch = "search" + parameterSearchOnly = "search-only" +) + var ( globalResolvers []*Resolver // all (global) resolvers localResolvers []*Resolver // all resolvers that are in site-local or link-local IP ranges @@ -122,31 +130,47 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { return nil, true, nil // skip } + // Get parameters and check if keys exist. query := u.Query() - verifyDomain := query.Get("verify") + for key := range query { + switch key { + case parameterName, + parameterVerify, + parameterBlockedIf, + parameterSearch, + parameterSearchOnly: + // Known key, continue. + default: + // Unknown key, abort. + return nil, false, fmt.Errorf(`unknown parameter "%s"`, key) + } + } + + // Check domain verification config. + verifyDomain := query.Get(parameterVerify) if verifyDomain != "" && u.Scheme != ServerTypeDoT { return nil, false, fmt.Errorf("domain verification only supported in DOT") } - if verifyDomain == "" && u.Scheme == ServerTypeDoT { return nil, false, fmt.Errorf("DOT must have a verify query parameter set") } - blockType := query.Get("blockedif") + // Check block detection type. + blockType := query.Get(parameterBlockedIf) if blockType == "" { blockType = BlockDetectionZeroIP } - switch blockType { case BlockDetectionDisabled, BlockDetectionEmptyAnswer, BlockDetectionRefused, BlockDetectionZeroIP: default: return nil, false, fmt.Errorf("invalid value for upstream block detection (blockedif=)") } + // Build resolver. newResolver := &Resolver{ ConfigURL: resolverURL, Info: &ResolverInfo{ - Name: query.Get("name"), + Name: query.Get(parameterName), Type: u.Scheme, Source: source, IP: ip, @@ -159,7 +183,7 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { } // Parse search domains. - searchDomains := query.Get("search") + searchDomains := query.Get(parameterSearch) if searchDomains != "" { err = configureSearchDomains(newResolver, strings.Split(searchDomains, ","), true) if err != nil { @@ -168,14 +192,13 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { } // Check if searchOnly is set and valid. - if query.Has("searchOnly") { + if query.Has(parameterSearchOnly) { newResolver.SearchOnly = true - - if query.Get("searchOnly") != "" { - return nil, false, errors.New("searchOnly may only be used as an empty parameter") + if query.Get(parameterSearchOnly) != "" { + return nil, false, fmt.Errorf("%s may only be used as an empty parameter", parameterSearchOnly) } if len(newResolver.Search) == 0 { - return nil, false, errors.New("cannot use searchOnly without search scopes") + return nil, false, fmt.Errorf("cannot use %s without search scopes", parameterSearchOnly) } } @@ -186,7 +209,7 @@ func createResolver(resolverURL, source string) (*Resolver, bool, error) { func configureSearchDomains(resolver *Resolver, searches []string, hardfail bool) error { // Check all search domains. for i, value := range searches { - trimmedDomain := strings.Trim(value, ".") + trimmedDomain := strings.ToLower(strings.Trim(value, ".")) err := checkSearchScope(trimmedDomain) if err != nil { if hardfail { From 92bf99d0ed1cd446244b483139b39e8c103e1280 Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 11:47:44 +0100 Subject: [PATCH 6/7] Fix linter warnings --- resolver/main.go | 2 -- resolver/resolvers.go | 6 ++---- resolver/scopes.go | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/resolver/main.go b/resolver/main.go index 8692397e..36fe273f 100644 --- a/resolver/main.go +++ b/resolver/main.go @@ -14,8 +14,6 @@ import ( "github.com/safing/portbase/modules" "github.com/safing/portbase/notifications" "github.com/safing/portbase/utils/debug" - - // Dependency. _ "github.com/safing/portmaster/core/base" "github.com/safing/portmaster/intel" ) diff --git a/resolver/resolvers.go b/resolver/resolvers.go index e76e1be4..2e66b332 100644 --- a/resolver/resolvers.go +++ b/resolver/resolvers.go @@ -1,7 +1,6 @@ package resolver import ( - "errors" "fmt" "net" "net/url" @@ -10,11 +9,10 @@ import ( "strings" "sync" - "github.com/safing/portbase/utils" - "golang.org/x/net/publicsuffix" "github.com/safing/portbase/log" + "github.com/safing/portbase/utils" "github.com/safing/portmaster/netenv" "github.com/safing/portmaster/network/netutils" ) @@ -266,7 +264,7 @@ func getSystemResolvers() (resolvers []*Resolver) { } if resolver.Info.IPScope.IsLAN() { - configureSearchDomains(resolver, nameserver.Search, false) + _ = configureSearchDomains(resolver, nameserver.Search, false) } resolvers = append(resolvers, resolver) diff --git a/resolver/scopes.go b/resolver/scopes.go index 54757a43..883bc0ad 100644 --- a/resolver/scopes.go +++ b/resolver/scopes.go @@ -117,7 +117,7 @@ var ( ".x.", ".zil.", - // EmerDNS: Domain name registraion on EmerCoin, https://emercoin.com/en/emerdns/ + // EmerDNS: Domain name registration on EmerCoin, https://emercoin.com/en/emerdns/ ".bazar.", ".coin.", ".emc.", From e3a450e96b7b83c3388a85c28177857e1d9f64f9 Mon Sep 17 00:00:00 2001 From: Daniel Date: Wed, 16 Mar 2022 11:48:42 +0100 Subject: [PATCH 7/7] Set recursion available flag on nameserver responses --- nameserver/response.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nameserver/response.go b/nameserver/response.go index 2a0e992e..d78888c8 100644 --- a/nameserver/response.go +++ b/nameserver/response.go @@ -27,6 +27,8 @@ func sendResponse( // Dropping query. return nil } + // Signify that we are a recursive resolver. + reply.RecursionAvailable = true // Add extra RRs through a custom RRProvider. for _, rrProvider := range rrProviders {