Re: [PATCH] netfilter: nf_conntrack_h323: fix NULL pointer deref in decode_seqof()

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says decode_seqof() can be made to
  dereference NULL through the eight *_Caps.supportedPrefixes rows (and…
- [Low] The new comment in decode_seqof() is wrong in three ways.

--

netfilter: nf_conntrack_h323: fix NULL pointer deref in decode_seqof()

This patch adds a NULL check on f->fields in decode_seqof(). When the
pointer is NULL it returns H323_ERROR_BOUND. The commit message says an
attacker can use the eight *_Caps.supportedPrefixes rows to trigger a NULL
pointer dereference there.

> The eight supportedPrefixes table entries (_H310Caps, _H320Caps,
> _H321Caps, _H322Caps, _H323Caps, _H324Caps, _VoiceCaps and
> _T120OnlyCaps, all reached through the _SupportedProtocols CHOICE)
> are declared SEQOF,SEMI with fields=NULL.  decode_seqof()
> dereferences f->fields on the first loop iteration whenever the
> attacker-controlled SEMI count is non-zero, causing a NULL pointer
> dereference.

[ ... ]

> Cc: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

[Severity: Medium]
Can decode_seqof() actually be reached for these rows?

The parent entries in _SupportedProtocols in nf_conntrack_h323_types.c
are:

	{FNAME("h310") SEQ, 1, 1, 3, SKIP | EXT, 0, _H310Caps},
	...
	{FNAME("t120-only") SEQ, 1, 1, 3, SKIP | EXT, 0, _T120OnlyCaps},

With lb=1, nonStandardData is the only root component of _H310Caps.
dataRatesSupported and supportedPrefixes are both extension additions.

The parent is SKIP without DECODE, so decode_seq() sets base to NULL when
it starts:

	base = (base && (f->attr & DECODE)) ? base + f->offset : NULL;

In the extension loop of decode_seq(), each present extension reads its
open type length. It then reaches this check before Decoders[son->type]
is called:

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

The supportedPrefixes row is also plain SKIP:

	{FNAME("supportedPrefixes") SEQOF, SEMI, 0, 0, SKIP, 0, NULL},

So it looks like Decoders[SEQOF] is never called for any of the eight
rows.

The other SEQOF rows in nf_conntrack_h323_types.c that have NULL fields
seem to fall into two groups:

  - STOP rows. decode_seq() returns H323_ERROR_STOP for these before any
    decoder runs.
  - SKIP rows in an extension position. These are skipped by length.

Some examples are Setup_UUIE, Connect_UUIE,
EndpointType.supportedTunnelledProtocols, RRQ.alternateEndpoints and
RCF.alternateGatekeeper.

If none of these rows can reach decode_seqof(), the new check is dead
code. Could this be resent as hardening, without the Cc: stable tag, the
Fixes: tag and the claim of an attacker-reachable NULL dereference?

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3507c..4d66db5bf5f70 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -692,6 +692,13 @@ static int decode_seqof(struct bitstr *bs, const struct field_t *f,
>  
>  	/* Decode nested field */
>  	son = f->fields;
> +	/*
> +	 * Malformed table entries may carry a NULL fields pointer (e.g.
> +	 * the *_Caps.supportedPrefixes SEQOF,SEMI rows); treat them as
> +	 * undecodable instead of dereferencing NULL when count > 0.
> +	 */
> +	if (!son)
> +		return H323_ERROR_BOUND;

[Severity: Low]
Is this comment accurate? A few parts of it don't seem to match the code.

The comment calls the *_Caps.supportedPrefixes rows malformed and uses
them as an example of rows that reach decode_seqof(). As noted above,
decode_seq() skips them by length. A NULL fields pointer on SKIP and STOP
rows also looks like the normal convention in nf_conntrack_h323_types.c.
Many rows use it.

The comment says the problem only occurs "when count > 0". The check
runs after count has been decoded, though, and it returns an error for
every count, including 0. Before this patch, count == 0 with a NULL base
succeeded.

Elsewhere in this file, H323_ERROR_BOUND means the bitstream would run
past the end of the buffer. Would a different error code fit a missing
table descriptor better?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009124116.3950469-1-bsdhenrymartin%40gmail.com