From: florian Date: Thu, 9 Dec 2021 16:20:12 +0000 (+0000) Subject: Rework in which state to add and not add the server-ip and X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=82a5f3d74c6ab52e9b2d46960728126c6ed84024;p=openbsd Rework in which state to add and not add the server-ip and requested-ip option as well as setting ciaddr. This started with joel@ pointing out that their CPE is ignoring RENEWING and REBINDING requests when ciaddr was not set. RFC 2131 4.3.6, Table 4 has a good overview, we got a bunch of it wrong. Previously the logic for this was all over the place which made it difficult to reason about, it is now contained in the engine process in request_dhcp_request() and request_dhcp_discover(). Problem pointed out by, lots of testing and review as well as OK joel@ Additional testing and 50% review benno@ --- diff --git a/sbin/dhcpleased/dhcpleased.h b/sbin/dhcpleased/dhcpleased.h index b3b4938ad3c..02121c19841 100644 --- a/sbin/dhcpleased/dhcpleased.h +++ b/sbin/dhcpleased/dhcpleased.h @@ -1,4 +1,4 @@ -/* $OpenBSD: dhcpleased.h,v 1.11 2021/08/12 12:41:08 florian Exp $ */ +/* $OpenBSD: dhcpleased.h,v 1.12 2021/12/09 16:20:12 florian Exp $ */ /* * Copyright (c) 2017, 2021 Florian Obser @@ -287,15 +287,10 @@ struct imsg_dhcp { uint8_t packet[1500]; }; - -struct imsg_req_discover { - uint32_t if_index; - uint32_t xid; -}; - -struct imsg_req_request { +struct imsg_req_dhcp { uint32_t if_index; uint32_t xid; + struct in_addr ciaddr; struct in_addr requested_ip; struct in_addr server_identifier; struct in_addr dhcp_server; diff --git a/sbin/dhcpleased/engine.c b/sbin/dhcpleased/engine.c index 17e65fbe789..09867c595b4 100644 --- a/sbin/dhcpleased/engine.c +++ b/sbin/dhcpleased/engine.c @@ -1,4 +1,4 @@ -/* $OpenBSD: engine.c,v 1.29 2021/11/14 18:13:19 florian Exp $ */ +/* $OpenBSD: engine.c,v 1.30 2021/12/09 16:20:12 florian Exp $ */ /* * Copyright (c) 2017, 2021 Florian Obser @@ -1170,6 +1170,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp) return; } iface->server_identifier.s_addr = server_identifier.s_addr; + iface->dhcp_server.s_addr = server_identifier.s_addr; iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr; state_transition(iface, IF_REQUESTING); break; @@ -1228,6 +1229,7 @@ parse_dhcp(struct dhcpleased_iface *iface, struct imsg_dhcp *dhcp) clock_gettime(CLOCK_MONOTONIC, &iface->request_time); iface->server_identifier.s_addr = server_identifier.s_addr; + iface->dhcp_server.s_addr = server_identifier.s_addr; iface->requested_ip.s_addr = dhcp_hdr->yiaddr.s_addr; iface->mask.s_addr = subnet_mask.s_addr; #ifndef SMALL @@ -1354,11 +1356,8 @@ 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 { - /* make sure we send broadcast */ - iface->dhcp_server.s_addr = INADDR_ANY; + else iface->timo.tv_sec = START_EXP_BACKOFF; - } request_dhcp_request(iface); break; case IF_REQUESTING: @@ -1377,10 +1376,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state) break; case IF_RENEWING: if (old_state == IF_BOUND) { - iface->dhcp_server.s_addr = - iface->server_identifier.s_addr; - iface->server_identifier.s_addr = INADDR_ANY; - iface->timo.tv_sec = (iface->rebinding_time - iface->renewal_time) / 2; /* RFC 2131 4.4.5 */ } else @@ -1392,7 +1387,6 @@ state_transition(struct dhcpleased_iface *iface, enum if_state new_state) break; case IF_REBINDING: if (old_state == IF_RENEWING) { - iface->dhcp_server.s_addr = INADDR_ANY; iface->timo.tv_sec = (iface->lease_time - iface->rebinding_time) / 2; /* RFC 2131 4.4.5 */ } else @@ -1470,28 +1464,90 @@ iface_timeout(int fd, short events, void *arg) void request_dhcp_discover(struct dhcpleased_iface *iface) { - struct imsg_req_discover imsg_req_discover; + struct imsg_req_dhcp imsg; - imsg_req_discover.if_index = iface->if_index; - imsg_req_discover.xid = iface->xid = arc4random(); - engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg_req_discover, - sizeof(imsg_req_discover)); + memset(&imsg, 0, sizeof(imsg)); + + iface->xid = arc4random(); + imsg.if_index = iface->if_index; + imsg.xid = iface->xid; + + /* + * similar to RFC 2131 4.3.6, Table 4 for DHCPDISCOVER + * ------------------------------ + * | | INIT | + * ------------------------------ + * |broad/unicast | broadcast | + * |server-ip | MUST NOT | + * |requested-ip | MAY | + * |ciaddr | zero | + * ------------------------------ + * + * Leaving everything at 0 from the memset results in this table with + * requested-ip not set. + */ + + engine_imsg_compose_frontend(IMSG_SEND_DISCOVER, 0, &imsg, sizeof(imsg)); } void request_dhcp_request(struct dhcpleased_iface *iface) { - struct imsg_req_request imsg_req_request; + struct imsg_req_dhcp imsg; iface->xid = arc4random(); - imsg_req_request.if_index = iface->if_index; - imsg_req_request.xid = iface->xid; - imsg_req_request.server_identifier.s_addr = - iface->server_identifier.s_addr; - imsg_req_request.requested_ip.s_addr = iface->requested_ip.s_addr; - imsg_req_request.dhcp_server.s_addr = iface->dhcp_server.s_addr; - engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg_req_request, - sizeof(imsg_req_request)); + imsg.if_index = iface->if_index; + imsg.xid = iface->xid; + + /* + * RFC 2131 4.3.6, Table 4 + * --------------------------------------------------------------------- + * | |REBOOTING |REQUESTING |RENEWING |REBINDING | + * --------------------------------------------------------------------- + * |broad/unicast |broadcast |broadcast |unicast |broadcast | + * |server-ip |MUST NOT |MUST |MUST NOT |MUST NOT | + * |requested-ip |MUST |MUST |MUST NOT |MUST NOT | + * |ciaddr |zero |zero |IP address |IP address| + * --------------------------------------------------------------------- + */ + switch (iface->state) { + case IF_DOWN: + fatalx("invalid state IF_DOWN in %s", __func__); + break; + case IF_INIT: + fatalx("invalid state IF_INIT in %s", __func__); + break; + case IF_BOUND: + fatalx("invalid state IF_BOUND in %s", __func__); + break; + case IF_REBOOTING: + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast */ + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */ + imsg.requested_ip = iface->requested_ip; /* MUST */ + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */ + break; + case IF_REQUESTING: + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast */ + imsg.server_identifier = + iface->server_identifier; /* MUST */ + imsg.requested_ip = iface->requested_ip; /* MUST */ + imsg.ciaddr.s_addr = INADDR_ANY; /* zero */ + break; + case IF_RENEWING: + imsg.dhcp_server = iface->dhcp_server; /* unicast */ + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */ + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT */ + imsg.ciaddr = iface->requested_ip; /* IP address */ + break; + case IF_REBINDING: + imsg.dhcp_server.s_addr = INADDR_ANY; /* broadcast */ + imsg.server_identifier.s_addr = INADDR_ANY; /* MUST NOT */ + imsg.requested_ip.s_addr = INADDR_ANY; /* MUST NOT */ + imsg.ciaddr = iface->requested_ip; /* IP address */ + break; + } + + engine_imsg_compose_frontend(IMSG_SEND_REQUEST, 0, &imsg, sizeof(imsg)); } void diff --git a/sbin/dhcpleased/frontend.c b/sbin/dhcpleased/frontend.c index bd1e37fe952..57e60c57ee2 100644 --- a/sbin/dhcpleased/frontend.c +++ b/sbin/dhcpleased/frontend.c @@ -1,4 +1,4 @@ -/* $OpenBSD: frontend.c,v 1.24 2021/11/20 17:54:40 florian Exp $ */ +/* $OpenBSD: frontend.c,v 1.25 2021/12/09 16:20:12 florian Exp $ */ /* * Copyright (c) 2017, 2021 Florian Obser @@ -70,6 +70,7 @@ struct iface { struct imsg_ifinfo ifinfo; int send_discover; uint32_t xid; + struct in_addr ciaddr; struct in_addr requested_ip; struct in_addr server_identifier; struct in_addr dhcp_server; @@ -90,10 +91,10 @@ int get_xflags(char *); struct iface *get_iface_by_id(uint32_t); void remove_iface(uint32_t); void set_bpfsock(int, uint32_t); +void iface_data_from_imsg(struct iface*, struct imsg_req_dhcp *); ssize_t build_packet(uint8_t, char *, uint32_t, struct ether_addr *, - struct in_addr *, struct in_addr *); -void send_discover(struct iface *); -void send_request(struct iface *); + struct in_addr *, struct in_addr *, struct in_addr *); +void send_packet(uint8_t, struct iface *); void bpf_send_packet(struct iface *, uint8_t *, ssize_t); int udp_send_packet(struct iface *, uint8_t *, ssize_t); #ifndef SMALL @@ -480,39 +481,39 @@ frontend_dispatch_engine(int fd, short event, void *bula) break; #endif /* SMALL */ case IMSG_SEND_DISCOVER: { - struct imsg_req_discover imsg_req_discover; - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_discover)) + struct imsg_req_dhcp imsg_req_dhcp; + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp)) fatalx("%s: IMSG_SEND_DISCOVER wrong " "length: %lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&imsg_req_discover, imsg.data, - sizeof(imsg_req_discover)); - iface = get_iface_by_id(imsg_req_discover.if_index); - if (iface != NULL) { - iface->xid = imsg_req_discover.xid; - send_discover(iface); - } + memcpy(&imsg_req_dhcp, imsg.data, + sizeof(imsg_req_dhcp)); + + iface = get_iface_by_id(imsg_req_dhcp.if_index); + + if (iface == NULL) + break; + + iface_data_from_imsg(iface, &imsg_req_dhcp); + send_packet(DHCPDISCOVER, iface); break; } case IMSG_SEND_REQUEST: { - struct imsg_req_request imsg_req_request; - if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_request)) + struct imsg_req_dhcp imsg_req_dhcp; + if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp)) fatalx("%s: IMSG_SEND_REQUEST wrong " "length: %lu", __func__, IMSG_DATA_SIZE(imsg)); - memcpy(&imsg_req_request, imsg.data, - sizeof(imsg_req_request)); - iface = get_iface_by_id(imsg_req_request.if_index); - if (iface != NULL) { - iface->xid = imsg_req_request.xid; - iface->requested_ip.s_addr = - imsg_req_request.requested_ip.s_addr; - iface->server_identifier.s_addr = - imsg_req_request.server_identifier.s_addr; - iface->dhcp_server.s_addr = - imsg_req_request.dhcp_server.s_addr; - send_request(iface); - } + memcpy(&imsg_req_dhcp, imsg.data, + sizeof(imsg_req_dhcp)); + + iface = get_iface_by_id(imsg_req_dhcp.if_index); + + if (iface == NULL) + break; + + iface_data_from_imsg(iface, &imsg_req_dhcp); + send_packet(DHCPREQUEST, iface); break; } default: @@ -885,10 +886,20 @@ bpf_receive(int fd, short events, void *arg) } } +void +iface_data_from_imsg(struct iface* iface, struct imsg_req_dhcp *imsg) +{ + iface->xid = imsg->xid; + iface->ciaddr.s_addr = imsg->ciaddr.s_addr; + iface->requested_ip.s_addr = imsg->requested_ip.s_addr; + iface->server_identifier.s_addr = imsg->server_identifier.s_addr; + iface->dhcp_server.s_addr = imsg->dhcp_server.s_addr; +} + ssize_t build_packet(uint8_t message_type, char *if_name, uint32_t xid, - struct ether_addr *hw_address, struct in_addr *requested_ip, - struct in_addr *server_identifier) + struct ether_addr *hw_address, struct in_addr *ciaddr, struct in_addr + *requested_ip, struct in_addr *server_identifier) { static uint8_t dhcp_cookie[] = DHCP_COOKIE; static uint8_t dhcp_message_type[] = {DHO_DHCP_MESSAGE_TYPE, 1, @@ -926,6 +937,7 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid, hdr->hops = 0; hdr->xid = xid; hdr->secs = 0; + hdr->ciaddr.s_addr = ciaddr->s_addr; memcpy(hdr->chaddr, hw_address, sizeof(*hw_address)); p += sizeof(struct dhcp_hdr); memcpy(p, dhcp_cookie, sizeof(dhcp_cookie)); @@ -966,20 +978,20 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid, memcpy(p, dhcp_req_list, sizeof(dhcp_req_list)); p += sizeof(dhcp_req_list); - if (message_type == DHCPREQUEST) { + if (requested_ip->s_addr != INADDR_ANY) { memcpy(dhcp_requested_address + 2, requested_ip, sizeof(*requested_ip)); memcpy(p, dhcp_requested_address, sizeof(dhcp_requested_address)); p += sizeof(dhcp_requested_address); + } - if (server_identifier->s_addr != INADDR_ANY) { - memcpy(dhcp_server_identifier + 2, server_identifier, - sizeof(*server_identifier)); - memcpy(p, dhcp_server_identifier, - sizeof(dhcp_server_identifier)); - p += sizeof(dhcp_server_identifier); - } + if (server_identifier->s_addr != INADDR_ANY) { + memcpy(dhcp_server_identifier + 2, server_identifier, + sizeof(*server_identifier)); + memcpy(p, dhcp_server_identifier, + sizeof(dhcp_server_identifier)); + p += sizeof(dhcp_server_identifier); } *p = DHO_END; @@ -995,7 +1007,7 @@ build_packet(uint8_t message_type, char *if_name, uint32_t xid, } void -send_discover(struct iface *iface) +send_packet(uint8_t message_type, struct iface *iface) { ssize_t pkt_len; char ifnamebuf[IF_NAMESIZE], *if_name; @@ -1004,27 +1016,17 @@ send_discover(struct iface *iface) iface->send_discover = 1; return; } - iface->send_discover = 0; - - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf); - log_debug("DHCPDISCOVER on %s", if_name == NULL ? "?" : if_name); - pkt_len = build_packet(DHCPDISCOVER, if_name, iface->xid, - &iface->ifinfo.hw_address, &iface->requested_ip, NULL); - bpf_send_packet(iface, dhcp_packet, pkt_len); -} + iface->send_discover = 0; -void -send_request(struct iface *iface) -{ - ssize_t pkt_len; - char ifnamebuf[IF_NAMESIZE], *if_name; + if ((if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf)) == NULL) + return; /* iface went away, nothing to do */ - if_name = if_indextoname(iface->ifinfo.if_index, ifnamebuf); - log_debug("DHCPREQUEST on %s", if_name == NULL ? "?" : if_name); + log_debug("%s on %s", message_type == DHCPDISCOVER ? "DHCPDISCOVER" : + "DHCPREQUEST", if_name); - pkt_len = build_packet(DHCPREQUEST, if_name, iface->xid, - &iface->ifinfo.hw_address, &iface->requested_ip, + pkt_len = build_packet(message_type, if_name, iface->xid, + &iface->ifinfo.hw_address, &iface->ciaddr, &iface->requested_ip, &iface->server_identifier); if (iface->dhcp_server.s_addr != INADDR_ANY) { if (udp_send_packet(iface, dhcp_packet, pkt_len) == -1) @@ -1162,7 +1164,7 @@ set_bpfsock(int bpfsock, uint32_t if_index) EV_PERSIST, bpf_receive, iface); event_add(&iface->bpfev.ev, NULL); if (iface->send_discover) - send_discover(iface); + send_packet(DHCPDISCOVER, iface); } }