diff --git a/RELNOTES b/RELNOTES index 265f8329..01d8772f 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 8f1db46b..5ecae672 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 fb2ca0fb..e8eac799 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 389f03ee..f5240117 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 258e37b5..b78b5371 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 cfbdd04e..99c08714 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); }