Skip to content

Commit

Permalink
Pull request: 2889 internal hosts restriction
Browse files Browse the repository at this point in the history 
Merge in DNS/adguard-home from 2889-imp-autohosts to master

Closes AdguardTeam#2889.

Squashed commit of the following:

commit 1d3b649
Merge: abc6e1c 1a214ea
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Apr 8 17:59:51 2021 +0300

    Merge branch 'master' into 2889-imp-autohosts

commit abc6e1c
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Apr 8 17:34:56 2021 +0300

    dnsforward: imp code

commit 4b2b914
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Thu Apr 8 17:31:34 2021 +0300

    dnsforward: respond with nxdomain

commit 8146674
Author: Eugene Burkov <e.burkov@adguard.com>
Date:   Tue Apr 6 19:16:14 2021 +0300

    dnsforward: restrict the access to intl hosts for ext clients
  • Loading branch information
@EugeneOne1 @heyxkhoa
EugeneOne1 authored and heyxkhoa committed Mar 17, 2023
1 parent 835b90a commit 485eeb0
Show file tree
Hide file tree
Showing 3 changed files with 216 additions and 50 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ and this project adheres to

### Changed

- The access to the private hosts is now forbidden for users from external
networks ([#2889]).
- The reverse lookup for local addresses is now performed via local resolvers
([#2704]).
- Stricter validation of the IP addresses of static leases in the DHCP server
Expand Down Expand Up @@ -64,6 +66,7 @@ and this project adheres to
[#2828]: https://github.com/AdguardTeam/AdGuardHome/issues/2828
[#2835]: https://github.com/AdguardTeam/AdGuardHome/issues/2835
[#2838]: https://github.com/AdguardTeam/AdGuardHome/issues/2838
[#2889]: https://github.com/AdguardTeam/AdGuardHome/issues/2889
[#2927]: https://github.com/AdguardTeam/AdGuardHome/issues/2927


Expand Down
45 changes: 37 additions & 8 deletions internal/dnsforward/dns.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ type dnsContext struct {
// origReqDNSSEC shows if the DNSSEC flag in the original request from
// the client is set.
origReqDNSSEC bool
// isLocalClient shows if client's IP address is from locally-served
// network.
isLocalClient bool
}

// resultCode is the result of a request processing function.
Expand Down Expand Up @@ -81,6 +84,7 @@ func (s *Server) handleDNSRequest(_ *proxy.Proxy, d *proxy.DNSContext) error {
// appropriate handler.
mods := []modProcessFunc{
processInitial,
s.processDetermineLocal,
s.processInternalHosts,
s.processRestrictLocal,
s.processInternalIPAddrs,
Expand Down Expand Up @@ -191,6 +195,21 @@ func (s *Server) onDHCPLeaseChanged(flags int) {
s.tablePTRLock.Unlock()
}

// processDetermineLocal determines if the client's IP address is from
// locally-served network and saves the result into the context.
func (s *Server) processDetermineLocal(dctx *dnsContext) (rc resultCode) {
rc = resultCodeSuccess

var ip net.IP
if ip = IPFromAddr(dctx.proxyCtx.Addr); ip == nil {
return rc
}

dctx.isLocalClient = s.subnetDetector.IsLocallyServedNetwork(ip)

return rc
}

// hostToIP tries to get an IP leased by DHCP and returns the copy of address
// since the data inside the internal table may be changed while request
// processing. It's safe for concurrent use.
Expand Down Expand Up @@ -235,11 +254,22 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
return resultCodeSuccess
}

// TODO(e.burkov): Restrict the access for external clients.
d := dctx.proxyCtx
if !dctx.isLocalClient {
log.Debug("dns: %q requests for internal host", d.Addr)
d.Res = s.genNXDomain(req)

// Do not even put into query log.
return resultCodeFinish
}

ip, ok := s.hostToIP(host)
if !ok {
return resultCodeSuccess
// TODO(e.burkov): Inspect special cases when user want to apply
// some rules handled by other processors to the hosts with TLD.
d.Res = s.genNXDomain(req)

return resultCodeFinish
}

log.Debug("dns: internal record: %s -> %s", q.Name, ip)
Expand All @@ -257,8 +287,8 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
return resultCodeSuccess
}

// processRestrictLocal responds with empty answers to PTR requests for IP
// addresses in locally-served network from external clients.
// processRestrictLocal responds with NXDOMAIN to PTR requests for IP addresses
// in locally-served network from external clients.
func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
d := ctx.proxyCtx
req := d.Req
Expand All @@ -280,10 +310,9 @@ func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
// assume that all the DHCP leases we give are locally-served or at
// least don't need to be unaccessable externally.
if s.subnetDetector.IsLocallyServedNetwork(ip) {
clientIP := IPFromAddr(d.Addr)
if !s.subnetDetector.IsLocallyServedNetwork(clientIP) {
log.Debug("dns: %q requests for internal ip", clientIP)
d.Res = s.makeResponse(req)
if !ctx.isLocalClient {
log.Debug("dns: %q requests for internal ip", d.Addr)
d.Res = s.genNXDomain(req)

// Do not even put into query log.
return resultCodeFinish
Expand Down
218 changes: 176 additions & 42 deletions internal/dnsforward/dns_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"net"
"testing"

"github.com/AdguardTeam/AdGuardHome/internal/aghnet"
"github.com/AdguardTeam/AdGuardHome/internal/aghtest"
"github.com/AdguardTeam/AdGuardHome/internal/dnsfilter"
"github.com/AdguardTeam/dnsproxy/proxy"
Expand All @@ -13,56 +14,188 @@ import (
"github.com/stretchr/testify/require"
)

func TestServer_ProcessInternalHosts(t *testing.T) {
func TestServer_ProcessDetermineLocal(t *testing.T) {
snd, err := aghnet.NewSubnetDetector()
require.NoError(t, err)
s := &Server{
subnetDetector: snd,
}

testCases := []struct {
name string
cliIP net.IP
want bool
}{{
name: "local",
cliIP: net.IP{192, 168, 0, 1},
want: true,
}, {
name: "external",
cliIP: net.IP{250, 249, 0, 1},
want: false,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
proxyCtx := &proxy.DNSContext{
Addr: &net.TCPAddr{
IP: tc.cliIP,
},
}
dctx := &dnsContext{
proxyCtx: proxyCtx,
}
s.processDetermineLocal(dctx)

assert.Equal(t, tc.want, dctx.isLocalClient)
})
}
}

func TestServer_ProcessInternalHosts_localRestriction(t *testing.T) {
knownIP := net.IP{1, 2, 3, 4}

testCases := []struct {
name string
host string
suffix string
wantErrMsg string
wantIP net.IP
qtyp uint16
wantRes resultCode
isLocalCli bool
}{{
name: "success_external",
host: "example.com",
suffix: defaultAutohostSuffix,
wantErrMsg: "",
wantIP: nil,
qtyp: dns.TypeA,
wantRes: resultCodeSuccess,
}, {
name: "success_external_non_a",
host: "example.com",
suffix: defaultAutohostSuffix,
wantErrMsg: "",
wantIP: nil,
qtyp: dns.TypeCNAME,
wantRes: resultCodeSuccess,
}, {
name: "success_internal",
name: "local_client_success",
host: "example.lan",
suffix: defaultAutohostSuffix,
wantErrMsg: "",
wantIP: knownIP,
qtyp: dns.TypeA,
wantRes: resultCodeSuccess,
isLocalCli: true,
}, {
name: "success_internal_unknown",
host: "example-new.lan",
suffix: defaultAutohostSuffix,
wantErrMsg: "",
name: "local_client_unknown_host",
host: "wronghost.lan",
wantIP: nil,
qtyp: dns.TypeA,
wantRes: resultCodeSuccess,
wantRes: resultCodeFinish,
isLocalCli: true,
}, {
name: "success_internal_aaaa",
name: "external_client_known_host",
host: "example.lan",
suffix: defaultAutohostSuffix,
wantErrMsg: "",
wantIP: nil,
qtyp: dns.TypeAAAA,
wantRes: resultCodeSuccess,
wantRes: resultCodeFinish,
isLocalCli: false,
}, {
name: "external_client_unknown_host",
host: "wronghost.lan",
wantIP: nil,
wantRes: resultCodeFinish,
isLocalCli: false,
}}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
s := &Server{
autohostSuffix: defaultAutohostSuffix,
tableHostToIP: map[string]net.IP{
"example": knownIP,
},
}

req := &dns.Msg{
MsgHdr: dns.MsgHdr{
Id: dns.Id(),
},
Question: []dns.Question{{
Name: dns.Fqdn(tc.host),
Qtype: dns.TypeA,
Qclass: dns.ClassINET,
}},
}

dctx := &dnsContext{
proxyCtx: &proxy.DNSContext{
Req: req,
},
isLocalClient: tc.isLocalCli,
}

res := s.processInternalHosts(dctx)
require.Equal(t, tc.wantRes, res)
pctx := dctx.proxyCtx
if tc.wantRes == resultCodeFinish {
require.NotNil(t, pctx.Res)

assert.Equal(t, dns.RcodeNameError, pctx.Res.Rcode)
assert.Len(t, pctx.Res.Answer, 0)

return
}

if tc.wantIP == nil {
assert.Nil(t, pctx.Res)
} else {
require.NotNil(t, pctx.Res)

ans := pctx.Res.Answer
require.Len(t, ans, 1)

assert.Equal(t, tc.wantIP, ans[0].(*dns.A).A)
}
})
}
}

func TestServer_ProcessInternalHosts(t *testing.T) {
const (
examplecom = "example.com"
examplelan = "example.lan"
)

knownIP := net.IP{1, 2, 3, 4}
testCases := []struct {
name string
host string
suffix string
wantIP net.IP
wantRes resultCode
qtyp uint16
}{{
name: "success_external",
host: examplecom,
suffix: defaultAutohostSuffix,
wantIP: nil,
wantRes: resultCodeSuccess,
qtyp: dns.TypeA,
}, {
name: "success_external_non_a",
host: examplecom,
suffix: defaultAutohostSuffix,
wantIP: nil,
wantRes: resultCodeSuccess,
qtyp: dns.TypeCNAME,
}, {
name: "success_internal",
host: examplelan,
suffix: defaultAutohostSuffix,
wantIP: knownIP,
wantRes: resultCodeSuccess,
qtyp: dns.TypeA,
}, {
name: "success_internal_unknown",
host: "example-new.lan",
suffix: defaultAutohostSuffix,
wantIP: nil,
wantRes: resultCodeFinish,
qtyp: dns.TypeA,
}, {
name: "success_internal_aaaa",
host: examplelan,
suffix: defaultAutohostSuffix,
wantIP: nil,
wantRes: resultCodeSuccess,
qtyp: dns.TypeAAAA,
}, {
name: "success_custom_suffix",
host: "example.custom",
suffix: ".custom.",
wantIP: knownIP,
wantRes: resultCodeSuccess,
qtyp: dns.TypeA,
}}

for _, tc := range testCases {
Expand All @@ -89,20 +222,21 @@ func TestServer_ProcessInternalHosts(t *testing.T) {
proxyCtx: &proxy.DNSContext{
Req: req,
},
isLocalClient: true,
}

res := s.processInternalHosts(dctx)
pctx := dctx.proxyCtx
assert.Equal(t, tc.wantRes, res)
if tc.wantRes == resultCodeFinish {
require.NotNil(t, pctx.Res)
assert.Equal(t, dns.RcodeNameError, pctx.Res.Rcode)

if tc.wantErrMsg == "" {
assert.NoError(t, dctx.err)
} else {
require.Error(t, dctx.err)

assert.Equal(t, tc.wantErrMsg, dctx.err.Error())
return
}

pctx := dctx.proxyCtx
require.NoError(t, dctx.err)

if tc.qtyp == dns.TypeAAAA {
// TODO(a.garipov): Remove this special handling
// when we fully support AAAA.
Expand Down

0 comments on commit 485eeb0

Please sign in to comment.