Merge remote-tracking branch 'origin/master' into 1163-safesearch-1-3

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
 
@@ -80,8 +83,11 @@ The `safesearch_enabled` field is deprecated in the following HTTP APIs:
   ([#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
+
 <!--
 NOTE: Add new changes ABOVE THIS COMMENT.
 -->

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

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

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),

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 {