Skip to content

Commit

Permalink
Pull request #1770: 5567-extract-subnet-arpa
Browse files Browse the repository at this point in the history 
Merge in DNS/adguard-home from 5567-extract-subnet-arpa to master

Updates #5567.

Squashed commit of the following:

commit 288fb40
Merge: e16b3ce 9f7a582
Author: Eugene Burkov <[email protected]>
Date:   Fri Mar 17 14:01:39 2023 +0300

    Merge branch 'master' into 5567-extract-subnet-arpa

commit e16b3ce
Author: Eugene Burkov <[email protected]>
Date:   Fri Mar 17 13:58:58 2023 +0300

    dnsforward: use netip

commit 265b08c
Author: Eugene Burkov <[email protected]>
Date:   Thu Mar 16 19:11:49 2023 +0300

    dnsforward: imp tests more

commit 53a839c
Author: Eugene Burkov <[email protected]>
Date:   Thu Mar 16 19:09:15 2023 +0300

    dnsforward: imp naming in tests

commit 74dcccb
Author: Eugene Burkov <[email protected]>
Date:   Thu Mar 16 18:59:12 2023 +0300

    dnsforward: imp code & tests more

commit da8badf
Author: Eugene Burkov <[email protected]>
Date:   Wed Mar 15 14:52:48 2023 +0300

    all: log changes

commit c491cbf
Merge: 74a9317 2b5e485
Author: Eugene Burkov <[email protected]>
Date:   Wed Mar 15 14:44:31 2023 +0300

    Merge branch 'master' into 5567-extract-subnet-arpa

commit 74a9317
Author: Eugene Burkov <[email protected]>
Date:   Wed Mar 15 14:42:55 2023 +0300

    dnsforward: imp code, docs

commit 17df1a0
Author: Eugene Burkov <[email protected]>
Date:   Tue Mar 14 19:49:10 2023 +0300

    dnsforward: extract subnet from arpa
  • Loading branch information
@EugeneOne1
EugeneOne1 committed Mar 17, 2023
1 parent 9f7a582 commit 48431f8
Show file tree
Hide file tree
Showing 5 changed files with 239 additions and 23 deletions.
8 changes: 7 additions & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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.
-->
Expand Down
112 changes: 96 additions & 16 deletions internal/dnsforward/dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/binary"
"net"
"net/netip"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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
Expand Down
126 changes: 126 additions & 0 deletions internal/dnsforward/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
6 changes: 3 additions & 3 deletions internal/dnsforward/http.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
10 changes: 7 additions & 3 deletions internal/dnsforward/http_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: "",
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand Down

0 comments on commit 48431f8

Please sign in to comment.