Pull request: 2889 internal hosts restriction

Merge in DNS/adguard-home from 2889-imp-autohosts to master

Closes #2889.

Squashed commit of the following:

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

    Merge branch 'master' into 2889-imp-autohosts

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

    dnsforward: imp code

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

    dnsforward: respond with nxdomain

commit 814667417a1b02c152a034852858b96412874f85
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

CHANGELOG.md

@@ -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
@@ -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
 
 

internal/dnsforward/dns.go

@@ -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.
@@ -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,
@@ -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.
@@ -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)
@@ -257,8 +287,8 @@ func (s *Server) processInternalHosts(dctx *dnsContext) (rc resultCode) {
 	return resultCodeSuccess
 }
 
-// processRestrictLocal responds with empty answers to PTR requests for IP
+// processRestrictLocal responds with NXDOMAIN to PTR requests for IP addresses
-// addresses in locally-served network from external clients.
+// in locally-served network from external clients.
 func (s *Server) processRestrictLocal(ctx *dnsContext) (rc resultCode) {
 	d := ctx.proxyCtx
 	req := d.Req
@@ -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 !ctx.isLocalClient {
-		if !s.subnetDetector.IsLocallyServedNetwork(clientIP) {
+			log.Debug("dns: %q requests for internal ip", d.Addr)
-			log.Debug("dns: %q requests for internal ip", clientIP)
+			d.Res = s.genNXDomain(req)
-			d.Res = s.makeResponse(req)
 
 			// Do not even put into query log.
 			return resultCodeFinish

internal/dnsforward/dns_test.go

@@ -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"
@@ -13,56 +14,188 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+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
+		wantIP     net.IP
+		wantRes    resultCode
+		isLocalCli bool
+	}{{
+		name:       "local_client_success",
+		host:       "example.lan",
+		wantIP:     knownIP,
+		wantRes:    resultCodeSuccess,
+		isLocalCli: true,
+	}, {
+		name:       "local_client_unknown_host",
+		host:       "wronghost.lan",
+		wantIP:     nil,
+		wantRes:    resultCodeFinish,
+		isLocalCli: true,
+	}, {
+		name:       "external_client_known_host",
+		host:       "example.lan",
+		wantIP:     nil,
+		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
-		wantErrMsg string
 		wantIP  net.IP
-		qtyp       uint16
 		wantRes resultCode
+		qtyp    uint16
 	}{{
 		name:    "success_external",
-		host:       "example.com",
+		host:    examplecom,
 		suffix:  defaultAutohostSuffix,
-		wantErrMsg: "",
 		wantIP:  nil,
-		qtyp:       dns.TypeA,
 		wantRes: resultCodeSuccess,
+		qtyp:    dns.TypeA,
 	}, {
 		name:    "success_external_non_a",
-		host:       "example.com",
+		host:    examplecom,
 		suffix:  defaultAutohostSuffix,
-		wantErrMsg: "",
 		wantIP:  nil,
-		qtyp:       dns.TypeCNAME,
 		wantRes: resultCodeSuccess,
+		qtyp:    dns.TypeCNAME,
 	}, {
 		name:    "success_internal",
-		host:       "example.lan",
+		host:    examplelan,
 		suffix:  defaultAutohostSuffix,
-		wantErrMsg: "",
 		wantIP:  knownIP,
-		qtyp:       dns.TypeA,
 		wantRes: resultCodeSuccess,
+		qtyp:    dns.TypeA,
 	}, {
 		name:    "success_internal_unknown",
 		host:    "example-new.lan",
 		suffix:  defaultAutohostSuffix,
-		wantErrMsg: "",
 		wantIP:  nil,
+		wantRes: resultCodeFinish,
 		qtyp:    dns.TypeA,
-		wantRes:    resultCodeSuccess,
 	}, {
 		name:    "success_internal_aaaa",
-		host:       "example.lan",
+		host:    examplelan,
 		suffix:  defaultAutohostSuffix,
-		wantErrMsg: "",
 		wantIP:  nil,
-		qtyp:       dns.TypeAAAA,
 		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 {
@@ -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 == "" {
+				return
-				assert.NoError(t, dctx.err)
-			} else {
-				require.Error(t, dctx.err)
-
-				assert.Equal(t, tc.wantErrMsg, dctx.err.Error())
 			}
 
-			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.