From bf645e465f8e2fcc608e168ba9d4a51e1c2e47c5 Mon Sep 17 00:00:00 2001 From: Artem Murashkin Date: Tue, 11 Aug 2026 12:30:01 +0200 Subject: [PATCH] fix: query DNSBLs with the reversed address form (DEF-52371) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A blocklist lists an address written backwards in front of its zone: 1.2.3.4 in example.com is published as 4.3.2.1.example.com (RFC 5782 2.1, and reversed hex nibbles for IPv6 per 2.4). The operator built the name from the address as-is, so every lookup asked for a name no zone publishes and came back NXDOMAIN — @rbl matched no address against any real blocklist, and IPv6 operands produced a name that is not legal DNS at all. A listed address whose TXT lookup failed was also reported as unlisted, which keeps the operator inert against the many zones that publish an address record with no TXT. The address record alone settles the verdict; TXT only carries the reason string. Both defects failed open silently, so an @rbl rule looked like enforcement while never firing. The test fixtures encoded the first one: they used unreversed zone names, one of them palindromic. They now use the reversed form, and a case whose reversed and unreversed names differ fails if the name is ever built the wrong way round. Making the lookups reach real zones puts weight on what comes back, so the answer now has to be a listing code. A blocklist answers in 127.0.0.0/8, reserving the top of that block to complain about the query itself — a public resolver, an exhausted quota — and a resolver that invents addresses for names it cannot resolve answers outside the block altogether. Reading either as a listing would deny every request. Two smaller gaps in the same operator: a rule whose reason lookup finds nothing no longer leaves the previous match's message in httpbl_msg, and @rbl without a service hostname is now a configuration error instead of a rule that loads and can never match. The lookups stay on net's own resolver because the DNSBL libraries take neither a context nor a custom resolver — the first holds the deadline that keeps a slow blocklist from pinning goroutines, the second lets the tests answer a query. --- internal/operators/rbl.go | 98 +++++++++++++++++++++----- internal/operators/rbl_test.go | 121 +++++++++++++++++++++++++++------ 2 files changed, 182 insertions(+), 37 deletions(-) diff --git a/internal/operators/rbl.go b/internal/operators/rbl.go index 0401f8c7e..6d538f6fe 100644 --- a/internal/operators/rbl.go +++ b/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 diff --git a/internal/operators/rbl_test.go b/internal/operators/rbl_test.go index a8066f733..749e621c9 100644 --- a/internal/operators/rbl_test.go +++ b/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.. 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