Pull request: do not apply denyallow to IP addresses

Merge in DNS/urlfilter from denyallowips to master

Related to AdguardTeam/AdGuardHome#3175

Squashed commit of the following:

commit adb8306
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Dec 14 17:41:40 2021 +0300

    fix golangci-lint

commit becb0d7
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Dec 14 17:37:44 2021 +0300

    fix review issues

commit 028d7f8
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Dec 14 17:28:46 2021 +0300

    added bench, fixed specs

commit 13a71a8
Author: Andrey Meshkov <am@adguard.com>
Date:   Tue Dec 14 17:14:12 2021 +0300

    do not apply denyallow to IP addresses

bamboo-specs/bamboo.yaml

@@ -1,29 +1,31 @@
 ---
-version: 2
-plan:
- project-key: AGH
- key: URLFLTSPECS
- name: urlfilter - Build and run tests
- dockerGo: golang:1.16
+'version': 2
+'plan':
+ 'project-key': 'AGH'
+ 'key': 'URLFLTSPECS'
+ 'name': 'urlfilter - Build and run tests'
+'variables':
+ 'dockerGo': 'golang:1.17.5'
+ 'dockerLint': 'golangci/golangci-lint'
 
-stages:
- - Test:
- manual: 'false'
- final: 'false'
- jobs:
- - Lint
- - Test
+'stages':
+ - 'Test':
+ 'manual': 'false'
+ 'final': 'false'
+ 'jobs':
+ - 'Lint'
+ - 'Test'
 
-Lint:
- docker:
- image: golangci/golangci-lint
- key: LINT
- tasks:
- - checkout:
- force-clean-build: 'false'
- - script:
- interpreter: SHELL
- scripts:
+'Lint':
+ 'docker':
+ 'image': '${bamboo.dockerLint}'
+ 'key': 'LINT'
+ 'tasks':
+ - 'checkout':
+ 'force-clean-build': 'false'
+ - 'script':
+ 'interpreter': 'SHELL'
+ 'scripts':
  - |-
  set -x
  set -e
@@ -34,23 +36,23 @@ Lint:
  # Run linter
  golangci-lint run
 
- requirements:
- - adg-docker: 'true'
+ 'requirements':
+ - 'adg-docker': 'true'
 
 
-Test:
- docker:
- image: ${bamboo.dockerGo}
- volumes:
- ${system.GO_CACHE_DIR}: "${bamboo.cacheGo}"
- ${system.GO_PKG_CACHE_DIR}: "${bamboo.cacheGoPkg}"
- key: TEST
- tasks:
- - checkout:
- force-clean-build: 'false'
- - script:
- interpreter: SHELL
- scripts:
+'Test':
+ 'docker':
+ 'image': '${bamboo.dockerGo}'
+ 'volumes':
+ '${system.GO_CACHE_DIR}': '${bamboo.cacheGo}'
+ '${system.GO_PKG_CACHE_DIR}': '${bamboo.cacheGoPkg}'
+ 'key': 'TEST'
+ 'tasks':
+ - 'checkout':
+ 'force-clean-build': 'false'
+ - 'script':
+ 'interpreter': SHELL
+ 'scripts':
  - |-
  set -x
  set -e
@@ -60,22 +62,22 @@ Test:
 
  # Run tests
  go test -race -v -bench="." -coverprofile="coverage.txt" -covermode=atomic ./...
- requirements:
- - adg-docker: 'true'
+ 'requirements':
+ - 'adg-docker': 'true'
 
-branches:
- create: for-pull-request
- delete:
- after-deleted-days: '7'
- after-inactive-days: '30'
- integration:
- push-on-success: 'false'
- merge-from: urlfilter - Build and run tests
- link-to-jira: 'true'
+'branches':
+ 'create': 'for-pull-request'
+ 'delete':
+ 'after-deleted-days': '7'
+ 'after-inactive-days': '30'
+ 'integration':
+ 'push-on-success': 'false'
+ 'merge-from': 'urlfilter - Build and run tests'
+ 'link-to-jira': 'true'
 
-notifications: []
+'notifications': [ ]
 
-labels: []
+'labels': [ ]
 
-other:
- concurrent-build-plugin: system-default
+'other':
+ 'concurrent-build-plugin': 'system-default'

cmd/main.go

@@ -13,6 +13,7 @@ import (
  "github.com/AdguardTeam/golibs/log"
  "github.com/AdguardTeam/gomitmproxy"
  "github.com/AdguardTeam/gomitmproxy/mitm"
+ "github.com/AdguardTeam/urlfilter/filterutil"
  "github.com/AdguardTeam/urlfilter/proxy"
  goFlags "github.com/jessevdk/go-flags"
 )
