From 0ef9a46e3360cd5e9e5c4a8feb50afbff0a25c79 Mon Sep 17 00:00:00 2001 From: Shawn Routhier Date: Fri, 30 Dec 2011 23:08:41 +0000 Subject: [PATCH] Modify the DDNS handling code. In a previous patch we added logging code to the DDNS handling. This code included a bug that caused it to attempt to dereference a NULL pointer and eventually segfault. While reviewing the code as we addressed this problem, we determined that some of the updates to the lease structures would not work as planned since the structures being updated were in the process of being freed: these updates were removed. In addition we removed an incorrect call to the DDNS removal function that could cause a failure during the removal of DDNS information from the DNS server. Thanks to Jasper Jongmans for reporting this issue. [ISC-Bugs #27078] CVE: CVE-2011-4868 --- RELNOTES | 13 ++++++++++++ includes/dhcpd.h | 6 +++--- server/ddns.c | 52 ++++++++++++++++++++++++++++++++++++++++------- server/failover.c | 2 +- server/mdb.c | 10 ++++----- server/mdb6.c | 19 ++++++----------- 6 files changed, 73 insertions(+), 29 deletions(-) diff --git a/RELNOTES b/RELNOTES index eb5bd2a61..f1a50e6b4 100644 --- a/RELNOTES +++ b/RELNOTES @@ -48,6 +48,19 @@ work on other platforms. Please report any problems and suggested fixes to [ISC-Bugs #26704]. CVE: CVE-2011-4539 +! Modify the DDNS handling code. In a previous patch we added logging + code to the DDNS handling. This code included a bug that caused it + to attempt to dereference a NULL pointer and eventually segfault. + While reviewing the code as we addressed this problem, we determined + that some of the updates to the lease structures would not work as + planned since the structures being updated were in the process of + being freed: these updates were removed. In addition we removed an + incorrect call to the DDNS removal function that could cause a failure + during the removal of DDNS information from the DNS server. + Thanks to Jasper Jongmans for reporting this issue. + [ISC-Bugs #27078] + CVE: CVE-2011-4868 + Changes since 4.2.2 - Fix the code that checks for an existing DDNS transaction to cancel diff --git a/includes/dhcpd.h b/includes/dhcpd.h index a4d81f34b..566e1e30c 100644 --- a/includes/dhcpd.h +++ b/includes/dhcpd.h @@ -1539,7 +1539,7 @@ struct ipv6_pool { #define DDNS_EXECUTE_NEXT 0x20 #define DDNS_ABORT 0x40 #define DDNS_STATIC_LEASE 0x80 - +#define DDNS_ACTIVE_LEASE 0x100 /* * The following two groups are separate and we could reuse * values but not reusing them may be useful in the future. @@ -1580,7 +1580,7 @@ typedef struct dhcp_ddns_cb { int zone_addr_count; struct dns_zone *zone; - int flags; + u_int16_t flags; TIME timeout; int state; ddns_action_t cur_func; @@ -1932,7 +1932,7 @@ void parse_server_duid_conf(struct parse *cfile); /* ddns.c */ int ddns_updates(struct packet *, struct lease *, struct lease *, struct iasubopt *, struct iasubopt *, struct option_state *); -int ddns_removals(struct lease *, struct iasubopt *, struct dhcp_ddns_cb *); +int ddns_removals(struct lease *, struct iasubopt *, struct dhcp_ddns_cb *, isc_boolean_t); #if defined (TRACING) void trace_ddns_init(void); #endif diff --git a/server/ddns.c b/server/ddns.c index b003bd0b1..d92a7ef1b 100644 --- a/server/ddns.c +++ b/server/ddns.c @@ -124,8 +124,12 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old, if (ddns_cb == NULL) { return(0); } - /* assume that we shall update both the A and ptr records */ - ddns_cb->flags = DDNS_UPDATE_ADDR | DDNS_UPDATE_PTR; + /* + * Assume that we shall update both the A and ptr records and, + * as this is an update, set the active flag + */ + ddns_cb->flags = DDNS_UPDATE_ADDR | DDNS_UPDATE_PTR | + DDNS_ACTIVE_LEASE; /* * For v4 we flag static leases so we don't try @@ -330,7 +334,7 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old, /* If desired do the removals */ if (do_remove != 0) { - (void) ddns_removals(lease, lease6, NULL); + (void) ddns_removals(lease, lease6, NULL, ISC_TRUE); } goto out; } @@ -532,7 +536,7 @@ ddns_updates(struct packet *packet, struct lease *lease, struct lease *old, * the ddns messages. Currently we don't. */ if (do_remove) { - rcode1 = ddns_removals(lease, lease6, ddns_cb); + rcode1 = ddns_removals(lease, lease6, ddns_cb, ISC_TRUE); } else { ddns_fwd_srv_connector(lease, lease6, scope, ddns_cb, @@ -740,6 +744,15 @@ ddns_update_lease_text(dhcp_ddns_cb_t *ddns_cb, if (ddns_cb->flags & DDNS_STATIC_LEASE) return (ISC_R_SUCCESS); + /* + * If we are processing an expired or released v6 lease + * we don't actually have a scope to update + */ + if ((ddns_cb->address.len == 16) && + ((ddns_cb->flags & DDNS_ACTIVE_LEASE) == 0)) { + return (ISC_R_SUCCESS); + } + if (inscope != NULL) { scope = inscope; } else if (ddns_cb->address.len == 4) { @@ -1085,6 +1098,15 @@ ddns_update_lease_ptr(struct lease *lease, return (ISC_R_SUCCESS); } + /* + * If we are processing an expired or released v6 lease + * we don't actually have a scope to update + */ + if ((ddns_cb->address.len == 16) && + ((ddns_cb->flags & DDNS_ACTIVE_LEASE) == 0)) { + return (ISC_R_SUCCESS); + } + if (lease != NULL) { safe_lease_update(lease, ddns_cb, ddns_cb_set, file, line); @@ -1132,7 +1154,7 @@ ddns_update_lease_ptr(struct lease *lease, ISC_R_SUCCESS) && (find_ipv6_pool(&pool, D6O_IA_NA, &addr) != ISC_R_SUCCESS)) { - inet_ntop(AF_INET6, &lease6->addr, addrbuf, + inet_ntop(AF_INET6, &addr, addrbuf, MAX_ADDRESS_STRING_LEN); log_error("%s(%d): Pool for lease %s not found.", file, line, addrbuf); @@ -1152,7 +1174,7 @@ ddns_update_lease_ptr(struct lease *lease, find_lease6->ddns_cb = ddns_cb_set; iasubopt_dereference(&find_lease6, MDL); } else { - inet_ntop(AF_INET6, &lease6->addr, addrbuf, + inet_ntop(AF_INET6, &addr, addrbuf, MAX_ADDRESS_STRING_LEN); log_error("%s(%d): Lease %s not found within pool.", file, line, addrbuf); @@ -1594,12 +1616,18 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb, * 0 - badness occurred and we weren't able to do what was wanted * 1 - we were able to do stuff but it's in progress * in both cases any additional block has been passed on to it's handler + * + * active == ISC_TRUE if the lease is still active, and FALSE if the lease + * is inactive. This is used to indicate if the lease is inactive or going + * to inactive in IPv6 so we can avoid trying to update the lease with + * cb pointers and text information. */ int ddns_removals(struct lease *lease, struct iasubopt *lease6, - dhcp_ddns_cb_t *add_ddns_cb) + dhcp_ddns_cb_t *add_ddns_cb, + isc_boolean_t active) { isc_result_t rcode, execute_add = ISC_R_FAILURE; struct binding_scope **scope = NULL; @@ -1644,6 +1672,16 @@ ddns_removals(struct lease *lease, } else goto cleanup; + /* + * Set the flag bit if the lease is active, that is it isn't + * expired or released. This is used in the IPv6 paths to + * determine if we need to update the lease when the response + * from the DNS code is processed. + */ + if (active == ISC_TRUE) { + ddns_cb->flags |= DDNS_ACTIVE_LEASE; + } + /* No scope implies that DDNS has not been performed for this lease. */ if (*scope == NULL) goto cleanup; diff --git a/server/failover.c b/server/failover.c index 97e7d734a..26be290cc 100644 --- a/server/failover.c +++ b/server/failover.c @@ -5218,7 +5218,7 @@ isc_result_t dhcp_failover_process_bind_update (dhcp_failover_state_t *state, */ if (msg->binding_status == FTS_ACTIVE && (chaddr_changed || ident_changed)) { - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); if (lease->scope != NULL) binding_scope_dereference(&lease->scope, MDL); diff --git a/server/mdb.c b/server/mdb.c index 53c1b3091..cdf4ff1bc 100644 --- a/server/mdb.c +++ b/server/mdb.c @@ -1446,7 +1446,7 @@ void make_binding_state_transition (struct lease *lease) lease -> binding_state == FTS_ACTIVE && lease -> next_binding_state != FTS_RELEASED))) { #if defined (NSUPDATE) - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); #endif if (lease -> on_expiry) { execute_statements ((struct binding_value **)0, @@ -1512,7 +1512,7 @@ void make_binding_state_transition (struct lease *lease) * release message. This is not true of expiry, where the * peer may have extended the lease. */ - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); #endif if (lease -> on_release) { execute_statements ((struct binding_value **)0, @@ -1681,7 +1681,7 @@ void release_lease (lease, packet) /* If there are statements to execute when the lease is released, execute them. */ #if defined (NSUPDATE) - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); #endif if (lease -> on_release) { execute_statements ((struct binding_value **)0, @@ -1755,7 +1755,7 @@ void abandon_lease (lease, message) { struct lease *lt = (struct lease *)0; #if defined (NSUPDATE) - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); #endif if (!lease_copy (<, lease, MDL)) @@ -1787,7 +1787,7 @@ void dissociate_lease (lease) { struct lease *lt = (struct lease *)0; #if defined (NSUPDATE) - ddns_removals(lease, NULL, NULL); + ddns_removals(lease, NULL, NULL, ISC_FALSE); #endif if (!lease_copy (<, lease, MDL)) diff --git a/server/mdb6.c b/server/mdb6.c index 9d410f551..1925be00a 100644 --- a/server/mdb6.c +++ b/server/mdb6.c @@ -1058,7 +1058,7 @@ move_lease_to_inactive(struct ipv6_pool *pool, struct iasubopt *lease, #if defined (NSUPDATE) /* Process events upon expiration. */ if (pool->pool_type != D6O_IA_PD) { - ddns_removals(NULL, lease, NULL); + ddns_removals(NULL, lease, NULL, ISC_FALSE); } #endif @@ -1466,6 +1466,11 @@ lease_timeout_support(void *vpool) { * Note that if there are no leases in the pool, * expire_lease6() will return ISC_R_SUCCESS with * a NULL lease. + * + * expire_lease6() will call move_lease_to_inactive() which + * calls ddns_removals() do we want that on the standard + * expiration timer or a special 'depref' timer? Original + * query from DH, moved here by SAR. */ lease = NULL; if (expire_lease6(&lease, pool, cur_time) != ISC_R_SUCCESS) { @@ -1475,18 +1480,6 @@ lease_timeout_support(void *vpool) { break; } - /* Look to see if there were ddns updates, and if - * so, drop them. - * - * DH: Do we want to do this on a special 'depref' - * timer rather than expiration timer? - */ -#if defined (NSUPDATE) - if (pool->pool_type != D6O_IA_PD) { - ddns_removals(NULL, lease, NULL); - } -#endif - write_ia(lease->ia); iasubopt_dereference(&lease, MDL); -- 2.47.2