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

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

[Aamir Ahmed]

In decode_enum(), get_bit(bs) is called to read the extension marker
bit without first verifying that the bitstream has enough data
remaining. get_bit() dereferences bs->cur directly:

    unsigned int b = (*bs->cur) & (0x80 >> bs->bit);

When the bitstream is exhausted (bs->cur >= bs->end), this results in a
one-byte out-of-bounds read from the skb data area.

Both decode_seq() and decode_choice() correctly call
nf_h323_error_boundary(bs, 0, 1) before reading the extension bit via
get_bit(). decode_enum() is the only decoder that omits this check.

The bug is reachable when parsing a truncated H.323 message containing a
PER-encoded enumeration with the extension marker (such as the
screeningIndicator field), through port 1720 with the nf_conntrack_h323
helper active.

Add the missing nf_h323_error_boundary(bs, 0, 1) check before get_bit()
to match the pattern used in decode_seq() and decode_choice().

Fixes: 5e35941d9901 ("[NETFILTER]: Add H.323 conntrack/NAT helper")
Cc: stable@vger.kernel.org
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 6830c9da3507..ba6ce1f04e1e 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 {
-- 
2.43.0

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

[Aamir Ahmed]

Please drop this patch - decode_enum() cannot be reached.

The types table has four ENUM fields, all named screeningIndicator,
and each one sits past its parent SEQ's root field count (Setup index
28 vs lb 13, Connect 14 vs 4, Alerting 12 vs 3, ReleaseComplete 7 vs
2). They are therefore only ever seen by the extension loop in
decode_seq(), which skips any son without DECODE:

	if (!base || !(son->attr & DECODE)) {
		bs->cur += len;
		continue;
	}

All four are SKIP, so the decoder is never dispatched. No SEQOF item
or CHOICE alternative has type ENUM either, and Decoders[] is the only
reference to it, so the unguarded get_bit() cannot run on any input.

pw-bot: rejected

Regards,
Aamir Ahmed

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

[netdev-bot+sashiko]

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