Try to ensure that the various lease timeouts are sane. i.e., renew

is before rebind is before expiry. Don't allow lease expiry to be
set past the end of time, but do allow lease lengths >INT32_MAX.

Tweak default times to be more reliably in-line with RFCs.

Nuke getULong(), the last function in convert.c, since its last
uses were in this now re-written code.

Suspicions aroused while diagnosing the expiry problem weerd@ found.

1 files changed, 49 insertions, 40 deletions

diff --git a/sbin/dhclient/dhclient.c b/sbin/dhclient/dhclient.c
index 4e1d483eb4b..8d4ec4c24db 100644
--- a/sbin/dhclient/dhclient.c
+++ b/sbin/dhclient/dhclient.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dhclient.c,v 1.324 2014/10/29 15:28:51 krw Exp $	*/
+/*	$OpenBSD: dhclient.c,v 1.325 2014/11/01 15:49:07 krw Exp $	*/
 
 /*
  * Copyright 2004 Henning Brauer <henning@openbsd.org>
@@ -65,9 +65,6 @@
 #include <resolv.h>
 #include <stdint.h>
 
-#define DEFAULT_LEASE_TIME	43200	/* 12 hours. */
-#define TIME_MAX		2147483647
-
 char *path_dhclient_conf = _PATH_DHCLIENT_CONF;
 char *path_dhclient_db = NULL;
 
@@ -2523,49 +2520,61 @@ compare_lease(struct client_lease *active, struct client_lease *new)
 void
 set_lease_times(struct client_lease *lease)
 {
-	time_t cur_time;
+	time_t cur_time, time_max;
+	u_int32_t uint32val;
+
+	time(&cur_time);
+
+	time_max = LLONG_MAX - cur_time;
+	if (time_max > UINT32_MAX)
+		time_max = UINT32_MAX;
 
 	/*
 	 * Take the server-provided times if available.  Otherwise
 	 * figure them out according to the spec.
+	 *
+	 * expiry  == time to discard lease.
+	 * renewal == time to renew lease from server that provided it.
+	 * rebind  == time to renew lease from any server.
+	 *
+	 * 0 <= renewal <= rebind <= expiry <= time_max
+	 * &&
+	 * expiry >= MIN(time_max, 60)
 	 */
-	if (lease->options[DHO_DHCP_LEASE_TIME].len == 4)
-		lease->expiry =
-		    getULong(lease->options[DHO_DHCP_LEASE_TIME].data);
-	else
-		lease->expiry = DEFAULT_LEASE_TIME;
-	if (lease->options[DHO_DHCP_RENEWAL_TIME].len == 4)
-		lease->renewal =
-		    getULong(lease->options[DHO_DHCP_RENEWAL_TIME].data);
-	else
-		lease->renewal = lease->expiry / 2;
-	if (lease->options[DHO_DHCP_REBINDING_TIME].len == 4)
-		lease->rebind =
-		    getULong(lease->options[DHO_DHCP_REBINDING_TIME].data);
-	else
-		lease->rebind = lease->renewal + lease->renewal / 2 +
-		    lease->renewal / 4;
-
-	/*
-	 * A number that looks negative here is really just very large,
-	 * because the lease expiry offset is unsigned.
-	 */
-	if (lease->expiry < 0)
-		lease->expiry = TIME_MAX;
-	/* XXX should be fixed by resetting the client state */
-	if (lease->expiry < 60)
-		lease->expiry = 60;
-
-	time(&cur_time);
 
-	/* Lease lengths can never be negative. */
+	lease->expiry = 43200;	/* Default to 12 hours */
+	if (lease->options[DHO_DHCP_LEASE_TIME].len == sizeof(uint32val)) {
+		memcpy(&uint32val, lease->options[DHO_DHCP_LEASE_TIME].data,
+		    sizeof(uint32val));
+		lease->expiry = ntohl(uint32val);
+		if (lease->expiry < 60)
+			lease->expiry = 60;
+	}
+	if (lease->expiry > time_max)
+		lease->expiry = time_max;
+
+	lease->renewal = lease->expiry / 2;
+	if (lease->options[DHO_DHCP_RENEWAL_TIME].len == sizeof(uint32val)) {
+		memcpy(&uint32val, lease->options[DHO_DHCP_RENEWAL_TIME].data,
+		    sizeof(uint32val));
+		lease->renewal = ntohl(uint32val);
+		if (lease->renewal > lease->expiry)
+			lease->renewal = lease->expiry;
+	}
+
+	lease->rebind = (lease->expiry * 7) / 8;
+	if (lease->options[DHO_DHCP_REBINDING_TIME].len == sizeof(uint32val)) {
+		memcpy(&uint32val, lease->options[DHO_DHCP_REBINDING_TIME].data,
+		    sizeof(uint32val));
+		lease->rebind = ntohl(uint32val);
+		if (lease->rebind > lease->expiry)
+			lease->rebind = lease->expiry;
+	}
+	if (lease->rebind < lease->renewal)
+		lease->rebind = lease->renewal;
+
+	/* Convert lease lengths to times. */
 	lease->expiry += cur_time;
-	if (lease->expiry < cur_time)
-		lease->expiry = TIME_MAX;
 	lease->renewal += cur_time;
-	if (lease->renewal < cur_time)
-		lease->renewal = TIME_MAX;
 	lease->rebind += cur_time;
-	if (lease->rebind < cur_time)
-		lease->rebind = TIME_MAX;
 }