diff --git a/CHANGES b/CHANGES index 6470af7a5d..eed44fff49 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,8 @@ +4121. [bug] When updating a response-policy zone via AXFR, + summary data about other policy zones could fall + out of sync. Ultimately this could trigger an + assertion failure in rpz.c. [RT #39567] + 4120. [bug] A bug in RPZ could cause the server to crash if policy zones were updated while recursion was pending for RPZ processing of an active query. diff --git a/doc/arm/notes.xml b/doc/arm/notes.xml index 2a24c20030..72b46de8df 100644 --- a/doc/arm/notes.xml +++ b/doc/arm/notes.xml @@ -315,23 +315,37 @@ - A bug in the RPZ implementation could cause some policy - zones that did not specifically require recursion to be - treated as if they did; consequently, setting - qname-wait-recurse no; was - sometimes ineffective. This has been corrected. - In most configurations, behavioral changes due to this - fix will not be noticeable. [RT #39229] + Several bugs have been fixed in the RPZ implementation: - - - - A bug in RPZ could cause the server to crash if policy - zones were updated (e.g. via rndc reload - or an incoming zone transfer) while RPZ processing was still - ongoing for an active query. [RT #39415] - - + + + Policy zones that did not specifically require recursion + could be treated as if they did; consequently, setting + qname-wait-recurse no; was + sometimes ineffective. This has been corrected. + In most configurations, behavioral changes due to this + fix will not be noticeable. [RT #39229] + + + + + The server could crash if policy zones were updated (e.g. + via rndc reload or an incoming zone + transfer) while RPZ processing was still ongoing for an + active query. [RT #39415] + + + + + On servers with more than one policy zone configured, + if one policy zone was updated via AXFR (especially when + the zone was very large) a race condition could allow the + RPZ summary data to fall out of sync with the policy zones, + potentially leading to an assertion failure in rpz.c. + [RT #39567] + + + diff --git a/lib/dns/include/dns/rpz.h b/lib/dns/include/dns/rpz.h index f0201eb8ee..923ad436ce 100644 --- a/lib/dns/include/dns/rpz.h +++ b/lib/dns/include/dns/rpz.h @@ -101,20 +101,26 @@ typedef isc_uint32_t dns_rpz_zbits_t; #define DNS_RPZ_ZMASK(n) ((dns_rpz_zbits_t)((((n) >= DNS_RPZ_MAX_ZONES-1) ? \ 0 : (1<<((n)+1))) -1)) +/* + * The trigger counter type. + */ +typedef ssize_t dns_rpz_trigger_counter_t; + /* * The number of triggers of each type in a response policy zone. */ typedef struct dns_rpz_triggers dns_rpz_triggers_t; struct dns_rpz_triggers { - int client_ipv4; - int client_ipv6; - int qname; - int ipv4; - int ipv6; - int nsdname; - int nsipv4; - int nsipv6; + dns_rpz_trigger_counter_t client_ipv4; + dns_rpz_trigger_counter_t client_ipv6; + dns_rpz_trigger_counter_t qname; + dns_rpz_trigger_counter_t ipv4; + dns_rpz_trigger_counter_t ipv6; + dns_rpz_trigger_counter_t nsdname; + dns_rpz_trigger_counter_t nsipv4; + dns_rpz_trigger_counter_t nsipv6; }; + /* * A single response policy zone. */ @@ -204,6 +210,13 @@ struct dns_rpz_zones { */ dns_rpz_zbits_t load_begun; dns_rpz_have_t have; + + /* + * total_triggers maintains the total number of triggers in all + * policy zones in the view. It is only used to print summary + * statistics after a zone load of how the trigger counts + * changed. + */ dns_rpz_triggers_t total_triggers; isc_mem_t *mctx; diff --git a/lib/dns/rbtdb.c b/lib/dns/rbtdb.c index 74696af548..561c599070 100644 --- a/lib/dns/rbtdb.c +++ b/lib/dns/rbtdb.c @@ -1828,18 +1828,25 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { dns_fixedname_t fname; dns_name_t *name; isc_result_t result = ISC_R_UNEXPECTED; + unsigned int node_has_rpz; INSIST(!ISC_LINK_LINKED(node, deadlink)); switch (node->nsec) { case DNS_RBT_NSEC_NORMAL: - if (rbtdb->rpzs != NULL && node->rpz) { - dns_fixedname_init(&fname); - name = dns_fixedname_name(&fname); - dns_rbt_fullnamefromnode(node, name); - dns_rpz_delete(rbtdb->rpzs, rbtdb->rpz_num, name); - } + /* + * Though this may be wasteful, it has to be done before + * node is deleted. + */ + dns_fixedname_init(&fname); + name = dns_fixedname_name(&fname); + dns_rbt_fullnamefromnode(node, name); + + node_has_rpz = node->rpz; result = dns_rbt_deletenode(rbtdb->tree, node, ISC_FALSE); + if (result == ISC_R_SUCCESS && + rbtdb->rpzs != NULL && node_has_rpz) + dns_rpz_delete(rbtdb->rpzs, rbtdb->rpz_num, name); break; case DNS_RBT_NSEC_HAS_NSEC: dns_fixedname_init(&fname); @@ -1872,9 +1879,11 @@ delete_node(dns_rbtdb_t *rbtdb, dns_rbtnode_t *node) { isc_result_totext(result)); } } - if (rbtdb->rpzs != NULL && node->rpz) - dns_rpz_delete(rbtdb->rpzs, rbtdb->rpz_num, name); + node_has_rpz = node->rpz; result = dns_rbt_deletenode(rbtdb->tree, node, ISC_FALSE); + if (result == ISC_R_SUCCESS && + rbtdb->rpzs != NULL && node_has_rpz) + dns_rpz_delete(rbtdb->rpzs, rbtdb->rpz_num, name); break; case DNS_RBT_NSEC_NSEC: result = dns_rbt_deletenode(rbtdb->nsec, node, ISC_FALSE); @@ -7117,16 +7126,21 @@ loadnode(dns_rbtdb_t *rbtdb, dns_name_t *name, dns_rbtnode_t **nodep, } if (noderesult == ISC_R_SUCCESS) { - /* - * Clean rpz entries added above. - */ - if (rbtdb->rpzs != NULL && node->rpz) - dns_rpz_delete(rbtdb->load_rpzs, rbtdb->rpz_num, name); + unsigned int node_has_rpz; + /* * Remove the node we just added above. */ + node_has_rpz = node->rpz; tmpresult = dns_rbt_deletenode(rbtdb->tree, node, ISC_FALSE); - if (tmpresult != ISC_R_SUCCESS) + if (tmpresult == ISC_R_SUCCESS) { + /* + * Clean rpz entries added above. + */ + if (rbtdb->rpzs != NULL && node_has_rpz) + dns_rpz_delete(rbtdb->load_rpzs, + rbtdb->rpz_num, name); + } else { isc_log_write(dns_lctx, DNS_LOGCATEGORY_DATABASE, DNS_LOGMODULE_CACHE, @@ -7136,6 +7150,7 @@ loadnode(dns_rbtdb_t *rbtdb, dns_name_t *name, dns_rbtnode_t **nodep, "dns_rbt_addnode(NSEC): %s", isc_result_totext(tmpresult), isc_result_totext(noderesult)); + } } /* diff --git a/lib/dns/rpz.c b/lib/dns/rpz.c index ca03e9872e..baa6ee1782 100644 --- a/lib/dns/rpz.c +++ b/lib/dns/rpz.c @@ -552,7 +552,7 @@ adj_trigger_cnt(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num, const dns_rpz_cidr_key_t *tgt_ip, dns_rpz_prefix_t tgt_prefix, isc_boolean_t inc) { - int *cnt; + dns_rpz_trigger_counter_t *cnt; dns_rpz_zbits_t *have; switch (rpz_type) { @@ -1558,8 +1558,10 @@ dns_rpz_beginload(dns_rpz_zones_t **load_rpzsp, * reload the new zone data into a new blank summary database * if the reload fails, discard the new summary database * if the new zone data is acceptable, copy the records for the - * other zones into the new summary database and replace the - * old summary database with the new. + * other zones into the new summary CIDR and RBT databases + * and replace the old summary databases with the new, and + * correct the triggers and have values for the updated + * zone. * * At the first attempt to load a zone, there is no summary data * for the zone and so no records that need to be deleted. @@ -1578,30 +1580,33 @@ dns_rpz_beginload(dns_rpz_zones_t **load_rpzsp, */ rpzs->load_begun |= tgt; dns_rpz_attach_rpzs(rpzs, load_rpzsp); - UNLOCK(&rpzs->search_lock); - UNLOCK(&rpzs->maint_lock); - } else { - UNLOCK(&rpzs->search_lock); - UNLOCK(&rpzs->maint_lock); - + /* + * Setup the new RPZ struct with empty summary trees. + */ result = dns_rpz_new_zones(load_rpzsp, rpzs->mctx); if (result != ISC_R_SUCCESS) return (result); load_rpzs = *load_rpzsp; + /* + * Initialize some members so that dns_rpz_add() works. + */ load_rpzs->p.num_zones = rpzs->p.num_zones; - load_rpzs->total_triggers = rpzs->total_triggers; - memmove(load_rpzs->triggers, rpzs->triggers, - sizeof(load_rpzs->triggers)); - memset(&load_rpzs->triggers[rpz_num], 0, - sizeof(load_rpzs->triggers[rpz_num])); + memset(&load_rpzs->triggers, 0, sizeof(load_rpzs->triggers)); load_rpzs->zones[rpz_num] = rpz; isc_refcount_increment(&rpz->refs, NULL); } + UNLOCK(&rpzs->search_lock); + UNLOCK(&rpzs->maint_lock); + return (ISC_R_SUCCESS); } +/* + * This function updates "have" bits and also the qname_skip_recurse + * mask. It must be called when holding rpzs->search_lock. + */ static void fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { dns_rpz_num_t n; @@ -1609,7 +1614,14 @@ fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { dns_rpz_zbits_t zbit; char namebuf[DNS_NAME_FORMATSIZE]; -# define SET_TRIG(n, zbit, type) \ + /* + * rpzs->total_triggers is only used to log a message below. + */ + + memmove(&old_totals, &rpzs->total_triggers, sizeof(old_totals)); + memset(&rpzs->total_triggers, 0, sizeof(rpzs->total_triggers)); + +#define SET_TRIG(n, zbit, type) \ if (rpzs->triggers[n].type == 0) { \ rpzs->have.type &= ~zbit; \ } else { \ @@ -1617,8 +1629,6 @@ fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { rpzs->have.type |= zbit; \ } - memmove(&old_totals, &rpzs->total_triggers, sizeof(old_totals)); - memset(&rpzs->total_triggers, 0, sizeof(rpzs->total_triggers)); for (n = 0; n < rpzs->p.num_zones; ++n) { zbit = DNS_RPZ_ZBIT(n); SET_TRIG(n, zbit, client_ipv4); @@ -1631,6 +1641,8 @@ fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { SET_TRIG(n, zbit, nsipv6); } +#undef SET_TRIG + fix_qname_skip_recurse(rpzs); dns_name_format(&rpzs->zones[rpz_num]->origin, @@ -1638,8 +1650,8 @@ fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { isc_log_write(dns_lctx, DNS_LOGCATEGORY_RPZ, DNS_LOGMODULE_RBTDB, DNS_RPZ_INFO_LEVEL, "(re)loading policy zone '%s' changed from" - " %d to %d qname, %d to %d nsdname," - " %d to %d IP, %d to %d NSIP entries", + " %ld to %ld qname, %ld to %ld nsdname," + " %ld to %ld IP, %ld to %ld NSIP entries", namebuf, old_totals.qname, rpzs->total_triggers.qname, old_totals.nsdname, rpzs->total_triggers.nsdname, @@ -1647,13 +1659,48 @@ fix_triggers(dns_rpz_zones_t *rpzs, dns_rpz_num_t rpz_num) { rpzs->total_triggers.ipv4 + rpzs->total_triggers.ipv6, old_totals.nsipv4 + old_totals.nsipv6, rpzs->total_triggers.nsipv4 + rpzs->total_triggers.nsipv6); - -# undef SET_TRIG } /* - * Finish loading one zone. - * The RBTDB write tree lock must be held. + * Finish loading one zone. This function is called during a commit when + * a RPZ zone loading is complete. The RBTDB write tree lock must be + * held. + * + * Here, rpzs is a pointer to the view's common rpzs + * structure. *load_rpzsp is a rpzs structure that is local to the + * RBTDB, which is used during a single zone's load. + * + * During the zone load, i.e., between dns_rpz_beginload() and + * dns_rpz_ready(), only the zone that is being loaded updates + * *load_rpzsp. These updates in the summary databases inside load_rpzsp + * are made only for the rpz_num (and corresponding bit) of that + * zone. Nothing else reads or writes *load_rpzsp. The view's common + * rpzs is used during this time for queries. + * + * When zone loading is complete and we arrive here, the parts of the + * summary databases (CIDR and nsdname+qname RBT trees) from the view's + * common rpzs struct have to be merged into the summary databases of + * *load_rpzsp, as the summary databases of the view's common rpzs + * struct may have changed during the time the zone was being loaded. + * + * The function below carries out the merge. During the merge, it holds + * the maint_lock of the view's common rpzs struct so that it is not + * updated while the merging is taking place. + * + * After the merging is carried out, *load_rpzsp contains the most + * current state of the rpzs structure, i.e., the summary trees contain + * data for the new zone that was just loaded, as well as all other + * zones. + * + * Pointers to the summary databases of *load_rpzsp (CIDR and + * nsdname+qname RBT trees) are then swapped into the view's common rpz + * struct, so that the query path can continue using it. During the + * swap, the search_lock of the view's common rpz struct is acquired so + * that queries are paused while this swap occurs. + * + * The trigger counts for the new zone are also copied into the view's + * common rpz struct, and some other summary counts and masks are + * updated. */ isc_result_t dns_rpz_ready(dns_rpz_zones_t *rpzs, @@ -1679,10 +1726,12 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, if (load_rpzs == rpzs) { /* - * This is a successful initial zone loading, - * perhaps for a new instance of a view. + * This is a successful initial zone loading, perhaps + * for a new instance of a view. */ + LOCK(&rpzs->search_lock); fix_triggers(rpzs, rpz_num); + UNLOCK(&rpzs->search_lock); UNLOCK(&rpzs->maint_lock); dns_rpz_detach_rpzs(load_rpzsp); return (ISC_R_SUCCESS); @@ -1785,13 +1834,14 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, } } - fix_triggers(load_rpzs, rpz_num); - /* * Exchange the summary databases. */ LOCK(&rpzs->search_lock); + rpzs->triggers[rpz_num] = load_rpzs->triggers[rpz_num]; + fix_triggers(rpzs, rpz_num); + found = rpzs->cidr; rpzs->cidr = load_rpzs->cidr; load_rpzs->cidr = found; @@ -1800,9 +1850,6 @@ dns_rpz_ready(dns_rpz_zones_t *rpzs, rpzs->rbt = load_rpzs->rbt; load_rpzs->rbt = rbt; - rpzs->total_triggers = load_rpzs->total_triggers; - rpzs->have = load_rpzs->have; - UNLOCK(&rpzs->search_lock); result = ISC_R_SUCCESS;