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