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

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

[Enrique Llorente]

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 <ellorent@redhat.com>
---
 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;
 }
+
-- 
@@ -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

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

[Stefano Brivio]

On Fri, 31 Jan 2025 15:53:29 +0100
Enrique Llorente <ellorent@redhat.com> wrote:

> The logic composing the DHCP reply message is reusing the request
> message to compose the it, this kind be problematic from a security

Does "be problematic" imply "would be ... once we add longer options"?

> context and may break the functionality.

Which one? This is important to know for distribution maintainers and,
ultimately, users.

As far as I know it's all fine until now, the problem would arise in
your next patch, so perhaps state that.

The real reason why we need this is that we want to have a given, fixed
size for the option field (308) so that we can easily check we don't
exceed it once we start writing the FQDN in it.

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

s/opetions/options/

> 
> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> ---
>  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

On one hand, this fits with a function that's called "fill()". On the
other hand, I would have a slight preference to keep this in dhcp()
because dhcp() is a comprehensive summary (albeit a bit long) of what
we're doing.

Is there a particular reason why you moved non-option field assignments
to here?

> + * @c:		Execution context to copy from
> + * @req:	Request message to copy from
> + * @resp:	Response Message to write to

s/Message/message/

>   *
>   * Return: current size of options field
>   */
> -static int fill(struct msg *m)
> +static int fill(const struct ctx *c, struct msg const* req,

Coding style (assuming you need to change this).

> +		     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

Coding style.

Inconsistency between detail of messages. By this metric, the one above
should also say "We reply FAST" and the one below "This is OLD".

I don't think these comments really add anything.

> +	memset(&resp->sname, 0, sizeof(resp->sname));
> +	memset(&resp->file, 0, sizeof(resp->file));
> +	resp->yiaddr	= c->ip4.addr;
> +
> +

Excess newline.

> +	/* Copy these fields from request */

Also rather obvious, plus it would be problematic along with my
suggestion below about having the fields in order.

> +	memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr));
> +	resp->htype	= req->htype;

These look like a table, the 'resp' assignments above don't.

> +	resp->hlen	= req->hlen;
> +	resp->xid	= req->xid;
> +	resp->flags	= req->flags;
> +	resp->ciaddr	= req->ciaddr;
> +	resp->siaddr	= req->siaddr; /* TODO server ip ? */

This needs to be 0. The issue is pre-existing, but as you're already
doing this...

> +	resp->giaddr	= req->giaddr;
> +	resp->magic	= req->magic;

Could we have *all* those in order? If one is familiar with the
standard (or is reading it), it's easier to follow and find things.

>  	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;

Look at the line just above.

> +	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;
>  
> +

Stray change.

>  	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));
> +

Stray extra newline.

>  
> -	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;
> -

Stray change.

> -	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;
>  }
> +

Same here.

-- 
Stefano

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

[Enrique Llorente Pastora]

On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Fri, 31 Jan 2025 15:53:29 +0100
> Enrique Llorente <ellorent@redhat.com> wrote:
>
> > The logic composing the DHCP reply message is reusing the request
> > message to compose the it, this kind be problematic from a security
>
> Does "be problematic" imply "would be ... once we add longer options"?
>
> > context and may break the functionality.
>
> Which one? This is important to know for distribution maintainers and,
> ultimately, users.
>
> As far as I know it's all fine until now, the problem would arise in
> your next patch, so perhaps state that.
>
> The real reason why we need this is that we want to have a given, fixed
> size for the option field (308) so that we can easily check we don't
> exceed it once we start writing the FQDN in it.
>
> > This change create a new reply message and fill it in with proper fields
> > from request adding on top the generated opetions.
>
> s/opetions/options/
>

Thinking about it twice, maybe we don't need this change after all,
from what I see at
code it override the request options with the reply options and then
add the 255 finalizer and some padding if
is less than 308, meaning that the request options has no effect on
reply, isn't that enough ?


