fix: query DNSBLs with the reversed address form (DEF-52371)

internal/operators/rbl.go

@@ -7,9 +7,11 @@ package operators
 
 import (
 	"context"
+	"encoding/hex"
 	"errors"
-	"fmt"
 	"net"
+	"strconv"
+	"strings"
 	"time"
 
 	"github.com/corazawaf/coraza/v3/experimental/plugins/plugintypes"
@@ -45,6 +47,11 @@ var _ plugintypes.Operator = (*rbl)(nil)
 
 func newRBL(options plugintypes.OperatorOptions) (plugintypes.Operator, error) {
 	data := options.Arguments
+	if data == "" {
+		// Without a zone every lookup goes to the DNS root and no address can
+		// ever be listed, so the rule would load as enforcement that is inert.
+		return nil, errors.New("missing RBL service hostname")
+	}
 
 	return &rbl{
 		service:  data,
@@ -55,7 +62,8 @@ func newRBL(options plugintypes.OperatorOptions) (plugintypes.Operator, error) {
 // https://github.com/mrichman/godnsbl
 // https://github.com/SpiderLabs/ModSecurity/blob/b66224853b4e9d30e0a44d16b29d5ed3842a6b11/src/operators/rbl.cc
 func (o *rbl) Evaluate(tx plugintypes.TransactionState, ipAddr string) bool {
-	if net.ParseIP(ipAddr) == nil {
+	ip := net.ParseIP(ipAddr)
+	if ip == nil {
 		// The operand is unvalidated request data, so it stays out of the message.
 		tx.DebugLogger().Warn().Msg("RBL lookup skipped: operand is not an IP address")
 		return false
@@ -66,31 +74,89 @@ func (o *rbl) Evaluate(tx plugintypes.TransactionState, ipAddr string) bool {
 	ctx, cancel := context.WithTimeout(context.Background(), timeout)
 	defer cancel()
 
-	addr := fmt.Sprintf("%s.%s", ipAddr, o.service)
+	addr := rblQueryName(ip, o.service)
+	// LookupIP and LookupNetIP would undo that: they go through the shared
+	// lookup group in net, where concurrent lookups of one name join a single
+	// call that stops being cancellable as soon as it has a second waiter and
+	// then outlives all of them, so a slow blocklist would pin a goroutine per
+	// request.
 	res, err := o.resolver.LookupHost(ctx, addr)
 	if err != nil {
 		logRBLLookupError(tx, addr, err)
 		return false
 	}
+	if !anyListingCode(res) {
+		tx.DebugLogger().Warn().
+			Str("address", addr).
+			Str("answers", strings.Join(res, " ")).
+			Msg("RBL answer is not a listing code, treating IP as not listed")
+		return false
+	}
 
-	var status string
-	if len(res) > 0 {
-		txt, err := o.resolver.LookupTXT(ctx, addr)
-		if err != nil {
-			logRBLLookupError(tx, addr, err)
-			return false
-		}
-		if len(txt) > 0 {
-			status = txt[0]
-		}
+	// The address record alone decides that the IP is listed. The TXT record
+	// only carries the human-readable reason, so a zone that publishes none —
+	// or a TXT lookup that fails on its own — must not undo the verdict.
+	var reason string
+	if txt, err := o.resolver.LookupTXT(ctx, addr); err != nil {
+		// The address lookup that just succeeded proves the resolver answers,
+		// so a failure here is almost always a zone with no TXT record, and it
+		// costs nothing: the verdict is already settled.
+		tx.DebugLogger().Debug().Err(err).Str("address", addr).Msg("RBL reason lookup failed")
+	} else if len(txt) > 0 {
+		reason = txt[0]
 	}
-	if status != "" {
-		tx.Variables().TX().Set("httpbl_msg", []string{status})
-		tx.CaptureField(0, status)
+	// Whatever the reason turns out to be, it has to describe this match: a
+	// zone that publishes no TXT record must not leave an earlier RBL rule's
+	// message standing as if it belonged here.
+	if reason == "" {
+		tx.Variables().TX().Remove("httpbl_msg")
+	} else {
+		tx.Variables().TX().Set("httpbl_msg", []string{reason})
 	}
+	tx.CaptureField(0, reason)
 	return true
 }
 
+// anyListingCode reports whether an answer means the address asked about is
+// listed. RFC 5782 §2.1 puts a listing in 127.0.0.0/8, and zones answer in the
+// top of that block, 127.255.255.0/24, to complain about the query itself — a
+// public resolver, an exhausted quota — which says nothing about the address.
+// A resolver that invents an address for every name it cannot resolve answers
+// outside 127.0.0.0/8 altogether. Reading either as a listing would block
+// every request.
+func anyListingCode(answers []string) bool {
+	for _, answer := range answers {
+		v4 := net.ParseIP(answer).To4()
+		if v4 != nil && v4[0] == 127 && (v4[1] != 255 || v4[2] != 255) {
+			return true
+		}
+	}
+	return false
+}
+
+// rblQueryName builds the name to look up for ip in the blocklist zone, the
+// address written backwards in front of the zone: IPv4 as reversed decimal
+// octets (1.2.3.4 in example.com becomes 4.3.2.1.example.com) and IPv6 as
+// reversed hex nibbles, per RFC 5782 §2.1 and §2.4. An IPv4-mapped IPv6
+// address takes the IPv4 form, which is what its zone entry uses.
+func rblQueryName(ip net.IP, zone string) string {
+	var name strings.Builder
+	if v4 := ip.To4(); v4 != nil {
+		for i := len(v4) - 1; i >= 0; i-- {
+			name.WriteString(strconv.Itoa(int(v4[i])))
+			name.WriteByte('.')
+		}
+	} else {
+		nibbles := hex.EncodeToString(ip.To16())
+		for i := len(nibbles) - 1; i >= 0; i-- {
+			name.WriteByte(nibbles[i])
+			name.WriteByte('.')
+		}
+	}
+	name.WriteString(zone)
+	return name.String()
+}
+
 // logRBLLookupError reports a failed lookup at the level its cause deserves. A
 // missing record is the ordinary negative answer, a timeout is the service
 // being slow, and a cancelled lookup is the caller giving up, so none of them

internal/operators/rbl_test.go

@@ -40,17 +40,35 @@ func TestRbl(t *testing.T) {
 
 	logger := &testLogger{t}
 
+	// Zone entries carry the reversed names a blocklist actually publishes:
+	// 1.2.3.4 is listed as 4.3.2.1.<zone>. The names differ from the plain
+	// addresses, so a lookup built the wrong way round misses them.
 	srv, err := mockdns.NewServerWithLogger(map[string]mockdns.Zone{
-		"1.1.1.1.xbl.spamhaus.org.": {
-			A: []string{"1.2.3.4"},
+		"4.3.2.1.xbl.spamhaus.org.": {
+			A:   []string{"127.0.0.2"},
+			TXT: []string{"blocked"},
 		},
-		"1.1.1.2.xbl.spamhaus.org.": {
-			A:   []string{"1.2.3.5"},
-			TXT: []string{"not blocked"},
+		"5.3.2.1.xbl.spamhaus.org.": {
+			A: []string{"127.0.0.2"},
 		},
-		"1.1.1.3.xbl.spamhaus.org.": {
-			A:   []string{"1.2.3.6"},
-			TXT: []string{"blocked"},
+		"1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.xbl.spamhaus.org.": {
+			A:   []string{"127.0.0.2"},
+			TXT: []string{"blocked v6"},
+		},
+		// Only the unreversed name of 1.2.3.6 exists, so the operator must
+		// find nothing for it.
+		"1.2.3.6.xbl.spamhaus.org.": {
+			A:   []string{"127.0.0.2"},
+			TXT: []string{"unreversed"},
+		},
+		// 3.4.5.6 gets the answer a resolver that invents addresses for names
+		// it cannot resolve would give, and 4.4.5.6 the code a zone answers
+		// with when it objects to the query rather than the address.
+		"6.5.4.3.xbl.spamhaus.org.": {
+			A: []string{"203.0.113.9"},
+		},
+		"6.5.4.4.xbl.spamhaus.org.": {
+			A: []string{"127.255.255.254"},
 		},
 	}, logger, false)
 	if err != nil {
@@ -61,20 +79,33 @@ func TestRbl(t *testing.T) {
 	srv.PatchNet(op.(*rbl).resolver)
 	defer mockdns.UnpatchNet(op.(*rbl).resolver)
 
-	t.Run("IP with an A record but no TXT record is not reported listed", func(t *testing.T) {
+	t.Run("Listed IP with TXT record", func(t *testing.T) {
 		tx := corazawaf.NewWAF().NewTransaction()
-		if op.Evaluate(tx, "1.1.1.1") {
-			t.Errorf("an A record without a TXT record must not report the IP as listed")
+		if !op.Evaluate(tx, "1.2.3.4") {
+			t.Fatal("Unexpected result for listed IP")
+		}
+		if want, have := "blocked", tx.Variables().TX().Get("httpbl_msg")[0]; want != have {
+			t.Errorf("Unexpected result for listed IP: want %q, have %q", want, have)
 		}
 	})
 
-	t.Run("Listed IP with TXT record", func(t *testing.T) {
+	t.Run("Listed IP without TXT record", func(t *testing.T) {
 		tx := corazawaf.NewWAF().NewTransaction()
-		if !op.Evaluate(tx, "1.1.1.2") {
-			t.Errorf("Unexpected result for listed IP")
+		if !op.Evaluate(tx, "1.2.3.5") {
+			t.Error("a listed IP whose zone publishes no TXT record must still be reported listed")
 		}
-		if want, have := "not blocked", tx.Variables().TX().Get("httpbl_msg")[0]; want != have {
-			t.Errorf("Unexpected result for listed IP: want %q, have %q", want, have)
+		if got := tx.Variables().TX().Get("httpbl_msg"); len(got) > 0 {
+			t.Errorf("httpbl_msg set without a TXT record: %q", got)
+		}
+	})
+
+	t.Run("Listed IPv6", func(t *testing.T) {
+		tx := corazawaf.NewWAF().NewTransaction()
+		if !op.Evaluate(tx, "2001:db8::1") {
+			t.Fatal("Unexpected result for listed IPv6 address")
+		}
+		if want, have := "blocked v6", tx.Variables().TX().Get("httpbl_msg")[0]; want != have {
+			t.Errorf("Unexpected result for listed IPv6 address: want %q, have %q", want, have)
 		}
 	})
 
@@ -88,15 +119,63 @@ func TestRbl(t *testing.T) {
 		}
 	})
 
-	t.Run("Blocked IP", func(t *testing.T) {
+	t.Run("IP listed only under its unreversed name", func(t *testing.T) {
 		tx := corazawaf.NewWAF().NewTransaction()
-		if !op.Evaluate(tx, "1.1.1.3") {
-			t.Fatal("Unexpected result for blocked IP")
+		if op.Evaluate(tx, "1.2.3.6") {
+			t.Error("the operator queried the unreversed name")
 		}
-		if want, have := "blocked", tx.Variables().TX().Get("httpbl_msg")[0]; want != have {
-			t.Errorf("Unexpected result for blocked IP: want %q, have %q", want, have)
+	})
+
+	t.Run("Answer outside the listing range", func(t *testing.T) {
+		tx := corazawaf.NewWAF().NewTransaction()
+		if op.Evaluate(tx, "3.4.5.6") {
+			t.Error("an answer outside 127.0.0.0/8 must not be read as a listing")
 		}
 	})
+
+	t.Run("Answer complaining about the query", func(t *testing.T) {
+		tx := corazawaf.NewWAF().NewTransaction()
+		if op.Evaluate(tx, "4.4.5.6") {
+			t.Error("an answer in 127.255.255.0/24 must not be read as a listing")
+		}
+	})
+
+	t.Run("Reason of an earlier match does not survive", func(t *testing.T) {
+		tx := corazawaf.NewWAF().NewTransaction()
+		if !op.Evaluate(tx, "1.2.3.4") {
+			t.Fatal("Unexpected result for listed IP")
+		}
+		if !op.Evaluate(tx, "1.2.3.5") {
+			t.Fatal("Unexpected result for listed IP without a TXT record")
+		}
+		if got := tx.Variables().TX().Get("httpbl_msg"); len(got) > 0 {
+			t.Errorf("httpbl_msg still holds the earlier match's reason: %q", got)
+		}
+	})
+}
+
+func TestRblRequiresAService(t *testing.T) {
+	if _, err := newRBL(plugintypes.OperatorOptions{}); err == nil {
+		t.Error("an @rbl rule with no service hostname must not load")
+	}
+}
+
+func TestRblQueryName(t *testing.T) {
+	tests := []struct{ ip, want string }{
+		{"1.2.3.4", "4.3.2.1.example.com"},
+		{"127.0.0.2", "2.0.0.127.example.com"},
+		{"2001:db8::1", "1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.0.8.b.d.0.1.0.0.2.example.com"},
+		{"::ffff:1.2.3.4", "4.3.2.1.example.com"},
+	}
+	for _, tc := range tests {
+		ip := net.ParseIP(tc.ip)
+		if ip == nil {
+			t.Fatalf("test fixture %q is not an IP address", tc.ip)
+		}
+		if have := rblQueryName(ip, "example.com"); have != tc.want {
+			t.Errorf("rblQueryName(%q): want %q, have %q", tc.ip, tc.want, have)
+		}
+	}
 }
 
 // errorCapturingWAF returns a WAF whose debug logger records, at Error level