Re: [RESEND,nf-next] netfilter: nf_conntrack_h323: fix double cursor advance in decode_int()

[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 (1) · Low: 1

- [Medium] Commit message mischaracterizes the impact of the fixed double
  cursor advance in decode_int().
- [Low] The patch is a user-visible bug fix to
  net/netfilter/nf_conntrack_h323_asn1.c but carries no `Fixes:` tag, so…

Pre-existing issues:
- [Medium] The RAS timeToLive value decoded by the CONS/DECODE branch is
  fully remote-controlled (nf_conntrack_h323_types.c:1594/1646 define it…

--

netfilter: nf_conntrack_h323: fix double cursor advance in decode_int()

In the CONS case of decode_int(), get_uint() already advances the bitstream
cursor, so the following unconditional bs->cur += len consumed the value
bytes twice.  The patch moves the manual advance into an else branch so the
bytes are consumed exactly once.  The code change itself looks correct.

A few questions about the changelog and about one pre-existing item in this
area.

>     All fields following such an integer are decoded from a wrong bit
>     position.  This affects the timeToLive field in RAS
>     RegistrationRequest/RegistrationConfirm messages: subsequent fields
>     (e.g. transport addresses) end up being parsed from arbitrary offsets
>     inside the message, so the helper may create expectations based on
>     bogus addresses/ports.

[Severity: Medium]
Is this description of the impact accurate?

The only fields that reach the CONS plus DECODE branch are the two
timeToLive entries in _RegistrationRequest and _RegistrationConfirm in
nf_conntrack_h323_types.c.  Every other CONS integer is SKIP or STOP and
takes the (unchanged) manual advance path.

In both tables timeToLive is an extension component, so decode_seq() wraps
it as an open type and resets the cursor after the decoder returns:

net/netfilter/nf_conntrack_h323_asn1.c:decode_seq() {
	...
	beg = bs->cur;
	if ((err = (Decoders[son->type]) (bs, son, base, level + 1)) < H323_ERROR_NONE)
		return err;
	bs->cur = beg + len;
	bs->bit = 0;
	...
}

Doesn't that discard the over-advanced cursor, so parsing resumes at the
correct offset?

Also, the entry immediately following timeToLive in both tables carries
STOP:

net/netfilter/nf_conntrack_h323_types.c:_RegistrationRequest[] = {
	{FNAME("timeToLive") INT, CONS, 1, 0, DECODE | OPT,
	 offsetof(RegistrationRequest, timeToLive), NULL},
	{FNAME("tokens") SEQOF, SEMI, 0, 0, STOP | OPT, 0, NULL},

decode_seq() returns H323_ERROR_STOP at the STOP son, so no later field is
decoded at all.  The DECODE'd transport addresses (callSignalAddress,
rasAddress) are root components parsed before timeToLive.  Can a bogus
address or port expectation actually be produced by this bug?

The one effect that does escape decode_int() looks like the trailing
boundary check:

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

When fewer than len bytes follow the timeToLive value, the doubled advance
pushes bs->cur past bs->end and DecodeRasMessage() fails, so ras_help()
takes the accept path and skips the whole RRQ/RCF:

net/netfilter/nf_conntrack_h323_main.c:ras_help() {
	ret = DecodeRasMessage(data, datalen, &ras);
	if (ret < 0) {
		pr_debug("nf_ct_ras: decoding error: %s\n", ...);
		goto accept;
	}

That means no TTL based nf_ct_refresh() and no Q.931 expectation timeout
update for that message.  Could the changelog be reworded to describe this
spurious out-of-bound decode failure instead of the bogus address/port
expectation scenario?  As written it reads like a security relevant misparse
and may drive an urgent stable backport on the wrong grounds.

>     Assisted-by: LLM
>     Signed-off-by: Qing Luo <luoqing@kylinos.cn>

[Severity: Low]
Should this carry a Fixes: tag?

git blame at the baseline shows the unconditional bs->cur += len and the
surrounding CONS case in decode_int() came in with 5e35941d9901
("[NETFILTER]: Add H.323 conntrack/NAT helper"), so the defect has been
there since decode_int() was added.  Since decode_int() is reachable
remotely through DecodeRasMessage() from ras_help(), stable tooling would
need the tag to pick the affected trees:

Fixes: 5e35941d9901 ("[NETFILTER]: Add H.323 conntrack/NAT helper")

> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 6830c9da3507..a51275cc4d0f 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -336,8 +336,9 @@ static int decode_int(struct bitstr *bs, const struct field_t *f,
>  			unsigned int v = get_uint(bs, len) + f->lb;
>  			PRINT(" = %u", v);
>  			*((unsigned int *)(base + f->offset)) = v;

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the value
produced here: is the decoded timeToLive bounded anywhere?

nf_conntrack_h323_types.c declares it as {INT, CONS, 1, 0, DECODE | OPT} in
both _RegistrationRequest and _RegistrationConfirm, and get_uint(bs, len)
with len up to 4 covers the whole u32 range, so v is fully remote
controlled.

process_rrq() and process_rcf() then copy it straight into info->timeout
with no upper limit, unlike the default_rrq_ttl module parameter path:

net/netfilter/nf_conntrack_h323_main.c:process_rcf() {
	if (rcf->options & eRegistrationConfirm_timeToLive) {
		pr_debug("nf_ct_ras: RCF TTL = %u seconds\n", rcf->timeToLive);
		info->timeout = rcf->timeToLive;
	}

	if (info->timeout > 0) {
		nf_ct_refresh(ct, info->timeout * HZ);
		...
			WRITE_ONCE(exp->timeout,
				   nfct_time_stamp + (info->timeout * HZ));

Does info->timeout * HZ wrap in u32 arithmetic here, and can a peer keep the
RAS conntrack entry and its Q.931 expectation alive for an arbitrarily long
time, or wrap the product down to a near-zero timeout?

__nf_ct_refresh_acct() in nf_conntrack_core.c applies no upper clamp on
extra_jiffies:

	if (nf_ct_is_confirmed(ct))
		extra_jiffies += nfct_time_stamp;

	if (READ_ONCE(ct->timeout) != extra_jiffies)
		WRITE_ONCE(ct->timeout, extra_jiffies);

so the helper looks like the only place a bound could be applied.  Note this
predates the patch: before the change the correct TTL bytes were still read
and decode_seq() reset the cursor, so the value already reached
process_rrq()/process_rcf() whenever bytes followed the TTL field.

>  		}
> -		bs->cur += len;
>  		break;
>  	case UNCO:
>  		BYTE_ALIGN(bs);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831013315.95231-1-l1138897701%40163.com