> >
> > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > ---
> >  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
>
> On one hand, this fits with a function that's called "fill()". On the
> other hand, I would have a slight preference to keep this in dhcp()
> because dhcp() is a comprehensive summary (albeit a bit long) of what
> we're doing.
>
> Is there a particular reason why you moved non-option field assignments
> to here?
>
> > + * @c:               Execution context to copy from
> > + * @req:     Request message to copy from
> > + * @resp:    Response Message to write to
>
> s/Message/message/
>
> >   *
> >   * Return: current size of options field
> >   */
> > -static int fill(struct msg *m)
> > +static int fill(const struct ctx *c, struct msg const* req,
>
> Coding style (assuming you need to change this).
>
> > +                  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
>
> Coding style.
>
> Inconsistency between detail of messages. By this metric, the one above
> should also say "We reply FAST" and the one below "This is OLD".
>
> I don't think these comments really add anything.
>
> > +     memset(&resp->sname, 0, sizeof(resp->sname));
> > +     memset(&resp->file, 0, sizeof(resp->file));
> > +     resp->yiaddr    = c->ip4.addr;
> > +
> > +
>
> Excess newline.
>
> > +     /* Copy these fields from request */
>
> Also rather obvious, plus it would be problematic along with my
> suggestion below about having the fields in order.
>
> > +     memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr));
> > +     resp->htype     = req->htype;
>
> These look like a table, the 'resp' assignments above don't.
>
> > +     resp->hlen      = req->hlen;
> > +     resp->xid       = req->xid;
> > +     resp->flags     = req->flags;
> > +     resp->ciaddr    = req->ciaddr;
> > +     resp->siaddr    = req->siaddr; /* TODO server ip ? */
>
> This needs to be 0. The issue is pre-existing, but as you're already
> doing this...
>
> > +     resp->giaddr    = req->giaddr;
> > +     resp->magic     = req->magic;
>
> Could we have *all* those in order? If one is familiar with the
> standard (or is reading it), it's easier to follow and find things.
>
> >       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;
>
> Look at the line just above.
>
> > +     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;
> >
> > +
>
> Stray change.
>
> >       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));
> > +
>
> Stray extra newline.
>
> >
> > -     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;
> > -
>
> Stray change.
>
> > -     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;
> >  }
> > +
>
> Same here.
>
> --
> Stefano
>


--
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat

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

[Stefano Brivio]

On Mon, 3 Feb 2025 10:19:27 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 31 Jan 2025 15:53:29 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > The logic composing the DHCP reply message is reusing the request
> > > message to compose the it, this kind be problematic from a security  
> >
> > Does "be problematic" imply "would be ... once we add longer options"?
> >  
> > > context and may break the functionality.  
> >
> > Which one? This is important to know for distribution maintainers and,
> > ultimately, users.
> >
> > As far as I know it's all fine until now, the problem would arise in
> > your next patch, so perhaps state that.
> >
> > The real reason why we need this is that we want to have a given, fixed
> > size for the option field (308) so that we can easily check we don't
> > exceed it once we start writing the FQDN in it.
> >  
> > > This change create a new reply message and fill it in with proper fields
> > > from request adding on top the generated opetions.  
> >
> > s/opetions/options/
> 
> Thinking about it twice, maybe we don't need this change after all,
> from what I see at
> code it override the request options with the reply options and then
> add the 255 finalizer and some padding if
> is less than 308, meaning that the request options has no effect on
> reply, isn't that enough ?

It's not, because with FQDN options we can easily exceed the minimum
size of a DHCP message we get, so the original message might not be
enough to write everything we need to write.

It's about the lower bound (of the size), not the upper bound.

-- 
Stefano

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

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Sat, Feb 01, 2025 at 02:13:30PM +0100, Stefano Brivio wrote:
> On Fri, 31 Jan 2025 15:53:29 +0100
> Enrique Llorente <ellorent@redhat.com> wrote:
> 
> > The logic composing the DHCP reply message is reusing the request
> > message to compose the it, this kind be problematic from a security
> 
> Does "be problematic" imply "would be ... once we add longer options"?
> 
> > context and may break the functionality.
> 
> Which one? This is important to know for distribution maintainers and,
> ultimately, users.

Right, as a general rule commit messages be specific and concrete
about what the problem they're address is.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Enrique Llorente Pastora]

On Mon, Feb 3, 2025 at 10:29 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Sat, Feb 01, 2025 at 02:13:30PM +0100, Stefano Brivio wrote:
> > On Fri, 31 Jan 2025 15:53:29 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >
> > > The logic composing the DHCP reply message is reusing the request
> > > message to compose the it, this kind be problematic from a security
> >
> > Does "be problematic" imply "would be ... once we add longer options"?
> >
> > > context and may break the functionality.
> >
> > Which one? This is important to know for distribution maintainers and,
> > ultimately, users.
>
> Right, as a general rule commit messages be specific and concrete
> about what the problem they're address is.
>

