From baa9698c9d4bed741cdff14a07f1c71c81b21908 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Jul 2022 13:55:03 +0000 Subject: [PATCH 1/3] 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. --- bin/tests/system/rrl/tests.sh | 13 ++---- lib/dns/include/dns/rrl.h | 4 +- lib/dns/rrl.c | 74 +++++++++++++++++++++-------------- lib/ns/client.c | 9 +++-- lib/ns/query.c | 8 ++-- 5 files changed, 60 insertions(+), 48 deletions(-) diff --git a/bin/tests/system/rrl/tests.sh b/bin/tests/system/rrl/tests.sh index ff03a3176d..e3207ca1cd 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 66c29ebe26..bdffed99ac 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 599e76b004..9ecc6c5dc5 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 6476edd59d..6c491afa59 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 93d2d6c353..beeae0e653 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 From 89c2032421e6d64dbf1655dccce7faccad9eaf87 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Jul 2022 14:13:28 +0000 Subject: [PATCH 2/3] Document RRL processing for wildcard names All valid wildcard domain names are interpreted as the zone's origin name concatenated to the "*" name. --- doc/arm/reference.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/arm/reference.rst b/doc/arm/reference.rst index 3b64f07bb4..c6fd9f505e 100644 --- a/doc/arm/reference.rst +++ b/doc/arm/reference.rst @@ -5497,7 +5497,9 @@ Response Rate Limiting All non-empty responses for a valid domain name (qname) and record type (qtype) are identical and have a limit specified with - :any:`responses-per-second` (default 0 or no limit). + :any:`responses-per-second` (default 0 or no limit). All valid wildcard + domain names are interpreted as the zone's origin name concatenated to the + "*" name. .. namedconf:statement:: nodata-per-second :tags: query From 0b0cf12741f4d975328c1e4fe95ee1bbc4af46ba Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Mon, 25 Jul 2022 14:59:41 +0000 Subject: [PATCH 3/3] Add CHANGES and release notes for [GL #3459] --- CHANGES | 4 ++++ doc/notes/notes-current.rst | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/CHANGES b/CHANGES index 280ccc71a6..a7c5f1c51e 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,7 @@ +5956. [func] Make RRL code treat all QNAMEs that are subject to + wildcard processing within a given zone as the same + name. [GL #3459] + 5955. [port] The libxml2 library has deprecated the usage of xmlInitThreads() and xmlCleanupThreads() functions. Use xmlInitParser() and xmlCleanupParser() instead. diff --git a/doc/notes/notes-current.rst b/doc/notes/notes-current.rst index e97eb97108..e758ca8966 100644 --- a/doc/notes/notes-current.rst +++ b/doc/notes/notes-current.rst @@ -37,6 +37,10 @@ Removed Features Feature Changes ~~~~~~~~~~~~~~~ +- Response Rate Limiting (RRL) code now treats all QNAMEs that are + subject to wildcard processing within a given zone as the same name, + to prevent circumventing the limits enforced by RRL. :gl:`#3459` + - Zones using ``dnssec-policy`` now require dynamic DNS or ``inline-signing`` to be configured explicitly :gl:`#3381`.