From: florian <florian@openbsd.org>
Date: Tue, 13 Feb 2024 12:53:05 +0000 (+0000)
Subject: Only generate a new xid at the start of getting a new lease.
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=ee9c6200f9dcdfe6c777f48c0a27d9aa918683e0;p=openbsd

Only generate a new xid at the start of getting a new lease.

"RFC 2131 4.1 Constructing and sending DHCP messages" has this:

| Selecting a new 'xid' for each retransmission is an implementation
| decision.  A client may choose to reuse the same 'xid' or select a new
| 'xid' for each retransmitted message.

We used to change xid for each request / response cycle but this ran
into problems with slow dhcp servers where we would change the xid too
frequently and would ignore late coming replies from the server.

Andre S points out that table 5 in "4.4.1 Initialization and
allocation of network address" says for the xid field in "DHCPREQUEST"
messages:

| 'xid' from server DHCPOFFER message

This seems to suggest that we need to use the same xid for the whole
DHCPDISCOVER / DHCPOFFER / DHCPREQUEST / DHCPACK exchange of messages.

Nothing else in the RFC is saying this though.

But since there are DHCP servers out there that depend on this, we
only generate a new xid when entering the INIT, REBOOTING and RENEWING
state.

I do wonder if we should just go with a static value of 0x04, which
was chosen by a fair dice roll, so guaranteed to be random.

Issue reported, initial diff and fix tested by Andre S
deraadt likes this version
OK tb
---

diff --git a/sbin/dhcpleased/engine.c b/sbin/dhcpleased/engine.c
index 6d371a5a112..84d02cc12ee 100644
--- a/sbin/dhcpleased/engine.c
+++ b/sbin/dhcpleased/engine.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: engine.c,v 1.42 2024/01/26 21:14:08 jan Exp $	*/
+/*	$OpenBSD: engine.c,v 1.43 2024/02/13 12:53:05 florian Exp $	*/
 
 /*
  * Copyright (c) 2017, 2021 Florian Obser <florian@openbsd.org>
@@ -1385,8 +1385,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 	char		 ifnamebuf[IF_NAMESIZE], *if_name;
 
 	iface->state = new_state;
-	if (new_state != old_state)
-		iface->xid = arc4random();
 
 	switch (new_state) {
 	case IF_DOWN:
@@ -1426,6 +1424,7 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 		case IF_DOWN:
 		case IF_IPV6_ONLY:
 			iface->timo.tv_sec = START_EXP_BACKOFF;
+			iface->xid = arc4random();
 			break;
 		case IF_BOUND:
 			fatal("invalid transition Bound -> Init");
@@ -1436,8 +1435,10 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 	case IF_REBOOTING:
 		if (old_state == IF_REBOOTING)
 			iface->timo.tv_sec *= 2;
-		else
+		else {
 			iface->timo.tv_sec = START_EXP_BACKOFF;
+			iface->xid = arc4random();
+		}
 		request_dhcp_request(iface);
 		break;
 	case IF_REQUESTING:
@@ -1458,6 +1459,7 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state)
 		if (old_state == IF_BOUND) {
 			iface->timo.tv_sec = (iface->rebinding_time -
 			    iface->renewal_time) / 2; /* RFC 2131 4.4.5 */
+			iface->xid = arc4random();
 		} else
 			iface->timo.tv_sec /= 2;