This looks about right ?

    The logic composing the DHCP reply message is reusing the request
    message to compose it, future long options like FQDN may
    exceed the request message limit making it go beyond the lower
    bound.

    This change create a new reply message with a fixed options size of 308
    and fill it in with proper fields from requests adding on top the generated
    options, this way the reply lower bound does not depend on the request.


> --
> David Gibson (he or they)       | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
>                                 | around.
> http://www.ozlabs.org/~dgibson



-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat

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

[Enrique Llorente Pastora]

On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On Fri, 31 Jan 2025 15:53:29 +0100
> Enrique Llorente <ellorent@redhat.com> wrote:
>
> > The logic composing the DHCP reply message is reusing the request
> > message to compose the it, this kind be problematic from a security
>
> Does "be problematic" imply "would be ... once we add longer options"?
>
> > context and may break the functionality.
>
> Which one? This is important to know for distribution maintainers and,
> ultimately, users.
>
> As far as I know it's all fine until now, the problem would arise in
> your next patch, so perhaps state that.
>
> The real reason why we need this is that we want to have a given, fixed
> size for the option field (308) so that we can easily check we don't
> exceed it once we start writing the FQDN in it.
>
> > This change create a new reply message and fill it in with proper fields
> > from request adding on top the generated opetions.
>
> s/opetions/options/
>
> >
> > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > ---
> >  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
>
> On one hand, this fits with a function that's called "fill()". On the
> other hand, I would have a slight preference to keep this in dhcp()
> because dhcp() is a comprehensive summary (albeit a bit long) of what
> we're doing.
>
> Is there a particular reason why you moved non-option field assignments
> to here?

The original fill() function do set some non options fields

m->op = BOOTREPLY;
m->secs = 0;

That was my motivation to add the rest of them.

What do you think about doing the opposite and moving op and secs out
of fill next to the rest of options ?
The place would be just after parsing request with "packet_get(p, 0,
offset, offsetof(struct msg, o), &opt_len);"

Something like:

        m   = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len);
        if (!m                                          ||
            mlen  != ntohs(uh->len) - sizeof(*uh)       ||
            mlen  <  offsetof(struct msg, o)            ||
            m->op != BOOTREQUEST)
                return -1;

        reply.op        = BOOTREPLY;
        reply.htype     = m->htype;
        reply.hlen      = m->hlen;
        reply.hops      = 0;
        reply.xid       = m->xid;
        reply.secs      = 0;
        reply.flags     = m->flags;
        reply.ciaddr    = m->ciaddr;
        reply.yiaddr    = c->ip4.addr;
        reply.siaddr    = 0;
        reply.giaddr    = m->giaddr;
        memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr));
        memset(&reply.sname, 0, sizeof(reply.sname));
        memset(&reply.file, 0, sizeof(reply.file));
        reply.magic     = m->magic;


>
> > + * @c:               Execution context to copy from
> > + * @req:     Request message to copy from
> > + * @resp:    Response Message to write to
>
> s/Message/message/
>
> >   *
> >   * Return: current size of options field
> >   */
> > -static int fill(struct msg *m)
> > +static int fill(const struct ctx *c, struct msg const* req,
>
> Coding style (assuming you need to change this).
>
> > +                  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
>
> Coding style.
>
> Inconsistency between detail of messages. By this metric, the one above
> should also say "We reply FAST" and the one below "This is OLD".
>
> I don't think these comments really add anything.
>
> > +     memset(&resp->sname, 0, sizeof(resp->sname));
> > +     memset(&resp->file, 0, sizeof(resp->file));
> > +     resp->yiaddr    = c->ip4.addr;
> > +
> > +
>
> Excess newline.
>
> > +     /* Copy these fields from request */
>
> Also rather obvious, plus it would be problematic along with my
> suggestion below about having the fields in order.
>
> > +     memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr));
> > +     resp->htype     = req->htype;
>
> These look like a table, the 'resp' assignments above don't.
>
> > +     resp->hlen      = req->hlen;
> > +     resp->xid       = req->xid;
> > +     resp->flags     = req->flags;
> > +     resp->ciaddr    = req->ciaddr;
> > +     resp->siaddr    = req->siaddr; /* TODO server ip ? */
>
> This needs to be 0. The issue is pre-existing, but as you're already
> doing this...
>
> > +     resp->giaddr    = req->giaddr;
> > +     resp->magic     = req->magic;
>
> Could we have *all* those in order? If one is familiar with the
> standard (or is reading it), it's easier to follow and find things.
>
> >       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;
>
> Look at the line just above.
>
> > +     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;
> >
> > +
>
> Stray change.
>
> >       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));
> > +
>
> Stray extra newline.
>
> >
> > -     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;
> > -
>
> Stray change.
>
> > -     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;
> >  }
> > +
>
> Same here.
>
> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat

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

