Re: [libnftnl PATCH 1/3] set: Fix for array overrun when setting NFTNL_SET_DESC_CONCAT

[Pablo Neira Ayuso <pablo@netfilter.org>]

Hi Phil,

On Wed, Nov 27, 2024 at 07:01:01PM +0100, Phil Sutter wrote:
> Assuming max data_len of 16 * 4B and no zero bytes in 'data':
> The while loop will increment field_count, use it as index for the
> field_len array and afterwards make sure it hasn't increased to
> NFT_REG32_COUNT. Thus a value of NFT_REG32_COUNT - 1 (= 15) will pass
> the check, get incremented to 16 and used as index to the 16 fields long
> array.
> Use a less fancy for-loop to avoid the increment vs. check problem.

for-loop is indeed better.

Patch LGTM, thanks.

> Fixes: 407f616ea5318 ("set: buffer overflow in NFTNL_SET_DESC_CONCAT setter")
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Reviewed-by: Pablo Neira Ayuso <pablo@netfilter.org>

> ---
>  src/set.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/set.c b/src/set.c
> index f127c19b7b8b8..5746397277c48 100644
> --- a/src/set.c
> +++ b/src/set.c
> @@ -185,8 +185,10 @@ int nftnl_set_set_data(struct nftnl_set *s, uint16_t attr, const void *data,
>  			return -1;
>  
>  		memcpy(&s->desc.field_len, data, data_len);
> -		while (s->desc.field_len[++s->desc.field_count]) {
> -			if (s->desc.field_count >= NFT_REG32_COUNT)
> +		for (s->desc.field_count = 0;
> +		     s->desc.field_count < NFT_REG32_COUNT;
> +		     s->desc.field_count++) {
> +			if (!s->desc.field_len[s->desc.field_count])
>  				break;
>  		}
>  		break;
> -- 
> 2.47.0
>