Pull request: filtering: fix legacy rewrite wildcards

Updates #3343. Squashed commit of the following: commit ab3c3e002a6d2a11bc3207fdaaeb292aaa194907 Author: Ainar Garipov <A.Garipov@AdGuard.COM> Date: Mon Jul 12 20:58:57 2021 +0300 filtering: fix legacy rewrite wildcards
2021-07-12 21:10:39 +03:00 · 2021-07-12 21:10:39 +03:00 · 194ea6ef95
commit 194ea6ef95
parent f419896ec6
4 changed files with 112 additions and 61 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -62,6 +62,7 @@ released by then.

 ### Fixed

+- Conflicts between IPv4 and IPv6 DNS rewrites ([#3343]).
 - Letter case mismatches in `CNAME` filtering ([#3335]).
 - Occasional breakages on network errors with DNS-over-HTTP upstreams ([#3217]).
 - Errors when setting static IP on Linux ([#3257]).
@ -103,6 +104,7 @@ released by then.
 [#3256]: https://github.com/AdguardTeam/AdGuardHome/issues/3256
 [#3257]: https://github.com/AdguardTeam/AdGuardHome/issues/3257
 [#3335]: https://github.com/AdguardTeam/AdGuardHome/issues/3335
+[#3343]: https://github.com/AdguardTeam/AdGuardHome/issues/3343



--- a/internal/filtering/filtering.go
+++ b/internal/filtering/filtering.go
@ -492,7 +492,7 @@ func (d *DNSFilter) processRewrites(host string, qtype uint16) (res Result) {
 	d.confLock.RLock()
 	defer d.confLock.RUnlock()

-	rr := findRewrites(d.Rewrites, host)
+	rr := findRewrites(d.Rewrites, host, qtype)
 	if len(rr) != 0 {
 		res.Reason = Rewritten
 	}
@ -517,15 +517,14 @@ func (d *DNSFilter) processRewrites(host string, qtype uint16) (res Result) {

 		cnames.Add(host)
 		res.CanonName = rr[0].Answer
-		rr = findRewrites(d.Rewrites, host)
+		rr = findRewrites(d.Rewrites, host, qtype)
 	}

 	for _, r := range rr {
-		if (r.Type == dns.TypeA && qtype == dns.TypeA) ||
-			(r.Type == dns.TypeAAAA && qtype == dns.TypeAAAA) {
-
+		if r.Type == qtype && (qtype == dns.TypeA || qtype == dns.TypeAAAA) {
 			if r.IP == nil { // IP exception
-				res.Reason = 0
+				res.Reason = NotFilteredNotFound
+
 				return res
 			}

--- a/internal/filtering/rewrites.go
+++ b/internal/filtering/rewrites.go
@ -15,10 +15,16 @@ import (

 // RewriteEntry is a rewrite array element
 type RewriteEntry struct {
+	// Domain is the domain for which this rewrite should work.
 	Domain string `yaml:"domain"`
-	Answer string `yaml:"answer"` // IP address or canonical name
-	Type   uint16 `yaml:"-"`      // DNS record type: CNAME, A or AAAA
-	IP     net.IP `yaml:"-"`      // Parsed IP address (if Type is A or AAAA)
+	// Answer is the IP address, canonical name, or one of the special
+	// values: "A" or "AAAA".
+	Answer string `yaml:"answer"`
+	// IP is the IP address that should be used in the response if Type is
+	// A or AAAA.
+	IP net.IP `yaml:"-"`
+	// Type is the DNS record type: A, AAAA, or CNAME.
+	Type uint16 `yaml:"-"`
 }

 func (r *RewriteEntry) equals(b RewriteEntry) bool {
@ -26,27 +32,32 @@ func (r *RewriteEntry) equals(b RewriteEntry) bool {
 }

 func isWildcard(host string) bool {
-	return len(host) >= 2 &&
-		host[0] == '*' && host[1] == '.'
+	return len(host) > 1 && host[0] == '*' && host[1] == '.'
 }

-// Return TRUE of host name matches a wildcard pattern
-func matchDomainWildcard(host, wildcard string) bool {
-	return isWildcard(wildcard) &&
-		strings.HasSuffix(host, wildcard[1:])
+// matchDomainWildcard returns true if host matches the wildcard pattern.
+func matchDomainWildcard(host, wildcard string) (ok bool) {
+	return isWildcard(wildcard) && strings.HasSuffix(host, wildcard[1:])
 }

-type rewritesArray []RewriteEntry
+// rewritesSorted is a slice of legacy rewrites for sorting.
+//
+// The sorting priority:
+//
+//   A and AAAA > CNAME
+//   wildcard > exact
+//   lower level wildcard > higher level wildcard
+//
+type rewritesSorted []RewriteEntry

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

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

-// Priority:
-//  . CNAME < A/AAAA;
-//  . exact < wildcard;
-//  . higher level wildcard < lower level wildcard
-func (a rewritesArray) Less(i, j int) bool {
+// Less implements the sort.Interface interface for legacyRewritesSorted.
+func (a rewritesSorted) Less(i, j int) 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 {
@ -67,31 +78,37 @@ func (a rewritesArray) Less(i, j int) bool {
 	return len(a[i].Domain) > len(a[j].Domain)
 }

-// Prepare entry for use
+// prepare prepares the a new or decoded entry.
 func (r *RewriteEntry) prepare() {
-	if r.Answer == "AAAA" {
+	switch r.Answer {
+	case "AAAA":
 		r.IP = nil
 		r.Type = dns.TypeAAAA
+
 		return
-	} else if r.Answer == "A" {
+	case "A":
 		r.IP = nil
 		r.Type = dns.TypeA
+
 		return
+	default:
+		// Go on.
 	}

 	ip := net.ParseIP(r.Answer)
 	if ip == nil {
 		r.Type = dns.TypeCNAME
+
 		return
 	}

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

@ -101,19 +118,23 @@ func (d *DNSFilter) prepareRewrites() {
 	}
 }

-// Get the list of matched rewrite entries.
-// Priority: CNAME, A/AAAA;  exact, wildcard.
-// If matched exactly, don't return wildcard entries.
-// If matched by several wildcards, select the more specific one
-func findRewrites(a []RewriteEntry, host string) []RewriteEntry {
-	rr := rewritesArray{}
+// findRewrites returns the list of matched rewrite entries.  The priority is:
+// CNAME, then A and AAAA; exact, then wildcard.  If the 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(a []RewriteEntry, host string, qtype uint16) []RewriteEntry {
+	rr := rewritesSorted{}
 	for _, r := range a {
-		if r.Domain != host {
-			if !matchDomainWildcard(host, r.Domain) {
-				continue
-			}
+		if r.Domain != host && !matchDomainWildcard(host, r.Domain) {
+			continue
+		}
+
+		// Return CNAMEs for all types requests, but only the
+		// appropriate ones for A and AAAA.
+		if r.Type == dns.TypeCNAME ||
+			(r.Type == qtype && (qtype == dns.TypeA || qtype == dns.TypeAAAA)) {
+			rr = append(rr, r)
 		}
-		rr = append(rr, r)
 	}

 	if len(rr) == 0 {
--- a/internal/filtering/rewrites_test.go
+++ b/internal/filtering/rewrites_test.go
@ -57,59 +57,75 @@ func TestRewrites(t *testing.T) {
 	}, {
 		Domain: "a.host3.com",
 		Answer: "x.host.com",
+	}, {
+		Domain: "*.hostboth.com",
+		Answer: "1.2.3.6",
+	}, {
+		Domain: "*.hostboth.com",
+		Answer: "1234::5678",
 	}}
 	d.prepareRewrites()

 	testCases := []struct {
 		name      string
 		host      string
-		dtyp      uint16
 		wantCName string
 		wantVals  []net.IP
+		dtyp      uint16
 	}{{
-		name: "not_filtered_not_found",
-		host: "hoost.com",
-		dtyp: dns.TypeA,
+		name:      "not_filtered_not_found",
+		host:      "hoost.com",
+		wantCName: "",
+		wantVals:  nil,
+		dtyp:      dns.TypeA,
 	}, {
 		name:      "rewritten_a",
 		host:      "www.host.com",
-		dtyp:      dns.TypeA,
 		wantCName: "host.com",
 		wantVals:  []net.IP{{1, 2, 3, 4}, {1, 2, 3, 5}},
+		dtyp:      dns.TypeA,
 	}, {
 		name:      "rewritten_aaaa",
 		host:      "www.host.com",
-		dtyp:      dns.TypeAAAA,
 		wantCName: "host.com",
 		wantVals:  []net.IP{net.ParseIP("1:2:3::4")},
+		dtyp:      dns.TypeAAAA,
 	}, {
-		name:     "wildcard_match",
-		host:     "abc.host.com",
-		dtyp:     dns.TypeA,
-		wantVals: []net.IP{{1, 2, 3, 5}},
+		name:      "wildcard_match",
+		host:      "abc.host.com",
+		wantCName: "",
+		wantVals:  []net.IP{{1, 2, 3, 5}},
+		dtyp:      dns.TypeA,
 	}, {
-		name:     "wildcard_override",
-		host:     "a.host.com",
-		dtyp:     dns.TypeA,
-		wantVals: []net.IP{{1, 2, 3, 4}},
+		name:      "wildcard_override",
+		host:      "a.host.com",
+		wantCName: "",
+		wantVals:  []net.IP{{1, 2, 3, 4}},
+		dtyp:      dns.TypeA,
 	}, {
 		name:      "wildcard_cname_interaction",
 		host:      "www.host2.com",
-		dtyp:      dns.TypeA,
 		wantCName: "host.com",
 		wantVals:  []net.IP{{1, 2, 3, 4}, {1, 2, 3, 5}},
+		dtyp:      dns.TypeA,
 	}, {
 		name:      "two_cnames",
 		host:      "b.host.com",
-		dtyp:      dns.TypeA,
 		wantCName: "somehost.com",
 		wantVals:  []net.IP{{0, 0, 0, 0}},
+		dtyp:      dns.TypeA,
 	}, {
 		name:      "two_cnames_and_wildcard",
 		host:      "b.host3.com",
-		dtyp:      dns.TypeA,
 		wantCName: "x.host.com",
 		wantVals:  []net.IP{{1, 2, 3, 5}},
+		dtyp:      dns.TypeA,
+	}, {
+		name:      "issue3343",
+		host:      "www.hostboth.com",
+		wantCName: "",
+		wantVals:  []net.IP{net.ParseIP("1234::5678")},
+		dtyp:      dns.TypeAAAA,
 	}}

 	for _, tc := range testCases {
@ -143,12 +159,15 @@ func TestRewritesLevels(t *testing.T) {
 	d.Rewrites = []RewriteEntry{{
 		Domain: "host.com",
 		Answer: "1.1.1.1",
+		Type:   dns.TypeA,
 	}, {
 		Domain: "*.host.com",
 		Answer: "2.2.2.2",
+		Type:   dns.TypeA,
 	}, {
 		Domain: "*.sub.host.com",
 		Answer: "3.3.3.3",
+		Type:   dns.TypeA,
 	}}
 	d.prepareRewrites()

@ -234,53 +253,61 @@ func TestRewritesExceptionIP(t *testing.T) {
 	d.Rewrites = []RewriteEntry{{
 		Domain: "host.com",
 		Answer: "1.2.3.4",
+		Type:   dns.TypeA,
 	}, {
 		Domain: "host.com",
 		Answer: "AAAA",
+		Type:   dns.TypeAAAA,
 	}, {
 		Domain: "host2.com",
 		Answer: "::1",
+		Type:   dns.TypeAAAA,
 	}, {
 		Domain: "host2.com",
 		Answer: "A",
+		Type:   dns.TypeA,
 	}, {
 		Domain: "host3.com",
 		Answer: "A",
+		Type:   dns.TypeA,
 	}}
 	d.prepareRewrites()

 	testCases := []struct {
 		name string
 		host string
-		dtyp uint16
 		want []net.IP
+		dtyp uint16
 	}{{
 		name: "match_A",
 		host: "host.com",
-		dtyp: dns.TypeA,
 		want: []net.IP{{1, 2, 3, 4}},
+		dtyp: dns.TypeA,
 	}, {
 		name: "exception_AAAA_host.com",
 		host: "host.com",
+		want: nil,
 		dtyp: dns.TypeAAAA,
 	}, {
 		name: "exception_A_host2.com",
 		host: "host2.com",
+		want: nil,
 		dtyp: dns.TypeA,
 	}, {
 		name: "match_AAAA_host2.com",
 		host: "host2.com",
-		dtyp: dns.TypeAAAA,
 		want: []net.IP{net.ParseIP("::1")},
+		dtyp: dns.TypeAAAA,
 	}, {
 		name: "exception_A_host3.com",
 		host: "host3.com",
+		want: nil,
 		dtyp: dns.TypeA,
 	}, {
 		name: "match_AAAA_host3.com",
 		host: "host3.com",
+		want: nil,
 		dtyp: dns.TypeAAAA,
-		want: []net.IP{},
 	}}

 	for _, tc := range testCases {
@ -293,7 +320,9 @@ func TestRewritesExceptionIP(t *testing.T) {
 			}

 			assert.Equal(t, Rewritten, r.Reason)
+
 			require.Len(t, r.IPList, len(tc.want))
+
 			for _, ip := range tc.want {
 				assert.True(t, ip.Equal(r.IPList[0]))
 			}