[PATCH] dhcp: Don't re-use request message for reply - dev

31 Jan 2025

The logic composing the DHCP reply message is reusing the request
message to compose the it, this kind be problematic from a security
context and may break the functionality.

This change create a new reply message and fill it in with proper fields
from request adding on top the generated opetions.

Signed-off-by: Enrique Llorente &lt;ellorent(a)redhat.com&gt;
---
 dhcp.c | 55 ++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 17 deletions(-)

diff --git a/dhcp.c b/dhcp.c
index d8515aa..d8ff330 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -142,17 +142,36 @@ static void fill_one(struct msg *m, int o, int *offset)
 }
 
 /**
- * fill() - Fill options in message
- * @m:		Message to fill
+ * fill() - Fill fields and options in response message
+ * @c:		Execution context to copy from
+ * @req:	Request message to copy from
+ * @resp:	Response Message to write to
  *
  * Return: current size of options field
  */
-static int fill(struct msg *m)
+static int fill(const struct ctx *c, struct msg const* req,
+		     struct msg *resp)
 {
 	int i, o, offset = 0;
 
-	m->op = BOOTREPLY;
-	m->secs = 0;
+	resp->op = BOOTREPLY;
+	resp->secs = 0;
+	resp->hops = 0; // We are not a RELAY agent
+	memset(&resp->sname, 0, sizeof(resp->sname));
+	memset(&resp->file, 0, sizeof(resp->file));
+	resp->yiaddr	= c->ip4.addr;
+
+
+	/* Copy these fields from request */
+	memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr));
+	resp->htype	= req->htype;
+	resp->hlen	= req->hlen;
+	resp->xid	= req->xid;
+	resp->flags	= req->flags;
+	resp->ciaddr	= req->ciaddr;
+	resp->siaddr	= req->siaddr; /* TODO server ip ? */
+	resp->giaddr	= req->giaddr;
+	resp->magic	= req->magic;
 
 	for (o = 0; o < 255; o++)
 		opts[o].sent = 0;
@@ -162,24 +181,24 @@ static int fill(struct msg *m)
 	 * Put it there explicitly, unless requested via option 55.
 	 */
 	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
-		fill_one(m, 53, &offset);
+		fill_one(resp, 53, &offset);
 
 	for (i = 0; i < opts[55].clen; i++) {
 		o = opts[55].c[i];
 		if (opts[o].slen != -1)
-			fill_one(m, o, &offset);
+			fill_one(resp, o, &offset);
 	}
 
 	for (o = 0; o < 255; o++) {
 		if (opts[o].slen != -1 && !opts[o].sent)
-			fill_one(m, o, &offset);
+			fill_one(resp, o, &offset);
 	}
 
-	m->o[offset++] = 255;
-	m->o[offset++] = 0;
+	resp->o[offset++] = 255;
+	resp->o[offset++] = 0;
 
 	if (offset < OPT_MIN) {
-		memset(&m->o[offset], 0, OPT_MIN - offset);
+		memset(&resp->o[offset], 0, OPT_MIN - offset);
 		offset = OPT_MIN;
 	}
 
@@ -291,8 +310,9 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	const struct ethhdr *eh;
 	const struct iphdr *iph;
 	const struct udphdr *uh;
+	struct msg const *m;
+	struct msg resp;
 	unsigned int i;
-	struct msg *m;
 
 	eh  = packet_get(p, 0, offset, sizeof(*eh),  NULL);
 	offset += sizeof(*eh);
@@ -321,6 +341,7 @@ int dhcp(const struct ctx *c, const struct pool *p)
 	    m->op != BOOTREQUEST)
 		return -1;
 
+
 	offset += offsetof(struct msg, o);
 
 	for (i = 0; i < ARRAY_SIZE(opts); i++)
@@ -364,7 +385,6 @@ int dhcp(const struct ctx *c, const struct pool *p)
 
 	info("    from %s", eth_ntop(m->chaddr, macstr, sizeof(macstr)));
 
-	m->yiaddr = c->ip4.addr;
 	mask.s_addr = htonl(0xffffffff << (32 - c->ip4.prefix_len));
 	memcpy(opts[1].s,  &mask,                sizeof(mask));
 	memcpy(opts[3].s,  &c->ip4.guest_gw,     sizeof(c->ip4.guest_gw));
@@ -399,16 +419,17 @@ int dhcp(const struct ctx *c, const struct pool *p)
 		opts[6].slen = -1;
 
 	if (!c->no_dhcp_dns_search)
-		opt_set_dns_search(c, sizeof(m->o));
+		opt_set_dns_search(c, sizeof(resp.o));
+
 
-	dlen = offsetof(struct msg, o) + fill(m);
+	dlen = offsetof(struct msg, o) + fill(c, m, &resp);
 
 	if (m->flags & FLAG_BROADCAST)
 		dst = in4addr_broadcast;
 	else
 		dst = c->ip4.addr;
-
-	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen);
+	tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, &resp, dlen);
 
 	return 1;
 }
+
-- 
2.47.0


    