@@ -105,7 +106,7 @@ func run(options Options) {
 }
 
 func createServerConfig(options Options) proxy.Config {
- listenIP := net.ParseIP(options.ListenAddr)
+ listenIP := filterutil.ParseIP(options.ListenAddr)
  if listenIP == nil {
  log.Fatalf("cannot parse %s", options.ListenAddr)
  }

filterutil/net.go

@@ -0,0 +1,20 @@
+package filterutil
+
+import "net"
+
+// ParseIP parses the string and checks if it's a valid IP address or not. It
+// uses net.ParseIP internally and the purpose of this wrapper is to first do a
+// quick check without additional allocations.
+func ParseIP(s string) (ip net.IP) {
+ for _, c := range s {
+ if c != '.' && c != ':' &&
+ (c < '0' || c > '9') &&
+ (c < 'A' || c > 'F') &&
+ (c < 'a' || c > 'f') &&
+ c != '[' && c != ']' {
+ return nil
+ }
+ }
+
+ return net.ParseIP(s)
+}

filterutil/net_test.go

@@ -0,0 +1,48 @@
+package filterutil
+
+import (
+ "net"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+)
+
+func BenchmarkParseIP(b *testing.B) {
+ b.Run("filterutil.ParseIP", func(b *testing.B) {
+ b.ReportAllocs()
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ ParseIP("domain.com")
+ ParseIP("invalid:ip")
+ }
+ })
+
+ b.Run("net.ParseIP", func(b *testing.B) {
+ b.ReportAllocs()
+ b.ResetTimer()
+ for i := 0; i < b.N; i++ {
+ net.ParseIP("domain.com")
+ net.ParseIP("invalid:ip")
+ }
+ })
+}
+
+func TestParseIP(t *testing.T) {
+ testCases := []struct {
+ s string
+ }{{
+ s: "127.0.0.1",
+ }, {
+ s: "random string",
+ }, {
+ s: "2001:0db8:0000:0000:0000:8a2e:0370:7334",
+ }, {
+ s: "[2001:db8::8a2e:370:7334]",
+ }}
+
+ for _, tc := range testCases {
+ t.Run(tc.s, func(t *testing.T) {
+ require.Equal(t, net.ParseIP(tc.s), ParseIP(tc.s))
+ })
+ }
+}

filterutil/util.go

