From: Shawn Routhier Date: Fri, 8 Jul 2011 22:49:11 +0000 (+0000) Subject: DNS Update fix. A misconfigured server could crash during DNS update X-Git-Tag: v4_3_0a1~159 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=beaed73f000ac0da3f300eb2c46358d46b92ac2c;p=thirdparty%2Fdhcp.git DNS Update fix. A misconfigured server could crash during DNS update processing if the configuration included overlapping pools or multiple fixed-address entries for a single address. This issue affected both IPv4 and IPv6. The fix allows a server to detect such conditions, provides the user with extra information and recommended steps to fix the problem. If the user enables the appropriate option in site.h then server will be terminated --- diff --git a/RELNOTES b/RELNOTES index 265f8329d..01d8772fd 100644 --- a/RELNOTES +++ b/RELNOTES @@ -181,6 +181,15 @@ work on other platforms. Please report any problems and suggested fixes to See ACCEPT_LIST_IN_DOMAIN_NAME define in includes/site.h. [ISC-Bugs #24167] +- DNS Update fix. A misconfigured server could crash during DNS update + processing if the configuration included overlapping pools or + multiple fixed-address entries for a single address. This issue + affected both IPv4 and IPv6. The fix allows a server to detect such + conditions, provides the user with extra information and recommended + steps to fix the problem. If the user enables the appropriate option + in site.h then server will be terminated + [ISC-Bugs #23595] + Changes since 4.2.0 - Documentation cleanup covering multiple tickets diff --git a/common/dns.c b/common/dns.c index 8f1db46b7..5ecae6725 100644 --- a/common/dns.c +++ b/common/dns.c @@ -3,7 +3,7 @@ Domain Name Service subroutines. */ /* - * Copyright (c) 2009-2010 by Internet Systems Consortium, Inc. ("ISC") + * Copyright (c) 2009-2011 by Internet Systems Consortium, Inc. ("ISC") * Copyright (c) 2004-2007 by Internet Systems Consortium, Inc. ("ISC") * Copyright (c) 2001-2003 by Internet Software Consortium * @@ -448,12 +448,20 @@ ddns_cb_alloc(const char *file, int line) } } +#if defined (DEBUG_DNS_UPDATES) + log_info("%s(%d): Allocating ddns_cb=%p", file, line, ddns_cb); +#endif + return(ddns_cb); } void ddns_cb_free(dhcp_ddns_cb_t *ddns_cb, const char *file, int line) { +#if defined (DEBUG_DNS_UPDATES) + log_info("%s(%d): freeing ddns_cb=%p", file, line, ddns_cb); +#endif + data_string_forget(&ddns_cb->fwd_name, file, line); data_string_forget(&ddns_cb->rev_name, file, line); data_string_forget(&ddns_cb->dhcid, file, line); diff --git a/common/print.c b/common/print.c index fb2ca0fba..e8eac7994 100644 --- a/common/print.c +++ b/common/print.c @@ -1417,14 +1417,24 @@ print_dns_status (int direction, } en = " dhcid: "; - if (s + strlen(en) + ddns_cb->dhcid.len < end) { - strcpy(s, en); - s += strlen(s); - strncpy(s, (char *)ddns_cb->dhcid.data + 1, - ddns_cb->dhcid.len - 1); - s += strlen(s); + if (ddns_cb->dhcid.len > 0) { + if (s + strlen(en) + ddns_cb->dhcid.len-1 < end) { + strcpy(s, en); + s += strlen(s); + strncpy(s, (char *)ddns_cb->dhcid.data+1, + ddns_cb->dhcid.len-1); + s += strlen(s); + } else { + goto bailout; + } } else { - goto bailout; + en = " dhcid: "; + if (s + strlen(en) < end) { + strcpy(s, en); + s += strlen(s); + } else { + goto bailout; + } } en = " ttl: "; diff --git a/includes/dhcpd.h b/includes/dhcpd.h index 389f03eec..f5240117c 100644 --- a/includes/dhcpd.h +++ b/includes/dhcpd.h @@ -3,7 +3,7 @@ Definitions for dhcpd... */ /* - * Copyright (c) 2004-2010 by Internet Systems Consortium, Inc. ("ISC") + * Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC") * Copyright (c) 1996-2003 by Internet Software Consortium * * Permission to use, copy, modify, and distribute this software for any @@ -3529,3 +3529,6 @@ ddns_modify_ptr(dhcp_ddns_cb_t *ddns_cb); void ddns_cancel(dhcp_ddns_cb_t *ddns_cb); + +#define MAX_ADDRESS_STRING_LEN \ + (sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")) diff --git a/includes/site.h b/includes/site.h index 258e37b5e..b78b5371e 100644 --- a/includes/site.h +++ b/includes/site.h @@ -217,6 +217,10 @@ but is only included for backwards compatibility. */ /* #define REPLY_TO_SOURCE_PORT */ +/* Define this if you want to enable strict checks in DNS Updates mechanism. + Do not enable this unless are DHCP developer. */ +/* #define DNS_UPDATES_MEMORY_CHECKS */ + /* Define this if you want to allow domain list in domain-name option. RFC2132 does not allow that behavior, but it is somewhat used due to historic reasons. Note that it may be removed some time in the diff --git a/server/ddns.c b/server/ddns.c index cfbdd04ec..99c087148 100644 --- a/server/ddns.c +++ b/server/ddns.c @@ -809,6 +809,217 @@ ddns_update_lease_text(dhcp_ddns_cb_t *ddns_cb, return(ISC_R_SUCCESS); } +/* + * This function should be called when update_lease_ptr function fails. + * It does inform user about the condition, provides some hints how to + * resolve this and dies gracefully. This can happend in at least three + * cases (all are configuration mistakes): + * a) IPv4: user have duplicate fixed-address entries (the same + * address is defined twice). We may have found wrong lease. + * b) IPv6: user have overlapping pools (we tried to find + * a lease in a wrong pool) + * c) IPv6: user have duplicate fixed-address6 entires (the same + * address is defined twice). We may have found wrong lease. + * + * Comment: while it would be possible to recover from both cases + * by forcibly searching for leases in *all* following pools, that would + * only hide the real problem - a misconfiguration. Proper solution + * is to log the problem, die and let the user fix his config file. + */ +void +update_lease_failed(struct lease *lease, + struct iasubopt *lease6, + dhcp_ddns_cb_t *ddns_cb, + dhcp_ddns_cb_t *ddns_cb_set, + const char * file, int line) +{ + char lease_address[MAX_ADDRESS_STRING_LEN + 64]; + char reason[128]; /* likely reason */ + + sprintf(reason, "unknown"); + sprintf(lease_address, "unknown"); + + /* let's pretend that everything is ok, so we can continue for + for information gathering purposes */ + + if (ddns_cb != NULL) { + strncpy(lease_address, piaddr(ddns_cb->address), + MAX_ADDRESS_STRING_LEN); + + if (ddns_cb->address.len == 4) { + sprintf(reason, "duplicate IPv4 fixed-address entry"); + } else if (ddns_cb->address.len == 16) { + sprintf(reason, "duplicate IPv6 fixed-address6 entry " + "or overlapping pools"); + } else { + /* + * Should not happen. We have non-IPv4, non-IPv6 + * address. Something is very wrong here. + */ + sprintf(reason, "corrupted ddns_cb structure (address " + "length is %d)", ddns_cb->address.len); + } + } + + log_error("Failed to properly update internal lease structure with " + "DDNS"); + log_error("control block structures. Tried to update lease for" + "%s address, ddns_cb=%p.", lease_address, ddns_cb); + + log_error("%s", ""); + log_error("This condition can occur, if DHCP server configuration is " + "inconsistent."); + log_error("In particular, please do check that your configuration:"); + log_error("a) does not have overlapping pools (especially containing"); + log_error(" %s address).", lease_address); + log_error("b) there are no duplicate fixed-address or fixed-address6"); + log_error("entries for the %s address.", lease_address); + log_error("%s", ""); + log_error("Possible reason for this failure: %s", reason); + + log_fatal("%s(%d): Failed to update lease database with DDNS info for " + "address %s. Lease database inconsistent. Unable to recover." + " Terminating.", file, line, lease_address); +} + +/* + * utility function to update found lease. It does extra checks + * that we are indeed updating the right lease. It may happen + * that user have duplicate fixed-address entries, so we attempt + * to update wrong lease. See also safe_lease6_update. + */ + +void +safe_lease_update(struct lease *lease, + struct iasubopt *lease6, + dhcp_ddns_cb_t *oldcb, + dhcp_ddns_cb_t *newcb, + const char *file, int line) +{ + char addrbuf[MAX_ADDRESS_STRING_LEN]; + + if (lease != NULL) { + if ( (lease->ddns_cb == NULL) && (newcb == NULL) ) { + /* + * Trying to clean up pointer that is already null. We + * are most likely trying to update wrong lease here. + */ + + /* + * this error message pops out during every DNS Update + * for fixed leases. It is enabled only for debugging + * DNS Updates. Investigation pending. + */ +#if defined (DEBUG_DNS_UPDATES) + log_error("%s(%d): Invalid lease update. Tried to " + "clear already NULL DDNS control block " + "pointer for lease %s.", + file, line, piaddr(lease->ip_addr) ); +#endif + +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(lease, NULL, oldcb, newcb, file, + line); +#endif + /* + * May not reach this: update_lease_failed calls + * log_fatal. + */ + return; + } + + if ( (lease->ddns_cb != NULL) && (lease->ddns_cb != oldcb) ) { + /* + * There is existing cb structure, but it differs from + * what we expected to see there. Most likely we are + * trying to update wrong lease. + */ + log_error("%s(%d): Failed to update internal lease " + "structure with DDNS control block. Existing" + " ddns_cb structure does not match " + "expectations.IPv4=%s, old ddns_cb=%p, tried" + "to update to new ddns_cb=%p", file, line, + piaddr(lease->ip_addr), oldcb, newcb); + +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(lease, NULL, oldcb, newcb, file, + line); +#endif + /* + * May not reach this: update_lease_failed calls + * log_fatal. + */ + return; + } + /* additional IPv4 specific checks may be added here */ + + /* update the lease */ + lease->ddns_cb = newcb; + } else if (lease6 != NULL) { + if ( (lease6->ddns_cb == NULL) && (newcb == NULL) ) { + inet_ntop(AF_INET6, &lease6->addr, addrbuf, + MAX_ADDRESS_STRING_LEN); + /* + * Trying to clean up pointer that is already null. We + * are most likely trying to update wrong lease here. + */ +#if defined (DEBUG_DNS_UPDATES) + log_error("%s(%d): Failed to update internal lease " + "structure. Tried to clear already NULL " + "DDNS control block pointer for lease %s.", + file, line, addrbuf); +#endif + +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(NULL, lease6, oldcb, newcb, file, + line); +#endif + + /* + * May not reach this: update_lease_failed calls + * log_fatal. + */ + return; + } + + if ( (lease6->ddns_cb != NULL) && (lease6->ddns_cb != oldcb) ) { + /* + * there is existing cb structure, but it differs from + * what we expected to see there. Most likely we are + * trying to update wrong lease. + */ + inet_ntop(AF_INET6, &lease6->addr, addrbuf, + MAX_ADDRESS_STRING_LEN); + + log_error("%s(%d): Failed to update internal lease " + "structure with DDNS control block. Existing" + " ddns_cb structure does not match " + "expectations.IPv6=%s, old ddns_cb=%p, tried" + "to update to new ddns_cb=%p", file, line, + addrbuf, oldcb, newcb); + +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(NULL, lease6, oldcb, newcb, file, + line); +#endif + /* + * May not reach this: update_lease_failed calls + * log_fatal. + */ + return; + } + /* additional IPv6 specific checks may be added here */ + + /* update the lease */ + lease6->ddns_cb = newcb; + + } else { + /* should never get here */ + log_fatal("Impossible condition at %s:%d (called from %s:%d).", + MDL, file, line); + } +} + /* * Utility function to update the pointer to the DDNS control block * in a lease. @@ -828,32 +1039,84 @@ isc_result_t ddns_update_lease_ptr(struct lease *lease, struct iasubopt *lease6, dhcp_ddns_cb_t *ddns_cb, - dhcp_ddns_cb_t *ddns_cb_set) + dhcp_ddns_cb_t *ddns_cb_set, + const char * file, int line) { + char ddns_address[MAX_ADDRESS_STRING_LEN]; + sprintf(ddns_address, "uknown"); + if (ddns_cb) { + strncpy(ddns_address, piaddr(ddns_cb->address), + MAX_ADDRESS_STRING_LEN); + } +#if defined (DEBUG_DNS_UPDATES) + log_info("%s(%d): Updating lease_ptr for ddns_cp=%p (addr=%s)", + file, line, ddns_cb, ddns_address ); +#endif + if (lease != NULL) { - lease->ddns_cb = ddns_cb_set; + safe_lease_update(lease, NULL, ddns_cb, ddns_cb_set, + file, line); + /* lease->ddns_cb = ddns_cb_set; */ } else if (lease6 != NULL) { - lease6->ddns_cb = ddns_cb_set; + safe_lease_update(NULL, lease6, ddns_cb, ddns_cb_set, + file, line); + /* lease6->ddns_cb = ddns_cb_set; */ } else if (ddns_cb->address.len == 4) { struct lease *find_lease = NULL; if (find_lease_by_ip_addr(&find_lease, ddns_cb->address, MDL) != 0) { - find_lease->ddns_cb = ddns_cb_set; +#if defined (DEBUG_DNS_UPDATES) + log_info("%s(%d): find_lease_by_ip_addr(%s) successful:" + "lease=%p", file, line, ddns_address, + find_lease); +#endif + + /* find_lease->ddns_cb = ddns_cb_set; */ + safe_lease_update(find_lease, NULL, ddns_cb, + ddns_cb_set, file, line); lease_dereference(&find_lease, MDL); } else { - return(ISC_R_FAILURE); +#if defined (DEBUG_DNS_UPDATES) + log_error("%s(%d): ddns_update_lease_ptr failed. " + "Lease for %s not found. (Is it static lease?)", + file, line, piaddr(ddns_cb->address)); +#endif + +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(NULL, NULL, ddns_cb, ddns_cb_set, + file, line); +#endif + /* + * may not reach this. update_lease_failed + * calls log_fatal. + */ + return(ISC_R_FAILURE); + } } else if (ddns_cb->address.len == 16) { struct iasubopt *find_lease6 = NULL; struct ipv6_pool *pool = NULL; struct in6_addr addr; + char addrbuf[MAX_ADDRESS_STRING_LEN]; memcpy(&addr, &ddns_cb->address.iabuf, 16); if ((find_ipv6_pool(&pool, D6O_IA_TA, &addr) != ISC_R_SUCCESS) && (find_ipv6_pool(&pool, D6O_IA_NA, &addr) != ISC_R_SUCCESS)) { + inet_ntop(AF_INET6, &lease6->addr, addrbuf, + MAX_ADDRESS_STRING_LEN); + log_error("%s(%d): Pool for lease %s not found.", + file, line, addrbuf); +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(NULL, NULL, ddns_cb, ddns_cb_set, + file, line); +#endif + /* + * never reached. update_lease_failed + * calls log_fatal. + */ return(ISC_R_FAILURE); } @@ -862,12 +1125,25 @@ 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, + MAX_ADDRESS_STRING_LEN); + log_error("%s(%d): Lease %s not found within pool.", + file, line, addrbuf); +#if defined (DNS_UPDATES_MEMORY_CHECKS) + update_lease_failed(NULL, NULL, ddns_cb, ddns_cb_set, + file, line); +#endif + /* + * never reached. update_lease_failed + * calls log_fatal. + */ return(ISC_R_FAILURE); } ipv6_pool_dereference(&pool, MDL); } else { /* shouldn't get here */ - log_fatal("Impossible condition at %s:%d.", MDL); + log_fatal("Impossible condition at %s:%d, called from %s:%d.", + MDL, file, line); } return(ISC_R_SUCCESS); @@ -894,7 +1170,7 @@ ddns_ptr_add(dhcp_ddns_cb_t *ddns_cb, isc_result_totext (eresult)); } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_cb_free(ddns_cb, MDL); /* * A single DDNS operation may require several calls depending on @@ -948,7 +1224,7 @@ ddns_ptr_remove(dhcp_ddns_cb_t *ddns_cb, break; } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_fwd_srv_connector(NULL, NULL, NULL, ddns_cb->next_op, result); ddns_cb_free(ddns_cb, MDL); return; @@ -989,8 +1265,7 @@ ddns_fwd_srv_add2(dhcp_ddns_cb_t *ddns_cb, { isc_result_t result; const char *logstr = NULL; - char ddns_address[ - sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")]; + char ddns_address[MAX_ADDRESS_STRING_LEN]; /* Construct a printable form of the address for logging */ strcpy(ddns_address, piaddr(ddns_cb->address)); @@ -1042,7 +1317,7 @@ ddns_fwd_srv_add2(dhcp_ddns_cb_t *ddns_cb, ddns_address, logstr); } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_cb_free(ddns_cb, MDL); /* * A single DDNS operation may require several calls depending on @@ -1063,8 +1338,7 @@ ddns_fwd_srv_add1(dhcp_ddns_cb_t *ddns_cb, isc_result_t eresult) { isc_result_t result; - char ddns_address[ - sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")]; + char ddns_address[MAX_ADDRESS_STRING_LEN]; /* Construct a printable form of the address for logging */ strcpy(ddns_address, piaddr(ddns_cb->address)); @@ -1116,7 +1390,7 @@ ddns_fwd_srv_add1(dhcp_ddns_cb_t *ddns_cb, break; } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_cb_free(ddns_cb, MDL); /* * A single DDNS operation may require several calls depending on @@ -1167,7 +1441,7 @@ ddns_fwd_srv_connector(struct lease *lease, } if (result == ISC_R_SUCCESS) { - ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb); + ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb, MDL); } else { ddns_cb_free(ddns_cb, MDL); } @@ -1212,7 +1486,7 @@ ddns_fwd_srv_rem2(dhcp_ddns_cb_t *ddns_cb, } } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_fwd_srv_connector(NULL, NULL, NULL, ddns_cb->next_op, eresult); ddns_cb_free(ddns_cb, MDL); return; @@ -1231,8 +1505,7 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb, isc_result_t eresult) { isc_result_t result = eresult; - char ddns_address[ - sizeof("ffff:ffff:ffff:ffff:ffff:ffff:255.255.255.255")]; + char ddns_address[MAX_ADDRESS_STRING_LEN]; switch(eresult) { case ISC_R_SUCCESS: @@ -1281,7 +1554,7 @@ ddns_fwd_srv_rem1(dhcp_ddns_cb_t *ddns_cb, break; } - ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL); + ddns_update_lease_ptr(NULL, NULL, ddns_cb, NULL, MDL); ddns_fwd_srv_connector(NULL, NULL, NULL, ddns_cb->next_op, eresult); ddns_cb_free(ddns_cb, MDL); } @@ -1425,7 +1698,7 @@ ddns_removals(struct lease *lease, rcode = ddns_modify_fwd(ddns_cb); if (rcode == ISC_R_SUCCESS) { ddns_update_lease_ptr(lease, lease6, ddns_cb, - ddns_cb); + ddns_cb, MDL); return(1); } @@ -1464,7 +1737,8 @@ ddns_removals(struct lease *lease, rcode = ddns_modify_ptr(ddns_cb); if (rcode == ISC_R_SUCCESS) { - ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb); + ddns_update_lease_ptr(lease, lease6, ddns_cb, ddns_cb, + MDL); return(result); }