ComputerSurge/bind9
3
0
Fork 0
Code Pull Requests 1 Activity

[v9_10] ensure rpz summary consistence during AXFR updates

Browse Source 
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]

(cherry picked from commit 19365b43e9)
This commit is contained in:
Evan Hunt
2015-05-20 15:07:09 -07:00
parent 8655100300
commit 433b64a548
5 changed files with 162 additions and 68 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
CHANGES
View File

@@ -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.

46
doc/arm/notes.xml
View File

@@ -115,23 +115,37 @@
</listitem>
<listitem>
<para>
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
<command>qname-wait-recurse no;</command> 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:
</para>
</listitem>
<listitem>
<para>
A bug in RPZ could cause the server to crash if policy
zones were updated (e.g. via <command>rndc reload</command>
or an incoming zone transfer) while RPZ processing was still
ongoing for an active query. [RT #39415]
</para>
</listitem>
<itemizedlist>
<para>
Policy zones that did not specifically require recursion
could be treated as if they did; consequently, setting
<command>qname-wait-recurse no;</command> was
sometimes ineffective. This has been corrected.
In most configurations, behavioral changes due to this
fix will not be noticeable. [RT #39229]
</para>
</listitem>
<listitem>
<para>
The server could crash if policy zones were updated (e.g.
via <command>rndc reload</command> or an incoming zone
transfer) while RPZ processing was still ongoing for an
active query. [RT #39415]
</para>
</listitem>
<listitem>
<para>
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]
</para>
</listitem>
</itemizedlist>
</itemizedlist>
</sect2>
<sect2 id="end_of_life">

29
lib/dns/include/dns/rpz.h
View File

@@ -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;

43
lib/dns/rbtdb.c
View File

@@ -1832,18 +1832,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);
@@ -1876,9 +1883,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);
@@ -7125,16 +7134,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,
@@ -7144,6 +7158,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));
}
}
/*

107
lib/dns/rpz.c
View File

@@ -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;