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.

3 files changed, 51 insertions, 56 deletions

diff --git a/sbin/dhclient/convert.c b/sbin/dhclient/convert.c
index 14ba6f8bb9a..c78dd2fb335 100644
--- a/sbin/dhclient/convert.c
+++ b/sbin/dhclient/convert.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: convert.c,v 1.9 2014/10/29 15:28:51 krw Exp $	*/
+/*	$OpenBSD: convert.c,v 1.10 2014/11/01 15:49:07 krw Exp $	*/
 
 /*
  * Safe copying of option values into and out of the option buffer,
@@ -42,14 +42,3 @@
  * see ``http://www.vix.com/isc''.  To learn more about Vixie
  * Enterprises, see ``http://www.vix.com''.
  */
-
-#include "dhcpd.h"
-
-u_int32_t
-getULong(unsigned char *buf)
-{
-	u_int32_t ibuf;
-
-	memcpy(&ibuf, buf, sizeof(ibuf));
-	return (ntohl(ibuf));
-}
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;
 }
diff --git a/sbin/dhclient/dhcpd.h b/sbin/dhclient/dhcpd.h
index 28b00d218bc..11dfd149dc2 100644
--- a/sbin/dhclient/dhcpd.h
+++ b/sbin/dhclient/dhcpd.h
@@ -1,4 +1,4 @@
-/*	$OpenBSD: dhcpd.h,v 1.141 2014/10/29 15:28:51 krw Exp $	*/
+/*	$OpenBSD: dhcpd.h,v 1.142 2014/11/01 15:49:07 krw Exp $	*/
 
 /*
  * Copyright (c) 2004 Henning Brauer <henning@openbsd.org>
@@ -265,9 +265,6 @@ int subnet_exists(struct client_lease *);
 /* tables.c */
 extern const struct option dhcp_options[256];
 
-/* convert.c */
-u_int32_t getULong(unsigned char *);
-
 /* dhclient.c */
 extern char *path_dhclient_conf;
 extern char *path_dhclient_db;