Re: [net,06/11] netfilter: nf_conntrack_h323: fix OOB read in decode_int() CONS case

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
netfilter: nf_conntrack_h323: fix OOB read in decode_int() CONS case

This patch adds a boundary check in the CONS case of decode_int() to
prevent reading beyond the buffer after get_bits() returns a length
value. The fix validates that len bytes remain before calling
get_uint().

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 62aa22a07876..c972e9488e16 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -331,6 +331,8 @@ static int decode_int(struct bitstr *bs, const struct field_t *f,
>  		if (nf_h323_error_boundary(bs, 0, 2))
>  			return H323_ERROR_BOUND;
>  		len = get_bits(bs, 2) + 1;
> +		if (nf_h323_error_boundary(bs, len, 0))
> +			return H323_ERROR_BOUND;
>  		BYTE_ALIGN(bs);
>  		if (base && (f->attr & DECODE)) {	/* timeToLive */
>  			unsigned int v = get_uint(bs, len) + f->lb;

This patch fixes the CONS case, but the UNCO case in the same function
has the same vulnerability. In the UNCO case, get_len(bs) can return up
to 0x3FFF (16383 bytes), and the code advances bs->cur by that amount
without checking that len bytes remain:

	case UNCO:
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs);
		bs->cur += len;
		break;

The check only validates 2 bytes for get_len() itself, not the
subsequent len bytes that will be skipped. Does this need the same fix
pattern?

The decode_bitstr() function has a similar issue. In the SEMI case, it
calls get_len(bs) which can return up to 0x3FFF bits, then advances the
buffer by len >> 3 bytes plus partial bits without checking that
sufficient buffer space exists:

	case SEMI:
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs);
		break;
	}

	bs->cur += len >> 3;
	bs->bit = len & 7;

The check at the top only validates 2 bytes for the get_len() call. The
actual advance at the bottom can be up to 2047 bytes. Should this be
checked as well?

The decode_octstr() function has the same pattern in both the SEMI case
and the default case:

	case SEMI:
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 2, 0))
			return H323_ERROR_BOUND;
		len = get_len(bs) + f->lb;
		break;
	default:		/* 2 <= Range <= 255 */
		if (nf_h323_error_boundary(bs, 0, f->sz))
			return H323_ERROR_BOUND;
		len = get_bits(bs, f->sz) + f->lb;
		BYTE_ALIGN(bs);
		break;
	}

	bs->cur += len;

In the SEMI case, get_len() can return up to 0x3FFF and f->lb is added.
In the default case, get_bits(bs, f->sz) returns an attacker-controlled
value based on field size and f->lb is added. Both cases then advance by
len bytes without checking. The existing checks only validate the bytes
needed to READ the length value, not the len bytes that will be
consumed. Should boundary checks be added after both break statements?

Similarly, decode_bmpstr() (which handles 2-byte character strings) has
the same issue in both the BYTE case and default case:

	case BYTE:		/* Range == 256 */
		BYTE_ALIGN(bs);
		if (nf_h323_error_boundary(bs, 1, 0))
			return H323_ERROR_BOUND;
		len = (*bs->cur++) + f->lb;
		break;
	default:		/* 2 <= Range <= 255 */
		if (nf_h323_error_boundary(bs, 0, f->sz))
			return H323_ERROR_BOUND;
		len = get_bits(bs, f->sz) + f->lb;
		BYTE_ALIGN(bs);
		break;
	}

	bs->cur += len << 1;

After computing len from packet data plus f->lb, the function advances
by len << 1 bytes (doubling for 2-byte characters) without verifying
that len << 1 bytes are available. Should this also be checked?