svn commit: r318630 - stable/11/sbin/dhclient

[Nick Hibma]

Author: n_hibma
Date: Mon May 22 10:28:17 2017
New Revision: 318630
URL: https://svnweb.freebsd.org/changeset/base/318630

Log:
  MFC:
  
  	------------------------------------------------------------------------
  	r317923 | n_hibma | 2017-05-07 23:11:28 +0200 (Sun, 07 May 2017) | 8 lines
  
  	Fix the output of very large rebind, renew and lease time options in
  	lease file.
  
  	Some routers set very large values for rebind time (Netgear) and these
  	are erroneously reported as negative in the leasefile. This was due to a
  	wrong printf format specification of %ld for an unsigned long on 32-bit
  	platforms.
  
  	------------------------------------------------------------------------
  	r317915 | n_hibma | 2017-05-07 21:59:37 +0200 (Sun, 07 May 2017) | 16 lines
  
  	Fix handling of large DHCP expiry values.
  
  	They would overflow a signed 32-bit time_t on 32 bit architectures. This
  	was taken care of, but a compiler optimisation makes this behave
  	erratically. This could be resolved by adding a -fwrapv flag, but
  	instead we can check the value before adding the current timestamp to
  	it.
  
  	In the lease file values are still wrong though:
  
  		option dhcp-rebinding-time -644245096;
  
  PR:		218980

Modified:
  stable/11/sbin/dhclient/dhclient.c
  stable/11/sbin/dhclient/options.c

Modified: stable/11/sbin/dhclient/dhclient.c
==============================================================================
--- stable/11/sbin/dhclient/dhclient.c	Mon May 22 08:20:50 2017	(r318629)
+++ stable/11/sbin/dhclient/dhclient.c	Mon May 22 10:28:17 2017	(r318630)
@@ -107,7 +107,11 @@ struct pidfh *pidfile;
  */
 #define ASSERT_STATE(state_is, state_shouldbe) {}
 
-#define TIME_MAX 2147483647
+/*
+ * We need to check that the expiry, renewal and rebind times are not beyond
+ * the end of time (~2038 when a 32-bit time_t is being used).
+ */
+#define TIME_MAX        ((((time_t) 1 << (sizeof(time_t) * CHAR_BIT - 2)) - 1) * 2 + 1)
 
 int		log_priority;
 int		no_daemon;
@@ -762,15 +766,17 @@ dhcpack(struct packet *packet)
 	else
 		ip->client->new->expiry = default_lease_time;
 	/* A number that looks negative here is really just very large,
-	   because the lease expiry offset is unsigned. */
-	if (ip->client->new->expiry < 0)
-		ip->client->new->expiry = TIME_MAX;
+	   because the lease expiry offset is unsigned. Also make sure that
+           the addition of cur_time below does not overflow (a 32 bit) time_t. */
+	if (ip->client->new->expiry < 0 ||
+            ip->client->new->expiry > TIME_MAX - cur_time)
+		ip->client->new->expiry = TIME_MAX - cur_time;
 	/* XXX should be fixed by resetting the client state */
 	if (ip->client->new->expiry < 60)
 		ip->client->new->expiry = 60;
 
         /* Unless overridden in the config, take the server-provided renewal
-         * time if there is one; otherwise figure it out according to the spec.
+         * time if there is one. Otherwise figure it out according to the spec.
          * Also make sure the renewal time does not exceed the expiry time.
          */
         if (ip->client->config->default_actions[DHO_DHCP_RENEWAL_TIME] ==
@@ -782,7 +788,8 @@ dhcpack(struct packet *packet)
 		    ip->client->new->options[DHO_DHCP_RENEWAL_TIME].data);
 	else
 		ip->client->new->renewal = ip->client->new->expiry / 2;
-        if (ip->client->new->renewal > ip->client->new->expiry / 2)
+        if (ip->client->new->renewal < 0 ||
+            ip->client->new->renewal > ip->client->new->expiry / 2)
                 ip->client->new->renewal = ip->client->new->expiry / 2;
 
 	/* Same deal with the rebind time. */
@@ -794,20 +801,15 @@ dhcpack(struct packet *packet)
 		ip->client->new->rebind = getULong(
 		    ip->client->new->options[DHO_DHCP_REBINDING_TIME].data);
 	else
-		ip->client->new->rebind = ip->client->new->renewal * 7 / 4;
-        if (ip->client->new->rebind > ip->client->new->renewal * 7 / 4)
-                ip->client->new->rebind = ip->client->new->renewal * 7 / 4;
-
-	ip->client->new->expiry += cur_time;
-	/* Lease lengths can never be negative. */
-	if (ip->client->new->expiry < cur_time)
-		ip->client->new->expiry = TIME_MAX;
-	ip->client->new->renewal += cur_time;
-	if (ip->client->new->renewal < cur_time)
-		ip->client->new->renewal = TIME_MAX;
-	ip->client->new->rebind += cur_time;
-	if (ip->client->new->rebind < cur_time)
-		ip->client->new->rebind = TIME_MAX;
+		ip->client->new->rebind = ip->client->new->renewal / 4 * 7;
+	if (ip->client->new->rebind < 0 ||
+            ip->client->new->rebind > ip->client->new->renewal / 4 * 7)
+                ip->client->new->rebind = ip->client->new->renewal / 4 * 7;
+
+        /* Convert the time offsets into seconds-since-the-epoch */
+        ip->client->new->expiry += cur_time;
+        ip->client->new->renewal += cur_time;
+        ip->client->new->rebind += cur_time;
 
 	bind_lease(ip);
 }

Modified: stable/11/sbin/dhclient/options.c
==============================================================================
--- stable/11/sbin/dhclient/options.c	Mon May 22 08:20:50 2017	(r318629)
+++ stable/11/sbin/dhclient/options.c	Mon May 22 10:28:17 2017	(r318630)
@@ -783,7 +783,7 @@ pretty_print_option(unsigned int code, u
 				dp += 4;
 				break;
 			case 'L':
-				opcount = snprintf(op, opleft, "%ld",
+				opcount = snprintf(op, opleft, "%lu",
 				    (unsigned long)getULong(dp));
 				if (opcount >= opleft || opcount == -1)
 					goto toobig;
@@ -799,7 +799,7 @@ pretty_print_option(unsigned int code, u
 				dp += 2;
 				break;
 			case 'S':
-				opcount = snprintf(op, opleft, "%d",
+				opcount = snprintf(op, opleft, "%u",
 				    getUShort(dp));
 				if (opcount >= opleft || opcount == -1)
 					goto toobig;