[Stefano Brivio]

On Mon, 3 Feb 2025 10:53:26 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Mon, Feb 3, 2025 at 10:29 AM David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > On Sat, Feb 01, 2025 at 02:13:30PM +0100, Stefano Brivio wrote:  
> > > On Fri, 31 Jan 2025 15:53:29 +0100
> > > Enrique Llorente <ellorent@redhat.com> wrote:
> > >  
> > > > The logic composing the DHCP reply message is reusing the request
> > > > message to compose the it, this kind be problematic from a security  
> > >
> > > Does "be problematic" imply "would be ... once we add longer options"?
> > >  
> > > > context and may break the functionality.  
> > >
> > > Which one? This is important to know for distribution maintainers and,
> > > ultimately, users.  
> >
> > Right, as a general rule commit messages be specific and concrete
> > about what the problem they're address is.
> 
> This looks about right ?

To me yes, just:

>     The logic composing the DHCP reply message is reusing the request
>     message to compose it, future long options like FQDN may
>     exceed the request message limit making it go beyond the lower
>     bound.
> 
>     This change create a new reply message with a fixed options size of 308

creates

>     and fill it in with proper fields from requests adding on top the generated

fills

>     options, this way the reply lower bound does not depend on the request.

-- 
Stefano

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

[Stefano Brivio]

Sorry for the delay:

On Mon, 3 Feb 2025 11:26:42 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 31 Jan 2025 15:53:29 +0100
> > Enrique Llorente <ellorent@redhat.com> wrote:
> >  
> > > The logic composing the DHCP reply message is reusing the request
> > > message to compose the it, this kind be problematic from a security  
> >
> > Does "be problematic" imply "would be ... once we add longer options"?
> >  
> > > context and may break the functionality.  
> >
> > Which one? This is important to know for distribution maintainers and,
> > ultimately, users.
> >
> > As far as I know it's all fine until now, the problem would arise in
> > your next patch, so perhaps state that.
> >
> > The real reason why we need this is that we want to have a given, fixed
> > size for the option field (308) so that we can easily check we don't
> > exceed it once we start writing the FQDN in it.
> >  
> > > This change create a new reply message and fill it in with proper fields
> > > from request adding on top the generated opetions.  
> >
> > s/opetions/options/
> >  
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > >  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  
> >
> > On one hand, this fits with a function that's called "fill()". On the
> > other hand, I would have a slight preference to keep this in dhcp()
> > because dhcp() is a comprehensive summary (albeit a bit long) of what
> > we're doing.
> >
> > Is there a particular reason why you moved non-option field assignments
> > to here?  
> 
> The original fill() function do set some non options fields
> 
> m->op = BOOTREPLY;
> m->secs = 0;
> 
> That was my motivation to add the rest of them.

Hah, funny. Sorry for that, I had no idea. It was actually not intended
on my side.

> What do you think about doing the opposite and moving op and secs out
> of fill next to the rest of options ?

Yes, definitely, it makes sense and fits the original intention behind
fill().

> The place would be just after parsing request with "packet_get(p, 0,
> offset, offsetof(struct msg, o), &opt_len);"
> 
> Something like:
> 
>         m   = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len);
>         if (!m                                          ||
>             mlen  != ntohs(uh->len) - sizeof(*uh)       ||
>             mlen  <  offsetof(struct msg, o)            ||
>             m->op != BOOTREQUEST)
>                 return -1;
> 
>         reply.op        = BOOTREPLY;
>         reply.htype     = m->htype;
>         reply.hlen      = m->hlen;
>         reply.hops      = 0;
>         reply.xid       = m->xid;
>         reply.secs      = 0;
>         reply.flags     = m->flags;
>         reply.ciaddr    = m->ciaddr;
>         reply.yiaddr    = c->ip4.addr;
>         reply.siaddr    = 0;
>         reply.giaddr    = m->giaddr;
>         memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr));
>         memset(&reply.sname, 0, sizeof(reply.sname));
>         memset(&reply.file, 0, sizeof(reply.file));
>         reply.magic     = m->magic;

Looks good to me. Maybe use tabs:
	reply.op	= BOOTREPLY;

