Fix RRL responses-per-second bypass using wildcard names

It is possible to bypass Response Rate Limiting (RRL)
`responses-per-second` limitation using specially crafted wildcard
names, because the current implementation, when encountering a found
DNS name generated from a wildcard record, just strips the leftmost
label of the name before making a key for the bucket.

While that technique helps with limiting random requests like
<random>.example.com (because all those requests will be accounted
as belonging to a bucket constructed from "example.com" name), it does
not help with random names like subdomain.<random>.example.com.

The best solution would have been to strip not just the leftmost
label, but as many labels as necessary until reaching the suffix part
of the wildcard record from which the found name is generated, however,
we do not have that information readily available in the context of RRL
processing code.

Fix the issue by interpreting all valid wildcard domain names as
the zone's origin name concatenated to the "*" name, so they all will
be put into the same bucket.

diff --git a/bin/tests/system/rrl/tests.sh b/bin/tests/system/rrl/tests.sh
index ff03a3176d075af3578a41459aa67c3470ea9b68..e3207ca1cde4facf43e6a9c5c13de96108c3a8ba 100644 (file)
--- a/bin/tests/system/rrl/tests.sh
+++ b/bin/tests/system/rrl/tests.sh
@@ -172,9 +172,7 @@ burst 3 a1.tld2
 sleep 1
 burst 10 a1.tld2
 # Request 30 different qnames to try a wildcard.
-burst 30 'x$CNT.a2.tld2'
-# These should be counted and limited but are not.  See RT33138.
-burst 10 'y.x$CNT.a2.tld2'
+burst 30 'y.x$CNT.a2.tld2'
 
 #                                      IP      TC      drop  NXDOMAIN SERVFAIL NOERROR
 # referrals to "."
@@ -182,12 +180,9 @@ ck_result   a1.tld3        x               0       1       2       0       0       2
 # check 13 results including 1 second delay that allows an additional response
 ck_result   a1.tld2    192.0.2.1       3       4       6       0       0       8
 
-# Check the wild card answers.
-# The parent name of the 30 requests is counted.
-ck_result 'x*.a2.tld2' 192.0.2.2       2       10      18      0       0       12
-
-# These should be limited but are not.  See RT33138.
-ck_result 'y.x*.a2.tld2' 192.0.2.2     10      0       0       0       0       10
+# Check the wildcard answers.
+# The zone origin name of the 30 requests is counted.
+ck_result 'y.x*.a2.tld2'       192.0.2.2       2       10      18      0       0       12
 
 #########
 sec_start

diff --git a/lib/dns/include/dns/rrl.h b/lib/dns/include/dns/rrl.h
index 66c29ebe2690ffcc9ad5227620e6f9c931a51485..bdffed99ac0189ae711bf7e8929350ffa8d08840 100644 (file)
--- a/lib/dns/include/dns/rrl.h
+++ b/lib/dns/include/dns/rrl.h
@@ -255,8 +255,8 @@ typedef enum {
 } dns_rrl_result_t;
 
 dns_rrl_result_t
