Pull request: filtering: fix rw to subdomain

Updates #4016. Squashed commit of the following: commit 83bb15c5a5098103cd17e76b49f456fb4fa73408 Merge: 81905503 313555b1 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Dec 27 19:36:44 2021 +0300 Merge branch 'master' into 4016-rw-subdomain commit 81905503c977c004d7ddca1d4e7537bf76443a6e Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Dec 27 19:35:51 2021 +0300 filtering: fix self reqs commit b706f481f00232d28dade0bd747a7496753c7deb Merge: 29cf83de 661f4ece Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Dec 27 19:13:08 2021 +0300 Merge branch 'master' into 4016-rw-subdomain commit 29cf83de8e3ff60ea1c471c2a161055b1377392d Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Dec 27 19:07:08 2021 +0300 all: fix docs commit 9213fd8ec2b81e65b1198ab241400065f14684b1 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Dec 27 18:44:06 2021 +0300 filtering: fix rw to subdomain
2021-12-27 19:40:39 +03:00
parent 313555b10c
commit dea8a585f8
19 changed files with 193 additions and 105 deletions
--- a/internal/filtering/filtering.go
+++ b/internal/filtering/filtering.go
@@ -28,7 +28,7 @@ import (

 // The IDs of built-in filter lists.
 //
-// Keep in sync with client/src/helpers/contants.js.
+// Keep in sync with client/src/helpers/constants.js.
 const (
 	CustomListID = -iota
 	SysHostsListID
@@ -80,7 +80,7 @@ type Config struct {
 	ParentalCacheSize     uint `yaml:"parental_cache_size"`     // (in bytes)
 	CacheTime             uint `yaml:"cache_time"`              // Element's TTL (in minutes)

-	Rewrites []RewriteEntry `yaml:"rewrites"`
+	Rewrites []*LegacyRewrite `yaml:"rewrites"`

 	// Names of services to block (globally).
 	// Per-client settings can override this configuration.
@@ -161,9 +161,14 @@ type DNSFilter struct {

 // Filter represents a filter list
 type Filter struct {
-	ID       int64  // auto-assigned when filter is added (see nextFilterID)
-	Data     []byte `yaml:"-"` // List of rules divided by '\n'
-	FilePath string `yaml:"-"` // Path to a filtering rules file
+	// FilePath is the path to a filtering rules list file.
+	FilePath string `yaml:"-"`
+
+	// Data is the content of the file.
+	Data []byte `yaml:"-"`
+
+	// ID is automatically assigned when filter is added using nextFilterID.
+	ID int64
 }

 // Reason holds an enum detailing why it was filtered or not filtered
@@ -281,8 +286,14 @@ func (d *DNSFilter) WriteDiskConfig(c *Config) {
 	c.Rewrites = cloneRewrites(c.Rewrites)
 }

-func cloneRewrites(entries []RewriteEntry) (clone []RewriteEntry) {
-	return append([]RewriteEntry(nil), entries...)
+// cloneRewrites returns a deep copy of entries.
+func cloneRewrites(entries []*LegacyRewrite) (clone []*LegacyRewrite) {
+	clone = make([]*LegacyRewrite, len(entries))
+	for i, rw := range entries {
+		clone[i] = rw.clone()
+	}
+
+	return clone
 }

 // SetFilters - set new filters (synchronously or asynchronously)
@@ -477,10 +488,8 @@ func (d *DNSFilter) matchSysHosts(
 }

 // matchSysHostsIntl actually matches the request.  It's separated to avoid
-// perfoming checks twice.
-func (d *DNSFilter) matchSysHostsIntl(
-	req *urlfilter.DNSRequest,
-) (res Result, err error) {
+// performing checks twice.
+func (d *DNSFilter) matchSysHostsIntl(req *urlfilter.DNSRequest) (res Result, err error) {
 	dnsres, _ := d.EtcHosts.MatchRequest(*req)
 	if dnsres == nil {
 		return res, nil
@@ -530,15 +539,25 @@ func (d *DNSFilter) processRewrites(host string, qtype uint16) (res Result) {
 	cnames := stringutil.NewSet()
 	origHost := host
 	for matched && len(rewrites) > 0 && rewrites[0].Type == dns.TypeCNAME {
-		rwAns := rewrites[0].Answer
+		rw := rewrites[0]
+		rwPat := rw.Domain
+		rwAns := rw.Answer

 		log.Debug("rewrite: cname for %s is %s", host, rwAns)

-		if host == rwAns {
-			// Rewrite of a domain onto itself is an exception rule.
-			res.Reason = NotFilteredNotFound
+		if origHost == rwAns || rwPat == rwAns {
+			// Either a request for the hostname itself or a rewrite of
+			// a pattern onto itself, both of which are an exception rules.
+			// Return a not filtered result.
+			return Result{}
+		} else if host == rwAns && isWildcard(rwPat) {
+			// An "*.example.com → sub.example.com" rewrite matching in a loop.
+			//
+			// See https://github.com/AdguardTeam/AdGuardHome/issues/4016.

-			return res
+			res.CanonName = host
+
+			break
 		}

 		host = rwAns
@@ -560,7 +579,7 @@ func (d *DNSFilter) processRewrites(host string, qtype uint16) (res Result) {

 // setRewriteResult sets the Reason or IPList of res if necessary.  res must not
 // be nil.
-func setRewriteResult(res *Result, host string, rewrites []RewriteEntry, qtype uint16) {
+func setRewriteResult(res *Result, host string, rewrites []*LegacyRewrite, qtype uint16) {
 	for _, rw := range rewrites {
 		if rw.Type == qtype && (qtype == dns.TypeA || qtype == dns.TypeAAAA) {
 			if rw.IP == nil {
@@ -932,12 +951,18 @@ func New(c *Config, blockFilters []Filter) (d *DNSFilter) {
 	err := d.initSecurityServices()
 	if err != nil {
 		log.Error("filtering: initialize services: %s", err)
+
 		return nil
 	}

 	if c != nil {
 		d.Config = *c
-		d.prepareRewrites()
+		err = d.prepareRewrites()
+		if err != nil {
+			log.Error("rewrites: preparing: %s", err)
+
+			return nil
+		}
 	}

 	bsvcs := []string{}
--- a/internal/filtering/rewrites.go
+++ b/internal/filtering/rewrites.go
@@ -4,19 +4,24 @@ package filtering

 import (
 	"encoding/json"
+	"fmt"
 	"net"
 	"net/http"
 	"sort"
 	"strings"

 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
+	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/miekg/dns"
 )

-// RewriteEntry is a rewrite array element
-type RewriteEntry struct {
-	// Domain is the domain for which this rewrite should work.
+// LegacyRewrite is a single legacy DNS rewrite record.
+//
+// Instances of *LegacyRewrite must never be nil.
+type LegacyRewrite struct {
+	// Domain is the domain pattern for which this rewrite should work.
 	Domain string `yaml:"domain"`

 	// Answer is the IP address, canonical name, or one of the special
@@ -24,77 +29,96 @@ type RewriteEntry struct {
 	Answer string `yaml:"answer"`

 	// IP is the IP address that should be used in the response if Type is
-	// A or AAAA.
+	// dns.TypeA or dns.TypeAAAA.
 	IP net.IP `yaml:"-"`

 	// Type is the DNS record type: A, AAAA, or CNAME.
 	Type uint16 `yaml:"-"`
 }

-// equal returns true if the entry is considered equal to the other.
-func (e *RewriteEntry) equal(other RewriteEntry) (ok bool) {
-	return e.Domain == other.Domain && e.Answer == other.Answer
+// clone returns a deep clone of rw.
+func (rw *LegacyRewrite) clone() (cloneRW *LegacyRewrite) {
+	return &LegacyRewrite{
+		Domain: rw.Domain,
+		Answer: rw.Answer,
+		IP:     netutil.CloneIP(rw.IP),
+		Type:   rw.Type,
+	}
 }

-// matchesQType returns true if the entry matched qtype.
-func (e *RewriteEntry) matchesQType(qtype uint16) (ok bool) {
+// equal returns true if the rw is equal to the other.
+func (rw *LegacyRewrite) equal(other *LegacyRewrite) (ok bool) {
+	return rw.Domain == other.Domain && rw.Answer == other.Answer
+}
+
+// matchesQType returns true if the entry matches the question type qt.
+func (rw *LegacyRewrite) matchesQType(qt uint16) (ok bool) {
 	// Add CNAMEs, since they match for all types requests.
-	if e.Type == dns.TypeCNAME {
+	if rw.Type == dns.TypeCNAME {
 		return true
 	}

 	// Reject types other than A and AAAA.
-	if qtype != dns.TypeA && qtype != dns.TypeAAAA {
+	if qt != dns.TypeA && qt != dns.TypeAAAA {
 		return false
 	}

 	// If the types match or the entry is set to allow only the other type,
 	// include them.
-	return e.Type == qtype || e.IP == nil
+	return rw.Type == qt || rw.IP == nil
 }

 // normalize makes sure that the a new or decoded entry is normalized with
 // regards to domain name case, IP length, and so on.
-func (e *RewriteEntry) normalize() {
-	// TODO(a.garipov): Write a case-agnostic version of strings.HasSuffix
-	// and use it in matchDomainWildcard instead of using strings.ToLower
+//
+// If rw is nil, it returns an errors.
+func (rw *LegacyRewrite) normalize() (err error) {
+	if rw == nil {
+		return errors.Error("nil rewrite entry")
+	}
+
+	// TODO(a.garipov): Write a case-agnostic version of strings.HasSuffix and
+	// use it in matchDomainWildcard instead of using strings.ToLower
 	// everywhere.
-	e.Domain = strings.ToLower(e.Domain)
+	rw.Domain = strings.ToLower(rw.Domain)

-	switch e.Answer {
+	switch rw.Answer {
 	case "AAAA":
-		e.IP = nil
-		e.Type = dns.TypeAAAA
+		rw.IP = nil
+		rw.Type = dns.TypeAAAA

-		return
+		return nil
 	case "A":
-		e.IP = nil
-		e.Type = dns.TypeA
+		rw.IP = nil
+		rw.Type = dns.TypeA

-		return
+		return nil
 	default:
 		// Go on.
 	}

-	ip := net.ParseIP(e.Answer)
+	ip := net.ParseIP(rw.Answer)
 	if ip == nil {
-		e.Type = dns.TypeCNAME
+		rw.Type = dns.TypeCNAME

-		return
+		return nil
 	}

 	ip4 := ip.To4()
 	if ip4 != nil {
-		e.IP = ip4
-		e.Type = dns.TypeA
+		rw.IP = ip4
+		rw.Type = dns.TypeA
 	} else {
-		e.IP = ip
-		e.Type = dns.TypeAAAA
+		rw.IP = ip
+		rw.Type = dns.TypeAAAA
 	}
+
+	return nil
 }

-func isWildcard(host string) bool {
-	return len(host) > 1 && host[0] == '*' && host[1] == '.'
+// isWildcard returns true if pat is a wildcard domain pattern.
+func isWildcard(pat string) bool {
+	return len(pat) > 1 && pat[0] == '*' && pat[1] == '.'
 }

 // matchDomainWildcard returns true if host matches the wildcard pattern.
@@ -110,16 +134,16 @@ func matchDomainWildcard(host, wildcard string) (ok bool) {
 //   wildcard > exact
 //   lower level wildcard > higher level wildcard
 //
-type rewritesSorted []RewriteEntry
+type rewritesSorted []*LegacyRewrite

 // Len implements the sort.Interface interface for legacyRewritesSorted.
-func (a rewritesSorted) Len() int { return len(a) }
+func (a rewritesSorted) Len() (l int) { return len(a) }

 // Swap implements the sort.Interface interface for legacyRewritesSorted.
 func (a rewritesSorted) Swap(i, j int) { a[i], a[j] = a[j], a[i] }

 // Less implements the sort.Interface interface for legacyRewritesSorted.
-func (a rewritesSorted) Less(i, j int) bool {
+func (a rewritesSorted) Less(i, j int) (less bool) {
 	if a[i].Type == dns.TypeCNAME && a[j].Type != dns.TypeCNAME {
 		return true
 	} else if a[i].Type != dns.TypeCNAME && a[j].Type == dns.TypeCNAME {
@@ -136,14 +160,20 @@ func (a rewritesSorted) Less(i, j int) bool {
 		}
 	}

-	// both are wildcards
+	// Both are wildcards.
 	return len(a[i].Domain) > len(a[j].Domain)
 }

-func (d *DNSFilter) prepareRewrites() {
-	for i := range d.Rewrites {
-		d.Rewrites[i].normalize()
+// prepareRewrites normalizes and validates all legacy DNS rewrites.
+func (d *DNSFilter) prepareRewrites() (err error) {
+	for i, r := range d.Rewrites {
+		err = r.normalize()
+		if err != nil {
+			return fmt.Errorf("at index %d: %w", i, err)
+		}
 	}
+
+	return nil
 }

 // findRewrites returns the list of matched rewrite entries.  If rewrites are
@@ -154,10 +184,10 @@ func (d *DNSFilter) prepareRewrites() {
 // host is matched exactly, wildcard entries aren't returned.  If the host
 // matched by wildcards, return the most specific for the question type.
 func findRewrites(
-	entries []RewriteEntry,
+	entries []*LegacyRewrite,
 	host string,
 	qtype uint16,
-) (rewrites []RewriteEntry, matched bool) {
+) (rewrites []*LegacyRewrite, matched bool) {
 	for _, e := range entries {
 		if e.Domain != host && !matchDomainWildcard(host, e.Domain) {
 			continue
@@ -224,23 +254,32 @@ func (d *DNSFilter) handleRewriteList(w http.ResponseWriter, r *http.Request) {
 }

 func (d *DNSFilter) handleRewriteAdd(w http.ResponseWriter, r *http.Request) {
-	jsent := rewriteEntryJSON{}
-	err := json.NewDecoder(r.Body).Decode(&jsent)
+	rwJSON := rewriteEntryJSON{}
+	err := json.NewDecoder(r.Body).Decode(&rwJSON)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "json.Decode: %s", err)

 		return
 	}

-	ent := RewriteEntry{
-		Domain: jsent.Domain,
-		Answer: jsent.Answer,
+	rw := &LegacyRewrite{
+		Domain: rwJSON.Domain,
+		Answer: rwJSON.Answer,
 	}
-	ent.normalize()
+
+	err = rw.normalize()
+	if err != nil {
+		// Shouldn't happen currently, since normalize only returns a non-nil
+		// error when a rewrite is nil, but be change-proof.
+		aghhttp.Error(r, w, http.StatusBadRequest, "normalizing: %s", err)
+
+		return
+	}
+
 	d.confLock.Lock()
-	d.Config.Rewrites = append(d.Config.Rewrites, ent)
+	d.Config.Rewrites = append(d.Config.Rewrites, rw)
 	d.confLock.Unlock()
-	log.Debug("rewrite: added element: %s -> %s [%d]", ent.Domain, ent.Answer, len(d.Config.Rewrites))
+	log.Debug("rewrite: added element: %s -> %s [%d]", rw.Domain, rw.Answer, len(d.Config.Rewrites))

 	d.Config.ConfigModified()
 }
@@ -254,11 +293,12 @@ func (d *DNSFilter) handleRewriteDelete(w http.ResponseWriter, r *http.Request)
 		return
 	}

-	entDel := RewriteEntry{
+	entDel := &LegacyRewrite{
 		Domain: jsent.Domain,
 		Answer: jsent.Answer,
 	}
-	arr := []RewriteEntry{}
+	arr := []*LegacyRewrite{}
+
 	d.confLock.Lock()
 	for _, ent := range d.Config.Rewrites {
 		if ent.equal(entDel) {
--- a/internal/filtering/rewrites_test.go
+++ b/internal/filtering/rewrites_test.go
@@ -15,7 +15,7 @@ func TestRewrites(t *testing.T) {
 	d := newForTest(t, nil, nil)
 	t.Cleanup(d.Close)

-	d.Rewrites = []RewriteEntry{{
+	d.Rewrites = []*LegacyRewrite{{
 		// This one and below are about CNAME, A and AAAA.
 		Domain: "somecname",
 		Answer: "somehost.com",
@@ -66,8 +66,12 @@ func TestRewrites(t *testing.T) {
 	}, {
 		Domain: "BIGHOST.COM",
 		Answer: "1.2.3.7",
+	}, {
+		Domain: "*.issue4016.com",
+		Answer: "sub.issue4016.com",
 	}}
-	d.prepareRewrites()
+
+	require.NoError(t, d.prepareRewrites())

 	testCases := []struct {
 		name       string
@@ -153,6 +157,20 @@ func TestRewrites(t *testing.T) {
 		wantIPs:    nil,
 		wantReason: Rewritten,
 		dtyp:       dns.TypeHTTPS,
+	}, {
+		name:       "issue4016",
+		host:       "www.issue4016.com",
+		wantCName:  "sub.issue4016.com",
+		wantIPs:    nil,
+		wantReason: Rewritten,
+		dtyp:       dns.TypeA,
+	}, {
+		name:       "issue4016_self",
+		host:       "sub.issue4016.com",
+		wantCName:  "",
+		wantIPs:    nil,
+		wantReason: NotFilteredNotFound,
+		dtyp:       dns.TypeA,
 	}}

 	for _, tc := range testCases {
@@ -173,7 +191,7 @@ func TestRewritesLevels(t *testing.T) {
 	d := newForTest(t, nil, nil)
 	t.Cleanup(d.Close)
 	// Exact host, wildcard L2, wildcard L3.
-	d.Rewrites = []RewriteEntry{{
+	d.Rewrites = []*LegacyRewrite{{
 		Domain: "host.com",
 		Answer: "1.1.1.1",
 		Type:   dns.TypeA,
@@ -186,7 +204,8 @@ func TestRewritesLevels(t *testing.T) {
 		Answer: "3.3.3.3",
 		Type:   dns.TypeA,
 	}}
-	d.prepareRewrites()
+
+	require.NoError(t, d.prepareRewrites())

 	testCases := []struct {
 		name string
@@ -219,7 +238,7 @@ func TestRewritesExceptionCNAME(t *testing.T) {
 	d := newForTest(t, nil, nil)
 	t.Cleanup(d.Close)
 	// Wildcard and exception for a sub-domain.
-	d.Rewrites = []RewriteEntry{{
+	d.Rewrites = []*LegacyRewrite{{
 		Domain: "*.host.com",
 		Answer: "2.2.2.2",
 	}, {
@@ -229,7 +248,8 @@ func TestRewritesExceptionCNAME(t *testing.T) {
 		Domain: "*.sub.host.com",
 		Answer: "*.sub.host.com",
 	}}
-	d.prepareRewrites()
+
+	require.NoError(t, d.prepareRewrites())

 	testCases := []struct {
 		name string
@@ -253,7 +273,7 @@ func TestRewritesExceptionCNAME(t *testing.T) {
 		t.Run(tc.name, func(t *testing.T) {
 			r := d.processRewrites(tc.host, dns.TypeA)
 			if tc.want == nil {
-				assert.Equal(t, NotFilteredNotFound, r.Reason)
+				assert.Equal(t, NotFilteredNotFound, r.Reason, "got %s", r.Reason)

 				return
 			}
@@ -269,7 +289,7 @@ func TestRewritesExceptionIP(t *testing.T) {
 	d := newForTest(t, nil, nil)
 	t.Cleanup(d.Close)
 	// Exception for AAAA record.
-	d.Rewrites = []RewriteEntry{{
+	d.Rewrites = []*LegacyRewrite{{
 		Domain: "host.com",
 		Answer: "1.2.3.4",
 		Type:   dns.TypeA,
@@ -290,7 +310,8 @@ func TestRewritesExceptionIP(t *testing.T) {
 		Answer: "A",
 		Type:   dns.TypeA,
 	}}
-	d.prepareRewrites()
+
+	require.NoError(t, d.prepareRewrites())

 	testCases := []struct {
 		name string