and this would look even more readable (to me at least, with spaces
too, if it doesn't offend anybody):

	...
	reply.siaddr		= 0;
	reply.giaddr		= m->giaddr;
	memcpy(&reply.chaddr,	  m->chaddr,	sizeof(reply.chaddr));
	memset(&reply.sname,	  0,		sizeof(reply.sname));
	memset(&reply.file,	  0,		sizeof(reply.file));
	reply.magic		= m->magic;

...which is the reason why we don't have/want an automatic formatter. :)

-- 
Stefano

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

[Enrique Llorente Pastora]

On Mon, Feb 3, 2025 at 8:01 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> Sorry for the delay:
>
> On Mon, 3 Feb 2025 11:26:42 +0100
> Enrique Llorente Pastora <ellorent@redhat.com> wrote:
>
> > On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> > >
> > > On Fri, 31 Jan 2025 15:53:29 +0100
> > > Enrique Llorente <ellorent@redhat.com> wrote:
> > >
> > > > The logic composing the DHCP reply message is reusing the request
> > > > message to compose the it, this kind be problematic from a security
> > >
> > > Does "be problematic" imply "would be ... once we add longer options"?
> > >
> > > > context and may break the functionality.
> > >
> > > Which one? This is important to know for distribution maintainers and,
> > > ultimately, users.
> > >
> > > As far as I know it's all fine until now, the problem would arise in
> > > your next patch, so perhaps state that.
> > >
> > > The real reason why we need this is that we want to have a given, fixed
> > > size for the option field (308) so that we can easily check we don't
> > > exceed it once we start writing the FQDN in it.
> > >
> > > > This change create a new reply message and fill it in with proper fields
> > > > from request adding on top the generated opetions.
> > >
> > > s/opetions/options/
> > >
> > > >
> > > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > > ---
> > > >  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
> > >
> > > On one hand, this fits with a function that's called "fill()". On the
> > > other hand, I would have a slight preference to keep this in dhcp()
> > > because dhcp() is a comprehensive summary (albeit a bit long) of what
> > > we're doing.
> > >
> > > Is there a particular reason why you moved non-option field assignments
> > > to here?
> >
> > The original fill() function do set some non options fields
> >
> > m->op = BOOTREPLY;
> > m->secs = 0;
> >
> > That was my motivation to add the rest of them.
>
> Hah, funny. Sorry for that, I had no idea. It was actually not intended
> on my side.
>
> > What do you think about doing the opposite and moving op and secs out
> > of fill next to the rest of options ?
>
> Yes, definitely, it makes sense and fits the original intention behind
> fill().
>
> > The place would be just after parsing request with "packet_get(p, 0,
> > offset, offsetof(struct msg, o), &opt_len);"
> >
> > Something like:
> >
> >         m   = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len);
> >         if (!m                                          ||
> >             mlen  != ntohs(uh->len) - sizeof(*uh)       ||
> >             mlen  <  offsetof(struct msg, o)            ||
> >             m->op != BOOTREQUEST)
> >                 return -1;
> >
> >         reply.op        = BOOTREPLY;
> >         reply.htype     = m->htype;
> >         reply.hlen      = m->hlen;
> >         reply.hops      = 0;
> >         reply.xid       = m->xid;
> >         reply.secs      = 0;
> >         reply.flags     = m->flags;
> >         reply.ciaddr    = m->ciaddr;
> >         reply.yiaddr    = c->ip4.addr;
> >         reply.siaddr    = 0;
> >         reply.giaddr    = m->giaddr;
> >         memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr));
> >         memset(&reply.sname, 0, sizeof(reply.sname));
> >         memset(&reply.file, 0, sizeof(reply.file));
> >         reply.magic     = m->magic;
>
> Looks good to me. Maybe use tabs:
>         reply.op        = BOOTREPLY;
>
> and this would look even more readable (to me at least, with spaces
> too, if it doesn't offend anybody):
>
>         ...
>         reply.siaddr            = 0;
>         reply.giaddr            = m->giaddr;
>         memcpy(&reply.chaddr,     m->chaddr,    sizeof(reply.chaddr));
>         memset(&reply.sname,      0,            sizeof(reply.sname));
>         memset(&reply.file,       0,            sizeof(reply.file));
>         reply.magic             = m->magic;
>

Ok, I will "tab" so we have first column reply field, second column
values and last column sizeofs


> ...which is the reason why we don't have/want an automatic formatter. :)
>
> --
> Stefano
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat