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

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

[Henry Martin]

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.

Return H323_ERROR_BOUND when the nested field pointer is NULL.

This issue was discovered by Tencent CodeBuddy Security.

Cc: stable@vger.kernel.org
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 6830c9da3507..4d66db5bf5f7 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;
 	if (base)
 		base -= son->offset;
 	for (i = 0; i < count; i++) {
--
2.43.7

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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