@@ -53,7 +53,7 @@ func ExtractHostname(url string) string {
 // . max length of ascii hostname including dots is 253 characters
 // . TLD is >=2 characters
 // . TLD is [a-zA-Z]+ or "xn--[a-zA-Z0-9]+"
-// nolint(gocyclo)
+//nolint:gocyclo
 func IsDomainName(name string) bool {
  if len(name) > 253 {
  return false

rules/clients.go

@@ -4,6 +4,8 @@ import (
  "bytes"
  "net"
  "sort"
+
+ "github.com/AdguardTeam/urlfilter/filterutil"
 )
 
 // set representation for $client modifiers
@@ -59,7 +61,7 @@ func (c *clients) add(client string) {
  return
  }
 
- ip := net.ParseIP(client)
+ ip := filterutil.ParseIP(client)
  if ip != nil {
  mask := net.CIDRMask(32, 32)
  if ip.To4() == nil {
@@ -92,7 +94,7 @@ func (c *clients) containsAny(host, ipStr string) bool {
  return true
  }
 
- ip := net.ParseIP(ipStr)
+ ip := filterutil.ParseIP(ipStr)
  if ip != nil {
  for _, subnet := range c.nets {
  if subnet.Contains(ip) {

rules/dnsrewrite.go

@@ -2,11 +2,11 @@ package rules
 
 import (
  "fmt"
- "net"
  "strconv"
  "strings"
 
  "github.com/AdguardTeam/golibs/errors"
+ "github.com/AdguardTeam/urlfilter/filterutil"
  "github.com/miekg/dns"
 )
 
@@ -144,7 +144,7 @@ func loadDNSRewriteShort(s string) (rewrite *DNSRewrite, err error) {
  return nil, fmt.Errorf("unknown keyword: %q", s)
  }
 
- ip := net.ParseIP(s)
+ ip := filterutil.ParseIP(s)
  if ip != nil {
  if ip4 := ip.To4(); ip4 != nil {
  return &DNSRewrite{
@@ -387,7 +387,7 @@ func svcbDNSRewriteRRHandler(rcode RCode, rr RRType, valStr string) (dnsr *DNSRe
 // handlers.
 var dnsRewriteRRHandlers = map[RRType]dnsRewriteRRHandler{
  dns.TypeA: func(rcode RCode, rr RRType, valStr string) (dnsr *DNSRewrite, err error) {
- ip := net.ParseIP(valStr)
+ ip := filterutil.ParseIP(valStr)
  if ip4 := ip.To4(); ip4 == nil {
  return nil, fmt.Errorf("invalid ipv4: %q", valStr)
  }
@@ -400,7 +400,7 @@ var dnsRewriteRRHandlers = map[RRType]dnsRewriteRRHandler{
  },
 
  dns.TypeAAAA: func(rcode RCode, rr RRType, valStr string) (dnsr *DNSRewrite, err error) {
- ip := net.ParseIP(valStr)
+ ip := filterutil.ParseIP(valStr)
  if ip == nil {
  return nil, fmt.Errorf("invalid ipv6: %q", valStr)
  } else if ip4 := ip.To4(); ip4 != nil {

rules/host_rule.go

@@ -74,7 +74,7 @@ func NewHostRule(ruleText string, filterListID int) (*HostRule, error) {
  h.IP = net.IPv4(0, 0, 0, 0)
 
  } else {
- h.IP = net.ParseIP(first)
+ h.IP = filterutil.ParseIP(first)
  if h.IP == nil {
  return nil, &RuleSyntaxError{msg: "cannot parse IP", ruleText: ruleText}
  }

rules/network_rule.go

@@ -7,6 +7,7 @@ import (
  "sync"
 
  "github.com/AdguardTeam/golibs/errors"
+ "github.com/AdguardTeam/urlfilter/filterutil"
 )
 
 const (
@@ -215,7 +216,7 @@ func (f *NetworkRule) Match(r *Request) (ok bool) {
  f.IsOptionEnabled(OptionThirdParty) && !r.ThirdParty,
  f.IsOptionDisabled(OptionThirdParty) && r.ThirdParty,
  !f.matchRequestType(r.RequestType),
- !f.matchRequestDomain(r.Hostname),
+ !f.matchRequestDomain(r.Hostname, r.IsHostnameRequest),
  !f.matchSourceDomain(r.SourceHostname),
  !f.matchDNSType(r.DNSType),
  !f.matchClientTags(r.SortedClientTags),
@@ -474,25 +475,33 @@ func (f *NetworkRule) matchShortcut(r *Request) bool {
 }
 
 // matchRequestDomain checks if the filtering rule is allowed to match this
-// request domain, e.g. it checks it against the $denyallow modifier
-//
-// Please pay attention at how $denyallow works: the rule will work if the
-// request hostname **does not** belong to $denyallow domains.
-// The idea is to allow rules that block anything EXCEPT FOR some domains.
-// For instance, if we have a website that we know to load a lot of
-// third-party crap, but some of the domains are crucial for this website,
-// we may want to add something like this:
-// `*$script,domain=example.org,denyallow=essentialdomain1.com|essentialdomain2.com`
-func (f *NetworkRule) matchRequestDomain(domain string) bool {
+// request domain, e.g. it checks it against the $denyallow modifier. Please,
+// pay attention at how $denyallow works: the rule will work if the request
+// hostname **does not** belong to $denyallow domains. The idea is to allow
+// rules that block anything EXCEPT FOR some domains. For instance, if we have
+// a website that we know to load a lot of third-party crap, but some of the
+// domains are crucial for this website, we may want to add something like this:
+// "*$script,domain=example.org,denyallow=essential1.com|essential2.com".
+func (f *NetworkRule) matchRequestDomain(domain string, hostnameRequest bool) (ok bool) {
  if len(f.denyAllowDomains) == 0 {
  return true
  }
 
+ // If this is a hostname request, we're probably dealing with DNS filtering.
+ // In this case, we should avoid matching IP addresses here since they can
+ // only come from CNAME filtering. So regardless of whether it actually
+ // matches the "denyallow" list, we consider that it does not.
+ // Original issue: https://github.com/AdguardTeam/AdGuardHome/issues/3175.
+ if hostnameRequest && filterutil.ParseIP(domain) != nil {
+ return false
+ }
+
  return !isDomainOrSubdomainOfAny(domain, f.denyAllowDomains)
 }
 
-// matchSourceDomain checks if the specified filtering rule is allowed on this domain
-// e.g. it checks the domain against what's specified in the $domain modifier
+// matchSourceDomain checks if the specified filtering rule is allowed on this
+// domain e.g. it checks the domain against what's specified in the $domain
+// modifier.
 func (f *NetworkRule) matchSourceDomain(domain string) bool {
  if len(f.permittedDomains) == 0 && len(f.restrictedDomains) == 0 {
  return true