Merge commit from fork

TL;DR: Apparently you can set more than one PTR record per IP address,
and Anubis was wired to accept _any_ address that matches and didn't
do forward confirming reverse DNS to ensure that the matching address
made sense. This meant that attackers with the ability to set multiple
PTR records could just add whatever DNS records they wanted and bypass
Anubis all they want.

The good news is that this is a much lower severity issue than you'd
think, this requires attackers to own IP space. Either way, it's a
bug.

Signed-off-by: Xe Iaso <me@xeiaso.net>

docs/docs/CHANGELOG.md

@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 <!-- This changes the project to: -->
 
+- Fix [GHSA-5gwm-367w-7fgj](https://github.com/TecharoHQ/anubis/security/advisories/GHSA-5gwm-367w-7fgj), which allowed attackers that control the PTR records for their IP addresses to bypass Anubis via setting multiple PTR records pointing to various search engines.
 - Replace Thoth with [GeoIP databases](./admin/geoip.mdx). GeoIP databases are configured in the `geoip` block.
 - Thoth functionality has been removed. `THOTH_URL`, `THOTH_TOKEN`, and `THOTH_INSECURE` are now deprecated environment variables that will log a warning upon startup.
 - ASN description values now use the Maxmind values instead of the values that Thoth returned. This may impact your fail2ban rules.

internal/dns/dns.go

@@ -18,12 +18,14 @@ var (
 )
 
 type Dns struct {
+	lg    *slog.Logger
 	cache *DnsCache
 	ctx   context.Context
 }
 
-func New(ctx context.Context, cache *DnsCache) *Dns {
+func New(ctx context.Context, cache *DnsCache, lg *slog.Logger) *Dns {
 	return &Dns{
+		lg:    lg,
 		cache: cache,
 		ctx:   ctx,
 	}
@@ -31,7 +33,8 @@ func New(ctx context.Context, cache *DnsCache) *Dns {
 
 // ReverseDNS performs a reverse DNS lookup for the given IP address and trims the trailing dot from the results.
 func (d *Dns) ReverseDNS(addr string) ([]string, error) {
-	slog.Debug("DNS: performing reverse lookup", "addr", addr)
+	lg := d.lg.With()
+	lg.Debug("DNS: performing reverse lookup", "addr", addr)
 
 	if cached, ok := d.getCachedReverse(addr); ok {
 		return cached, nil
@@ -40,14 +43,14 @@ func (d *Dns) ReverseDNS(addr string) ([]string, error) {
 	names, err := DNSLookupAddr(addr)
 	if err != nil {
 		if dnsErr, ok := err.(*net.DNSError); ok && dnsErr.IsNotFound {
-			slog.Debug("DNS: no PTR record found", "addr", addr)
+			lg.Debug("DNS: no PTR record found", "addr", addr)
 			return []string{}, nil
 		}
-		slog.Error("DNS: reverse lookup failed", "addr", addr, "err", err)
+		lg.Error("DNS: reverse lookup failed", "addr", addr, "err", err)
 		return nil, err
 	}
 
-	slog.Debug("DNS: reverse lookup successful", "addr", addr, "names", names)
+	lg.Debug("DNS: reverse lookup successful", "addr", addr, "names", names)
 
 	trimmedNames := make([]string, len(names))
 	for i, name := range names {
@@ -60,7 +63,8 @@ func (d *Dns) ReverseDNS(addr string) ([]string, error) {
 
 // LookupHost performs a forward DNS lookup for the given hostname.
 func (d *Dns) LookupHost(host string) ([]string, error) {
-	slog.Debug("DNS: performing forward lookup", "host", host)
+	lg := d.lg.With()
+	lg.Debug("DNS: performing forward lookup", "host", host)
 
 	if cached, ok := d.getCachedForward(host); ok {
 		return cached, nil
@@ -69,14 +73,14 @@ func (d *Dns) LookupHost(host string) ([]string, error) {
 	addrs, err := DNSLookupHost(host)
 	if err != nil {
 		if dnsErr, ok := err.(*net.DNSError); ok && dnsErr.IsNotFound {
-			slog.Debug("DNS: no A/AAAA record found", "host", host)
+			lg.Debug("DNS: no A/AAAA record found", "host", host)
 			return []string{}, nil
 		}
-		slog.Error("DNS: forward lookup failed", "host", host, "err", err)
+		lg.Error("DNS: forward lookup failed", "host", host, "err", err)
 		return nil, err
 	}
 
-	slog.Debug("DNS: forward lookup successful", "host", host, "addrs", addrs)
+	lg.Debug("DNS: forward lookup successful", "host", host, "addrs", addrs)
 	d.forwardCachePut(host, addrs)
 	return addrs, nil
 }
@@ -84,28 +88,30 @@ func (d *Dns) LookupHost(host string) ([]string, error) {
 // verifyFCrDNSInternal performs the second half of the FCrDNS check, using a
 // pre-fetched list of names to perform the forward lookups.
 func (d *Dns) verifyFCrDNSInternal(addr string, names []string) bool {
+	lg := d.lg.With()
 	for _, name := range names {
 		if cached, err := d.LookupHost(name); err == nil {
 			if slices.Contains(cached, addr) {
-				slog.Info("DNS: forward lookup confirmed original IP", "name", name, "addr", addr)
+				lg.Info("DNS: forward lookup confirmed original IP", "name", name, "addr", addr)
 				return true
 			}
 			continue
 		}
 	}
 
-	slog.Info("DNS: could not confirm original IP in forward lookups", "addr", addr)
+	lg.Info("DNS: could not confirm original IP in forward lookups", "addr", addr)
 	return false
 }
 
 // VerifyFCrDNS performs a forward-confirmed reverse DNS (FCrDNS) lookup for the given IP address,
 // optionally matching against a provided pattern.
 func (d *Dns) VerifyFCrDNS(addr string, pattern *string) bool {
+	lg := d.lg.With()
 	var patternVal string
 	if pattern != nil {
 		patternVal = *pattern
 	}
-	slog.Debug("DNS: performing FCrDNS lookup", "addr", addr, "pattern", patternVal)
+	lg.Debug("DNS: performing FCrDNS lookup", "addr", addr, "pattern", patternVal)
 
 	names, err := d.ReverseDNS(addr)
 	if err != nil {
@@ -117,24 +123,24 @@ func (d *Dns) VerifyFCrDNS(addr string, pattern *string) bool {
 
 	// If a pattern is provided, check for a match.
 	if pattern != nil {
-		anyNameMatched := false
+		re, err := regexp.Compile(*pattern)
+		if err != nil {
+			lg.Error("DNS: verifyFCrDNS invalid regex pattern", "err", err)
+			return false
+		}
+
+		var matchedNames []string
 		for _, name := range names {
-			matched, err := regexp.MatchString(*pattern, name)
-			if err != nil {
-				slog.Error("DNS: verifyFCrDNS invalid regex pattern", "err", err)
-				return false // Invalid pattern is a failure.
-			}
-			if matched {
-				anyNameMatched = true
-				break
+			if re.MatchString(name) {
+				matchedNames = append(matchedNames, name)
 			}
 		}
 
-		if !anyNameMatched {
-			slog.Debug("DNS: FCrDNS no PTR matches the pattern", "addr", addr, "pattern", *pattern)
+		if len(matchedNames) == 0 {
 			return false
 		}
-		slog.Debug("DNS: FCrDNS PTR matched pattern, proceeding with forward check", "addr", addr, "pattern", *pattern)
+
+		names = matchedNames
 	}
 
 	// If we're here, either there was no pattern, or the pattern matched.

internal/dns/dns_test.go

@@ -1,7 +1,6 @@
 package dns
 
 import (
-	"context"
 	"errors"
 	"net"
 	"reflect"
@@ -9,14 +8,15 @@ import (
 	"time"
 
 	"github.com/TecharoHQ/anubis/lib/store/memory"
+	"github.com/neilotoole/slogt/v2"
 )
 
 // newTestDNS is a helper function to create a new Dns object with an in-memory cache for testing.
-func newTestDNS(forwardTTL int, reverseTTL int) *Dns {
-	ctx := context.Background()
-	memStore := memory.New(ctx)
+func newTestDNS(t *testing.T, forwardTTL int, reverseTTL int) *Dns {
+	t.Helper()
+	memStore := memory.New(t.Context())
 	cache := NewDNSCache(forwardTTL, reverseTTL, memStore)
-	return New(ctx, cache)
+	return New(t.Context(), cache, slogt.New(t))
 }
 
 // mockLookupAddr is a mock implementation of the net.LookupAddr function.
@@ -75,7 +75,7 @@ func TestMain(m *testing.M) {
 }
 
 func TestDns_ArpaReverseIP(t *testing.T) {
-	d := newTestDNS(0, 0)
+	d := newTestDNS(t, 0, 0)
 	tests := []struct {
 		name    string
 		ip      string
@@ -102,7 +102,7 @@ func TestDns_ArpaReverseIP(t *testing.T) {
 }
 
 func TestDns_ReverseDNS(t *testing.T) {
-	d := newTestDNS(1, 1) // short TTL for testing cache
+	d := newTestDNS(t, 1, 1) // short TTL for testing cache
 
 	// First call - cache miss
 	t.Run("cache miss", func(t *testing.T) {
@@ -173,7 +173,7 @@ func TestDns_ReverseDNS(t *testing.T) {
 }
 
 func TestDns_LookupHost(t *testing.T) {
-	d := newTestDNS(1, 1)
+	d := newTestDNS(t, 1, 1)
 
 	t.Run("cache miss", func(t *testing.T) {
 		got, err := d.LookupHost("dns.google")
@@ -236,7 +236,7 @@ func TestDns_LookupHost(t *testing.T) {
 }
 
 func TestDns_VerifyFCrDNS(t *testing.T) {
-	d := newTestDNS(1, 1)
+	d := newTestDNS(t, 1, 1)
 
 	// Helper to convert string to *string
 	p := func(s string) *string {

internal/dns/ghsa_5gwm_367w_7fgj_test.go

@@ -0,0 +1,82 @@
+package dns
+
+import (
+	"context"
+	"net"
+	"testing"
+
+	"github.com/neilotoole/slogt/v2"
+)
+
+// TestGHSA_5gwm_367w_7fgj requires the same PTR hostname to match the
+// policy pattern and forward-resolve to the original IP address.
+func TestGHSA_5gwm_367w_7fgj(t *testing.T) {
+	const addr = "192.0.2.42"
+	pattern := `^.*\.yandex\.(ru|com|net)$`
+
+	tests := []struct {
+		name    string
+		names   []string
+		forward map[string][]string
+		want    bool
+	}{
+		{
+			name:    "matching PTR does not resolve",
+			names:   []string{"fake.yandex.com.", "owned.example."},
+			forward: map[string][]string{"owned.example": {addr}},
+		},
+		{
+			name:    "nonmatching PTR comes first",
+			names:   []string{"owned.example.", "fake.yandex.com."},
+			forward: map[string][]string{"owned.example": {addr}},
+		},
+		{
+			name:  "matching PTR resolves to another IP",
+			names: []string{"fake.yandex.com.", "owned.example."},
+			forward: map[string][]string{
+				"fake.yandex.com": {"192.0.2.99"},
+				"owned.example":   {addr},
+			},
+		},
+		{
+			name:    "forward confirmed hostname does not match",
+			names:   []string{"owned.example."},
+			forward: map[string][]string{"owned.example": {addr}},
+		},
+		{
+			name:    "matching hostname is forward confirmed",
+			names:   []string{"real.yandex.com."},
+			forward: map[string][]string{"real.yandex.com": {addr}},
+			want:    true,
+		},
+		{
+			name:    "later matching hostname is forward confirmed",
+			names:   []string{"fake.yandex.com.", "real.yandex.com."},
+			forward: map[string][]string{"real.yandex.com": {addr}},
+			want:    true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			originalAddr, originalHost := DNSLookupAddr, DNSLookupHost
+			t.Cleanup(func() {
+				DNSLookupAddr, DNSLookupHost = originalAddr, originalHost
+			})
+			DNSLookupAddr = func(string) ([]string, error) {
+				return tt.names, nil
+			}
+			DNSLookupHost = func(host string) ([]string, error) {
+				if addrs, ok := tt.forward[host]; ok {
+					return addrs, nil
+				}
+				return nil, &net.DNSError{Name: host, IsNotFound: true}
+			}
+
+			d := New(context.Background(), nil, slogt.New(t))
+			if got := d.VerifyFCrDNS(addr, &pattern); got != tt.want {
+				t.Errorf("VerifyFCrDNS(%q, %q) with PTRs %v = %v, want %v", addr, pattern, tt.names, got, tt.want)
+			}
+		})
+	}
+}

lib/policy/celchecker_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/TecharoHQ/anubis/internal/dns"
 	"github.com/TecharoHQ/anubis/lib/config"
 	"github.com/TecharoHQ/anubis/lib/store/memory"
+	"github.com/neilotoole/slogt/v2"
 )
 
 func newTestDNS(t *testing.T) *dns.Dns {
@@ -15,7 +16,7 @@ func newTestDNS(t *testing.T) *dns.Dns {
 	ctx := t.Context()
 	memStore := memory.New(ctx)
 	cache := dns.NewDNSCache(300, 300, memStore)
-	return dns.New(ctx, cache)
+	return dns.New(ctx, cache, slogt.New(t))
 }
 
 func TestCELChecker_MapIterationWrappers(t *testing.T) {

lib/policy/expressions/environment_test.go

@@ -12,18 +12,19 @@ import (
 	"cel.dev/cel-go/common/types/ref"
 	"github.com/TecharoHQ/anubis/internal/dns"
 	"github.com/TecharoHQ/anubis/lib/store/memory"
+	"github.com/neilotoole/slogt/v2"
 )
 
 // newTestDNS is a helper function to create a new Dns object with an in-memory cache for testing.
-func newTestDNS(forwardTTL int, reverseTTL int) *dns.Dns {
+func newTestDNS(t *testing.T, forwardTTL int, reverseTTL int) *dns.Dns {
 	ctx := context.Background()
 	memStore := memory.New(ctx)
 	cache := dns.NewDNSCache(forwardTTL, reverseTTL, memStore)
-	return dns.New(ctx, cache)
+	return dns.New(ctx, cache, slogt.New(t))
 }
 
 func TestBotEnvironment(t *testing.T) {
-	dnsObj := newTestDNS(300, 300)
+	dnsObj := newTestDNS(t, 300, 300)
 	env, err := BotEnvironment(dnsObj)
 	if err != nil {
 		t.Fatalf("failed to create bot environment: %v", err)

lib/policy/expressions/registry_test.go

@@ -34,7 +34,7 @@ func TestRegisterBotVariable(t *testing.T) {
 		return result, true
 	})
 
-	env, err := BotEnvironment(newTestDNS(300, 300))
+	env, err := BotEnvironment(newTestDNS(t, 300, 300))
 	if err != nil {
 		t.Fatalf("can't create bot environment: %v", err)
 	}

lib/policy/policy.go

@@ -145,7 +145,7 @@ func ParseConfig(ctx context.Context, fin io.Reader, fname string, defaultDiffic
 	}
 
 	result.DnsCache = dns.NewDNSCache(result.orig.DNSTTL.Forward, result.orig.DNSTTL.Reverse, result.Store)
-	result.Dns = dns.New(ctx, result.DnsCache)
+	result.Dns = dns.New(ctx, result.DnsCache, lg.With("component", "dns"))
 
 	for _, b := range c.Bots {
 		if berr := b.Valid(); berr != nil {