Re: [PATCH] netfilter: nf_conntrack_h323: fix OOB read in decode_enum()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] get_bitmap() dereferences one byte more than
  nf_h323_error_boundary() validates whenever l = bs->bit + b is an…

--

netfilter: nf_conntrack_h323: fix OOB read in decode_enum()

This adds a nf_h323_error_boundary(bs, 0, 1) check in decode_enum() before
the extension marker bit is read with get_bit(), matching what decode_seq()
and decode_choice() already do.  Without it, a truncated H.323 message can
cause a one-byte read past bs->end.

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3507c..ba6ce1f04e1ed 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -363,6 +363,8 @@ static int decode_enum(struct bitstr *bs, const struct field_t *f,
>  {
>  	PRINT("%*s%s\n", level * TAB_SIZE, " ", f->name);
>  
> +	if (nf_h323_error_boundary(bs, 0, 1))
> +		return H323_ERROR_BOUND;
>  	if ((f->attr & EXT) && get_bit(bs)) {
>  		INC_BITS(bs, 7);
>  	} else {

[Severity: Medium]
The check added here closes the decode_enum() case, but does the same
one-byte over-read remain reachable through get_bitmap()?

nf_h323_error_boundary() only rounds up when there is a partial byte:

	bits += bs->bit;
	bytes += bits / BITS_PER_BYTE;
	if (bits % BITS_PER_BYTE > 0)
		bytes++;

	if (bs->cur + bytes > bs->end)
		return 1;

So for bits + bs->bit == 16 it only requires bs->cur + 2 <= bs->end.

get_bitmap() with l == bs->bit + b == 16 consumes two bytes in the loop and
then dereferences a third:

	for (bytes = l >> 3, shift = 24, v = 0; bytes;
	     bytes--, shift -= 8)
		v |= (unsigned int)(*bs->cur++) << shift;

	if (l < 32) {
		v |= (unsigned int)(*bs->cur) << shift;
		v <<= bs->bit;
	}

The third byte is at bs->end.  The same holds for l == 24, where the loop
reads three bytes and the l < 32 branch reads a fourth.

The width is attacker controlled at the extension bitmap in decode_seq():

	bmp2_len = get_bits(bs, 7) + 1;
	if (nf_h323_error_boundary(bs, 0, bmp2_len))
		return H323_ERROR_BOUND;
	if (bmp2_len > 32)
		return H323_ERROR_RANGE;
	bmp2 = get_bitmap(bs, bmp2_len);

Picking bmp2_len = 16 - bs->bit keeps bmp2_len within 1..32 while making l
exactly 16, so the boundary check asks for two remaining bytes and
get_bitmap() touches a third.  The earlier bmp = get_bitmap(bs, f->sz) call
in decode_seq() has the same property whenever bs->bit + f->sz is 16 or 24,
with the width coming from the static field table.

The stray byte is masked off afterwards by:

	v &= 0xffffffff << (32 - b);

and bs->end stays inside h323_buffer or inside the skb linear area, so
nothing is corrupted or leaked, but it is the same class of read past the
intended end of the bitstream that the subject line describes.

Would it make sense to respin this covering the
nf_h323_error_boundary()/get_bitmap() accounting as well, so that the
decoder family is consistent?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB00011EEAA381DC17852BA37CC8B22%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM