From: Aram Sargsyan Date: Mon, 25 Jul 2022 13:55:03 +0000 (+0000) Subject: Fix RRL responses-per-second bypass using wildcard names X-Git-Tag: v9.19.5~9^2~2 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=baa9698c9d4bed741cdff14a07f1c71c81b21908;p=thirdparty%2Fbind9.git 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 .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..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 ff03a3176d0..e3207ca1cde 100644 --- 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 66c29ebe269..bdffed99ac0 100644 --- 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 599e76b0042..9ecc6c5dc5d 100644 --- a/lib/dns/rrl.c +++ b/lib/dns/rrl.c @@ -30,11 +30,13 @@ #include #include +#include #include #include #include #include #include +#include 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 6476edd59df..6c491afa596 100644 --- 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 93d2d6c353d..beeae0e653e 100644 --- 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