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

[Stefano Brivio <sbrivio@redhat.com>]

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