-dns_rrl(dns_view_t *view, const isc_sockaddr_t *client_addr, bool is_tcp,
-       dns_rdataclass_t rdclass, dns_rdatatype_t qtype,
+dns_rrl(dns_view_t *view, dns_zone_t *zone, const isc_sockaddr_t *client_addr,
+       bool is_tcp, dns_rdataclass_t rdclass, dns_rdatatype_t qtype,
        const dns_name_t *qname, isc_result_t resp_result, isc_stdtime_t now,
        bool wouldlog, char *log_buf, unsigned int log_buf_len);
 

diff --git a/lib/dns/rrl.c b/lib/dns/rrl.c
index 599e76b00420ea63ed61aa654d5b9206a5b396e4..9ecc6c5dc5d33dce7c1ac4e1fe460ccb0ff84788 100644 (file)
--- a/lib/dns/rrl.c
+++ b/lib/dns/rrl.c
@@ -30,11 +30,13 @@
 #include <isc/util.h>
 
 #include <dns/log.h>
+#include <dns/name.h>
 #include <dns/rcode.h>
 #include <dns/rdataclass.h>
 #include <dns/rdatatype.h>
 #include <dns/rrl.h>
 #include <dns/view.h>
+#include <dns/zone.h>
 
 static void
 log_end(dns_rrl_t *rrl, dns_rrl_entry_t *e, bool early, char *log_buf,
@@ -413,12 +415,10 @@ hash_key(const dns_rrl_key_t *key) {
  */
 static void
 make_key(const dns_rrl_t *rrl, dns_rrl_key_t *key,
-        const isc_sockaddr_t *client_addr, dns_rdatatype_t qtype,
-        const dns_name_t *qname, dns_rdataclass_t qclass,
-        dns_rrl_rtype_t rtype) {
-       dns_name_t base;
-       dns_offsets_t base_offsets;
-       int labels, i;
+        const isc_sockaddr_t *client_addr, dns_zone_t *zone,
+        dns_rdatatype_t qtype, const dns_name_t *qname,
+        dns_rdataclass_t qclass, dns_rrl_rtype_t rtype) {
+       int i;
 
        memset(key, 0, sizeof(*key));
 
@@ -436,15 +436,30 @@ make_key(const dns_rrl_t *rrl, dns_rrl_key_t *key,
        }
 
        if (qname != NULL && qname->labels != 0) {
-               /*
-                * Ignore the first label of wildcards.
-                */
+               dns_name_t *origin = NULL;
+
                if ((qname->attributes & DNS_NAMEATTR_WILDCARD) != 0 &&
-                   (labels = dns_name_countlabels(qname)) > 1)
+                   zone != NULL && (origin = dns_zone_getorigin(zone)) != NULL)
                {
-                       dns_name_init(&base, base_offsets);
-                       dns_name_getlabelsequence(qname, 1, labels - 1, &base);
-                       key->s.qname_hash = dns_name_fullhash(&base, false);
+                       dns_fixedname_t fixed;
+                       dns_name_t *wild;
+                       isc_result_t result;
+
+                       /*
+                        * Put all wildcard names in one bucket using the zone's
+                        * origin name concatenated to the "*" name.
+                        */
+                       wild = dns_fixedname_initname(&fixed);
+                       result = dns_name_concatenate(dns_wildcardname, origin,
+                                                     wild, NULL);
+                       if (result != ISC_R_SUCCESS) {
+                               /*
+                                * Fallback to use the zone's origin name
+                                * instead of the concatenated name.
+                                */
+                               wild = origin;
+                       }
+                       key->s.qname_hash = dns_name_fullhash(wild, false);
                } else {
                        key->s.qname_hash = dns_name_fullhash(qname, false);
                }
@@ -509,7 +524,7 @@ response_balance(dns_rrl_t *rrl, const dns_rrl_entry_t *e, int age) {
  * Search for an entry for a response and optionally create it.
  */
 static dns_rrl_entry_t *
-get_entry(dns_rrl_t *rrl, const isc_sockaddr_t *client_addr,
+get_entry(dns_rrl_t *rrl, const isc_sockaddr_t *client_addr, dns_zone_t *zone,
          dns_rdataclass_t qclass, dns_rdatatype_t qtype,
          const dns_name_t *qname, dns_rrl_rtype_t rtype, isc_stdtime_t now,
          bool create, char *log_buf, unsigned int log_buf_len) {
@@ -520,7 +535,7 @@ get_entry(dns_rrl_t *rrl, const isc_sockaddr_t *client_addr,
        dns_rrl_bin_t *new_bin, *old_bin;
        int probes, age;
 
-       make_key(rrl, &key, client_addr, qtype, qname, qclass, rtype);
+       make_key(rrl, &key, client_addr, zone, qtype, qname, qclass, rtype);
        hval = hash_key(&key);
 
        /*
@@ -650,9 +665,9 @@ debit_rrl_entry(dns_rrl_t *rrl, dns_rrl_entry_t *e, double qps, double scale,
                /*
                 * The limit for clients that have used TCP is not scaled.
                 */
-               credit_e = get_entry(rrl, client_addr, 0, dns_rdatatype_none,
-                                    NULL, DNS_RRL_RTYPE_TCP, now, false,
-                                    log_buf, log_buf_len);
+               credit_e = get_entry(
+                       rrl, client_addr, NULL, 0, dns_rdatatype_none, NULL,
+                       DNS_RRL_RTYPE_TCP, now, false, log_buf, log_buf_len);
                if (credit_e != NULL) {
                        age = get_age(rrl, e, now);
                        if (age < rrl->window) {
@@ -1027,10 +1042,10 @@ log_stops(dns_rrl_t *rrl, isc_stdtime_t now, int limit, char *log_buf,
  * Main rate limit interface.
  */
 dns_rrl_result_t
-dns_rrl(dns_view_t *view, const isc_sockaddr_t *client_addr, bool is_tcp,
-       dns_rdataclass_t qclass, dns_rdatatype_t qtype, const dns_name_t *qname,
-       isc_result_t resp_result, isc_stdtime_t now, bool wouldlog,
-       char *log_buf, unsigned int log_buf_len) {
+dns_rrl(dns_view_t *view, dns_zone_t *zone, const isc_sockaddr_t *client_addr,
+       bool is_tcp, dns_rdataclass_t qclass, dns_rdatatype_t qtype,
+       const dns_name_t *qname, isc_result_t resp_result, isc_stdtime_t now,
+       bool wouldlog, char *log_buf, unsigned int log_buf_len) {
        dns_rrl_t *rrl;
        dns_rrl_rtype_t rtype;
        dns_rrl_entry_t *e;
@@ -1103,9 +1118,10 @@ dns_rrl(dns_view_t *view, const isc_sockaddr_t *client_addr, bool is_tcp,
         */
        if (is_tcp) {
                if (scale < 1.0) {
-                       e = get_entry(rrl, client_addr, 0, dns_rdatatype_none,
-                                     NULL, DNS_RRL_RTYPE_TCP, now, true,
-                                     log_buf, log_buf_len);
+                       e = get_entry(rrl, client_addr, NULL, 0,
+                                     dns_rdatatype_none, NULL,
+                                     DNS_RRL_RTYPE_TCP, now, true, log_buf,
+                                     log_buf_len);
                        if (e != NULL) {
                                e->responses = -(rrl->window + 1);
                                set_age(rrl, e, now);
@@ -1136,8 +1152,8 @@ dns_rrl(dns_view_t *view, const isc_sockaddr_t *client_addr, bool is_tcp,
                rtype = DNS_RRL_RTYPE_ERROR;
                break;
        }
-       e = get_entry(rrl, client_addr, qclass, qtype, qname, rtype, now, true,
-                     log_buf, log_buf_len);
+       e = get_entry(rrl, client_addr, zone, qclass, qtype, qname, rtype, now,
+                     true, log_buf, log_buf_len);
        if (e == NULL) {
                UNLOCK(&rrl->lock);
                return (DNS_RRL_RESULT_OK);
@@ -1171,8 +1187,8 @@ dns_rrl(dns_view_t *view, const isc_sockaddr_t *client_addr, bool is_tcp,
                dns_rrl_entry_t *e_all;
                dns_rrl_result_t rrl_all_result;
 
-               e_all = get_entry(rrl, client_addr, 0, dns_rdatatype_none, NULL,
-                                 DNS_RRL_RTYPE_ALL, now, true, log_buf,
+               e_all = get_entry(rrl, client_addr, zone, 0, dns_rdatatype_none,
+                                 NULL, DNS_RRL_RTYPE_ALL, now, true, log_buf,
                                  log_buf_len);
                if (e_all == NULL) {
                        UNLOCK(&rrl->lock);

diff --git a/lib/ns/client.c b/lib/ns/client.c
index 6476edd59df5dca081fe773295c0febed07b0124..6c491afa5966236aac97dbd8cf13233d8671240e 100644 (file)
--- a/lib/ns/client.c
+++ b/lib/ns/client.c
@@ -836,10 +836,11 @@ ns_client_error(ns_client_t *client, isc_result_t result) {
                        loglevel = ISC_LOG_DEBUG(1);
                }
                wouldlog = isc_log_wouldlog(ns_lctx, loglevel);
-               rrl_result = dns_rrl(
-                       client->view, &client->peeraddr, TCP_CLIENT(client),
-                       dns_rdataclass_in, dns_rdatatype_none, NULL, result,
-                       client->now, wouldlog, log_buf, sizeof(log_buf));
+               rrl_result = dns_rrl(client->view, NULL, &client->peeraddr,
+                                    TCP_CLIENT(client), dns_rdataclass_in,
+                                    dns_rdatatype_none, NULL, result,
+                                    client->now, wouldlog, log_buf,
+                                    sizeof(log_buf));
                if (rrl_result != DNS_RRL_RESULT_OK) {
                        /*
                         * Log dropped errors in the query category

diff --git a/lib/ns/query.c b/lib/ns/query.c
index 93d2d6c353d56da18dcc4f9821d7effa6cb0905c..beeae0e653e7eed50fcf43aed07d814fabec53b8 100644 (file)
--- a/lib/ns/query.c
+++ b/lib/ns/query.c
@@ -7028,10 +7028,10 @@ query_checkrrl(query_ctx_t *qctx, isc_result_t result) {
                }
 
                rrl_result = dns_rrl(
-                       qctx->view, &qctx->client->peeraddr, TCP(qctx->client),
-                       qctx->client->message->rdclass, qctx->qtype, constname,
-                       resp_result, qctx->client->now, wouldlog, log_buf,
-                       sizeof(log_buf));
+                       qctx->view, qctx->zone, &qctx->client->peeraddr,
+                       TCP(qctx->client), qctx->client->message->rdclass,
+                       qctx->qtype, constname, resp_result, qctx->client->now,
+                       wouldlog, log_buf, sizeof(log_buf));
                if (rrl_result != DNS_RRL_RESULT_OK) {
                        /*
                         * Log dropped or slipped responses in the query