diff --git a/CHANGELOG.md b/CHANGELOG.md index c57b224d..2b060ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,10 @@ NOTE: Add new changes BELOW THIS COMMENT. - The ability to manage safesearch for each service by using the new `safe_search` field ([#1163]). -### Changed +### Changed + +- ARPA domain names containing a subnet within private networks now also + considered private, behaving closer to [RFC 6761][rfc6761] ([#5567]). #### Configuration Changes @@ -65,8 +68,11 @@ In this release, the schema version has changed from 17 to 19. ([#5584]). [#1163]: https://github.com/AdguardTeam/AdGuardHome/issues/1163 +[#5567]: https://github.com/AdguardTeam/AdGuardHome/issues/5567 [#5584]: https://github.com/AdguardTeam/AdGuardHome/issues/5584 +[rfc6761]: https://www.rfc-editor.org/rfc/rfc6761 + diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index 9289eb45..cc02bb9c 100644 --- a/internal/dnsforward/dns.go +++ b/internal/dnsforward/dns.go @@ -4,6 +4,7 @@ import ( "encoding/binary" "net" "net/netip" + "strconv" "strings" "time" @@ -37,6 +38,8 @@ type dnsContext struct { // was parsed successfully and belongs to one of the locally served IP // ranges. It is also filled with unmapped version of the address if it's // within DNS64 prefixes. + // + // TODO(e.burkov): Use netip.Addr when we switch to netip more fully. unreversedReqIP net.IP // err is the error returned from a processing function. @@ -442,6 +445,88 @@ func (s *Server) processDHCPHosts(dctx *dnsContext) (rc resultCode) { return resultCodeSuccess } +// indexFirstV4Label returns the index at which the reversed IPv4 address +// starts, assuiming the domain is pre-validated ARPA domain having in-addr and +// arpa labels removed. +func indexFirstV4Label(domain string) (idx int) { + idx = len(domain) + for labelsNum := 0; labelsNum < net.IPv4len && idx > 0; labelsNum++ { + curIdx := strings.LastIndexByte(domain[:idx-1], '.') + 1 + _, parseErr := strconv.ParseUint(domain[curIdx:idx-1], 10, 8) + if parseErr != nil { + return idx + } + + idx = curIdx + } + + return idx +} + +// indexFirstV6Label returns the index at which the reversed IPv6 address +// starts, assuiming the domain is pre-validated ARPA domain having ip6 and arpa +// labels removed. +func indexFirstV6Label(domain string) (idx int) { + idx = len(domain) + for labelsNum := 0; labelsNum < net.IPv6len*2 && idx > 0; labelsNum++ { + curIdx := idx - len("a.") + if curIdx > 1 && domain[curIdx-1] != '.' { + return idx + } + + nibble := domain[curIdx] + if (nibble < '0' || nibble > '9') && (nibble < 'a' || nibble > 'f') { + return idx + } + + idx = curIdx + } + + return idx +} + +// extractARPASubnet tries to convert a reversed ARPA address being a part of +// domain to an IP network. domain must be an FQDN. +// +// TODO(e.burkov): Move to golibs. +func extractARPASubnet(domain string) (pref netip.Prefix, err error) { + err = netutil.ValidateDomainName(strings.TrimSuffix(domain, ".")) + if err != nil { + // Don't wrap the error since it's informative enough as is. + return netip.Prefix{}, err + } + + const ( + v4Suffix = "in-addr.arpa." + v6Suffix = "ip6.arpa." + ) + + domain = strings.ToLower(domain) + + var idx int + switch { + case strings.HasSuffix(domain, v4Suffix): + idx = indexFirstV4Label(domain[:len(domain)-len(v4Suffix)]) + case strings.HasSuffix(domain, v6Suffix): + idx = indexFirstV6Label(domain[:len(domain)-len(v6Suffix)]) + default: + return netip.Prefix{}, &netutil.AddrError{ + Err: netutil.ErrNotAReversedSubnet, + Kind: netutil.AddrKindARPA, + Addr: domain, + } + } + + var subnet *net.IPNet + subnet, err = netutil.SubnetFromReversedAddr(domain[idx:]) + if err != nil { + // Don't wrap the error since it's informative enough as is. + return netip.Prefix{}, err + } + + return netutil.IPNetToPrefixNoMapped(subnet) +} + // processRestrictLocal responds with NXDOMAIN to PTR requests for IP addresses // in locally served network from external clients. func (s *Server) processRestrictLocal(dctx *dnsContext) (rc resultCode) { @@ -453,34 +538,29 @@ func (s *Server) processRestrictLocal(dctx *dnsContext) (rc resultCode) { return resultCodeSuccess } - ip, err := netutil.IPFromReversedAddr(q.Name) + subnet, err := extractARPASubnet(q.Name) if err != nil { - log.Debug("dnsforward: parsing reversed addr: %s", err) + if errors.Is(err, netutil.ErrNotAReversedSubnet) { + log.Debug("dnsforward: request is not for arpa domain") - // DNS-Based Service Discovery uses PTR records having not an ARPA - // format of the domain name in question. Those shouldn't be - // invalidated. See http://www.dns-sd.org/ServerStaticSetup.html and - // RFC 2782. - name := strings.TrimSuffix(q.Name, ".") - if err = netutil.ValidateSRVDomainName(name); err != nil { - log.Debug("dnsforward: validating service domain: %s", err) - - return resultCodeError + return resultCodeSuccess } - log.Debug("dnsforward: request is not for arpa domain") + log.Debug("dnsforward: parsing reversed addr: %s", err) - return resultCodeSuccess + return resultCodeError } // Restrict an access to local addresses for external clients. We also // assume that all the DHCP leases we give are locally served or at least // shouldn't be accessible externally. - if !s.privateNets.Contains(ip) { + subnetAddr := subnet.Addr() + addrData := subnetAddr.AsSlice() + if !s.privateNets.Contains(addrData) { return resultCodeSuccess } - log.Debug("dnsforward: addr %s is from locally served network", ip) + log.Debug("dnsforward: addr %s is from locally served network", subnetAddr) if !dctx.isLocalClient { log.Debug("dnsforward: %q requests an internal ip", pctx.Addr) @@ -491,7 +571,7 @@ func (s *Server) processRestrictLocal(dctx *dnsContext) (rc resultCode) { } // Do not perform unreversing ever again. - dctx.unreversedReqIP = ip + dctx.unreversedReqIP = addrData // There is no need to filter request from external addresses since this // code is only executed when the request is for locally served ARPA diff --git a/internal/dnsforward/dns_test.go b/internal/dnsforward/dns_test.go index 02f1eb61..1bcca756 100644 --- a/internal/dnsforward/dns_test.go +++ b/internal/dnsforward/dns_test.go @@ -605,3 +605,129 @@ func TestIPStringFromAddr(t *testing.T) { assert.Empty(t, ipStringFromAddr(nil)) }) } + +// TODO(e.burkov): Add fuzzing when moving to golibs. +func TestExtractARPASubnet(t *testing.T) { + const ( + v4Suf = `in-addr.arpa.` + v4Part = `2.1.` + v4Suf + v4Whole = `4.3.` + v4Part + + v6Suf = `ip6.arpa.` + v6Part = `4.3.2.1.0.0.0.0.0.0.0.0.0.0.0.0.` + v6Suf + v6Whole = `f.e.d.c.0.0.0.0.0.0.0.0.0.0.0.0.` + v6Part + ) + + v4Pref := netip.MustParsePrefix("1.2.3.4/32") + v4PrefPart := netip.MustParsePrefix("1.2.0.0/16") + v6Pref := netip.MustParsePrefix("::1234:0:0:0:cdef/128") + v6PrefPart := netip.MustParsePrefix("0:0:0:1234::/64") + + testCases := []struct { + want netip.Prefix + name string + domain string + wantErr string + }{{ + want: netip.Prefix{}, + name: "not_an_arpa", + domain: "some.domain.name.", + wantErr: `bad arpa domain name "some.domain.name.": ` + + `not a reversed ip network`, + }, { + want: netip.Prefix{}, + name: "bad_domain_name", + domain: "abc.123.", + wantErr: `bad domain name "abc.123": ` + + `bad top-level domain name label "123": all octets are numeric`, + }, { + want: v4Pref, + name: "whole_v4", + domain: v4Whole, + wantErr: "", + }, { + want: v4PrefPart, + name: "partial_v4", + domain: v4Part, + wantErr: "", + }, { + want: v4Pref, + name: "whole_v4_within_domain", + domain: "a." + v4Whole, + wantErr: "", + }, { + want: v4Pref, + name: "whole_v4_additional_label", + domain: "5." + v4Whole, + wantErr: "", + }, { + want: v4PrefPart, + name: "partial_v4_within_domain", + domain: "a." + v4Part, + wantErr: "", + }, { + want: v4PrefPart, + name: "overflow_v4", + domain: "256." + v4Part, + wantErr: "", + }, { + want: v4PrefPart, + name: "overflow_v4_within_domain", + domain: "a.256." + v4Part, + wantErr: "", + }, { + want: netip.Prefix{}, + name: "empty_v4", + domain: v4Suf, + wantErr: `bad arpa domain name "in-addr.arpa": ` + + `not a reversed ip network`, + }, { + want: netip.Prefix{}, + name: "empty_v4_within_domain", + domain: "a." + v4Suf, + wantErr: `bad arpa domain name "in-addr.arpa": ` + + `not a reversed ip network`, + }, { + want: v6Pref, + name: "whole_v6", + domain: v6Whole, + wantErr: "", + }, { + want: v6PrefPart, + name: "partial_v6", + domain: v6Part, + }, { + want: v6Pref, + name: "whole_v6_within_domain", + domain: "g." + v6Whole, + wantErr: "", + }, { + want: v6Pref, + name: "whole_v6_additional_label", + domain: "1." + v6Whole, + wantErr: "", + }, { + want: v6PrefPart, + name: "partial_v6_within_domain", + domain: "label." + v6Part, + wantErr: "", + }, { + want: netip.Prefix{}, + name: "empty_v6", + domain: v6Suf, + wantErr: `bad arpa domain name "ip6.arpa": not a reversed ip network`, + }, { + want: netip.Prefix{}, + name: "empty_v6_within_domain", + domain: "g." + v6Suf, + wantErr: `bad arpa domain name "ip6.arpa": not a reversed ip network`, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + subnet, err := extractARPASubnet(tc.domain) + testutil.AssertErrorMsg(t, tc.wantErr, err) + assert.Equal(t, tc.want, subnet) + }) + } +} diff --git a/internal/dnsforward/http.go b/internal/dnsforward/http.go index a876a411..0c8b6726 100644 --- a/internal/dnsforward/http.go +++ b/internal/dnsforward/http.go @@ -388,15 +388,15 @@ func ValidateUpstreamsPrivate(upstreams []string, privateNets netutil.SubnetSet) var errs []error for _, domain := range keys { - var subnet *net.IPNet - subnet, err = netutil.SubnetFromReversedAddr(domain) + var subnet netip.Prefix + subnet, err = extractARPASubnet(domain) if err != nil { errs = append(errs, err) continue } - if !privateNets.Contains(subnet.IP) { + if !privateNets.Contains(subnet.Addr().AsSlice()) { errs = append( errs, fmt.Errorf("arpa domain %q should point to a locally-served network", domain), diff --git a/internal/dnsforward/http_test.go b/internal/dnsforward/http_test.go index 3370ffdd..ef2228c1 100644 --- a/internal/dnsforward/http_test.go +++ b/internal/dnsforward/http_test.go @@ -212,7 +212,7 @@ func TestDNSForwardHTTP_handleSetConfig(t *testing.T) { }, { name: "local_ptr_upstreams_bad", wantSet: `validating private upstream servers: checking domain-specific upstreams: ` + - `bad arpa domain name "non.arpa": not a reversed ip network`, + `bad arpa domain name "non.arpa.": not a reversed ip network`, }, { name: "local_ptr_upstreams_null", wantSet: "", @@ -373,7 +373,7 @@ func TestValidateUpstreamsPrivate(t *testing.T) { }, { name: "not_arpa_subnet", wantErr: `checking domain-specific upstreams: ` + - `bad arpa domain name "hello.world": not a reversed ip network`, + `bad arpa domain name "hello.world.": not a reversed ip network`, u: "[/hello.world/]#", }, { name: "non-private_arpa_address", @@ -389,8 +389,12 @@ func TestValidateUpstreamsPrivate(t *testing.T) { name: "several_bad", wantErr: `checking domain-specific upstreams: 2 errors: ` + `"arpa domain \"1.2.3.4.in-addr.arpa.\" should point to a locally-served network", ` + - `"bad arpa domain name \"non.arpa\": not a reversed ip network"`, + `"bad arpa domain name \"non.arpa.\": not a reversed ip network"`, u: "[/non.arpa/1.2.3.4.in-addr.arpa/127.in-addr.arpa/]#", + }, { + name: "partial_good", + wantErr: "", + u: "[/a.1.2.3.10.in-addr.arpa/a.10.in-addr.arpa/]#", }} for _, tc := range testCases {