From 48431f8b86a1e4dde8aebb7539682eb8d043c556 Mon Sep 17 00:00:00 2001
From: Eugene Burkov <e.burkov@adguard.com>
Date: Fri, 17 Mar 2023 17:10:33 +0300
Subject: [PATCH] Pull request #1770: 5567-extract-subnet-arpa
Merge in DNS/adguard-home from 5567-extract-subnet-arpa to master
Updates #5567.
Squashed commit of the following:
commit 288fb405b82eff2a95d75f8c557100908a998a08
Merge: e16b3ce5 9f7a582d
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Fri Mar 17 14:01:39 2023 +0300
Merge branch 'master' into 5567-extract-subnet-arpa
commit e16b3ce57ba41a9f4a7743dbdb93c2320e650140
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Fri Mar 17 13:58:58 2023 +0300
dnsforward: use netip
commit 265b08c5f82f8df555ab1a5f01c2e9ef8caef64a
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Thu Mar 16 19:11:49 2023 +0300
dnsforward: imp tests more
commit 53a839cb6dd924cabf0552386f76aa8775c88983
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Thu Mar 16 19:09:15 2023 +0300
dnsforward: imp naming in tests
commit 74dcccbdda217422260579e331289003a024695e
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Thu Mar 16 18:59:12 2023 +0300
dnsforward: imp code & tests more
commit da8badfaa75a0a67c10ce6f347e551dcfd4c0589
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Wed Mar 15 14:52:48 2023 +0300
all: log changes
commit c491cbfb3fd8d716303224c1f73329a47087753a
Merge: 74a93179 2b5e4850
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Wed Mar 15 14:44:31 2023 +0300
Merge branch 'master' into 5567-extract-subnet-arpa
commit 74a93179d7fb7f005455ce02f7f0c16b796c3914
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Wed Mar 15 14:42:55 2023 +0300
dnsforward: imp code, docs
commit 17df1a0ce461335649c6dab65c984eb0cce0bdf0
Author: Eugene Burkov <E.Burkov@AdGuard.COM>
Date: Tue Mar 14 19:49:10 2023 +0300
dnsforward: extract subnet from arpa
---
CHANGELOG.md | 8 +-
internal/dnsforward/dns.go | 112 +++++++++++++++++++++++----
internal/dnsforward/dns_test.go | 126 +++++++++++++++++++++++++++++++
internal/dnsforward/http.go | 6 +-
internal/dnsforward/http_test.go | 10 ++-
5 files changed, 239 insertions(+), 23 deletions(-)
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
+
<!--
NOTE: Add new changes ABOVE THIS COMMENT.
-->
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 {