Re: [PATCH 2/4] dhcp: Make option parsing more robust, explicitly handle options 0 and 255

[David Gibson <david@gibson.dropbear.id.au>]

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

On Thu, Jul 16, 2026 at 01:25:21AM +0200, Stefano Brivio wrote:
> The initial option-scanning loop in dhcp(), so far, ignored options 0
> (Pad Option, RFC 2132, Section 3.1) and 255 (End Option, RFC 2132,
> Section 3.2).
> 
> As a result:
> 
> - if we ever encountered option 0 in the middle of option fields
>   (never seen in practice), we would potentially terminate the loop
>   too early, before scanning remaining options
> 
> - a malformed message with an option 255 followed by a length byte
>   would (reliably) cause us to terminate as we would exceed the
>   allocated size for the 'opts' array, which is detected as buffer
>   overflow by the FORTIFY_SOURCE mechanism
> 
> The latter was reported as potential vulnerability by AISLE, but it's
> not actually a vulnerability as we always terminate without carrying
> on further handling, and in our security model the guest is able to
> sabotage its own connectivity in any case (for example, a malformed
> frame from the hypervisor would cause us to reset the connection, or
> entirely flooding the flow table would cause inbound connectivity to
> stop working, etc.).
> 
> The reported behaviour, however, is indeed a defect, as it affects
> the functional robustness to a hypothetical issue in a DHCP client,
> and that's something we definitely want to fix.
> 
> Make the option parsing loop more robust by:
> 
> - resizing 'opts' from 255 to 256 elements: there's no particular
>   reason to try to save a tiny bit of memory (which shouldn't even
>   be allocated in practice) instead of being defensive about it
> 
> - explicitly handle options 0 (skip one byte, continue) and 255 (stop
>   processing options) in the option-scanning loop
> 
> This bug was found and an initial version of the patch was written by
> the AISLE AI security scanning tool (https://aisle.com/platform).
> 
> Reported-by: AISLE
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
>  dhcp.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 1ff8cba..632019a 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -49,7 +49,7 @@ struct opt {
>  	uint8_t c[255];
>  };
>  
> -static struct opt opts[255];
> +static struct opt opts[256];
>  
>  #define DHCPDISCOVER	1
>  #define DHCPOFFER	2
> @@ -374,6 +374,14 @@ int dhcp(const struct ctx *c, struct iov_tail *data)
>  		if (!type || !olen)
>  			return -1;
>  
> +		if (*type == 255)
> +			break;
> +
> +		if (*type == 0) { /* Pad Option (RFC 2132, 3.1): one byte */
> +			opt_len--;
> +			continue;
> +		}
> +

I don't think this is quite right: at this point we've already
stripped off the non-existent length-byte with IOV_REMOVE_HEADER,
meaning opt_len will get out of sync with iov_tail_size(data).

I think we instead need to check for the 1-byte option cases between
the two IOV_REMOVE_HEADER() calls.  And.. since presumably the options
could theoretically end with some padding options, we probably want
the loop to be while (opt_len >= 1) instead of 2.  In fact the way we
mix recalculating opt_len from iov_tail_size() with sometimes directly
updating it is pretty nasty.  We might be better off with
	while ((opt_len = iov_tail_size(data)))


>  		opt_len = iov_tail_size(data);
>  		if (opt_len < *olen)
>  			return -1;
> -- 
> 2.43.0
> 

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