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

[Florian Westphal <fw@strlen.de>]

Jakub Kicinski <kuba@kernel.org> wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Jenny, could you please double-check?
I don't think this report is right.

> 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().

SO far I can follow LLM response.

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

I don't think so.  Pointer can advance, but right after the swtich/case
there is:

358         if (nf_h323_error_boundary(bs, 0, 0))
359                 return H323_ERROR_BOUND;
360         return H323_ERROR_NONE;

So bs->cur can advance past cur->end, but then the function
returns ERROR_BOUND.

> 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);

AFAICS we've checked get_len() doesn't cause oob access.

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

AFAICS it is, right after there is:
410         if (nf_h323_error_boundary(bs, 0, 0))
411                 return H323_ERROR_BOUND;

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

Pointer increment, right after there is:

483         if (nf_h323_error_boundary(bs, 0, 0))
484                 return H323_ERROR_BOUND;

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

AFAICS there are such checks... :-/

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

We write to bs->cur, but AFAICS the write has been vetted for.

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

AFAICS it is...

510         bs->cur += len << 1;
511
512         if (nf_h323_error_boundary(bs, 0, 0))
513                 return H323_ERROR_BOUND;

So AFAICS this LLM response is bunk.