diff --git a/CHANGELOG.md b/CHANGELOG.md index 38ec0381..28d07c0e 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/internal/dnsforward/dns.go b/internal/dnsforward/dns.go index a93ab0bd..0e181e10 100644 --- a/internal/dnsforward/dns.go +++ b/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 -// 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 @@ -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 diff --git a/internal/dnsforward/dns_test.go b/internal/dnsforward/dns_test.go index d3b91466..45362956 100644 --- a/internal/dnsforward/dns_test.go +++ b/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_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 { @@ -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.