Re: [PATCH] netfilter: conntrack_irc: fix port value truncation in parse_dcc()

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix: in parse_dcc()
  (net/netfilter/nf_conntrack_irc.c:92-95) the new bound check is…

--

netfilter: conntrack_irc: fix port value truncation in parse_dcc()

This adds a bound check to the DCC port parsing in parse_dcc() so that
values above 65535 are rejected instead of being silently truncated into
the u16 output.  The stated goal is to prevent a conntrack expectation
being created for a port that never appeared in the DCC command.

> diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
> index 92360963757a9..8321f12dbf155 100644
> --- a/net/netfilter/nf_conntrack_irc.c
> +++ b/net/netfilter/nf_conntrack_irc.c
> @@ -64,6 +64,7 @@ static int parse_dcc(char *data, const char *data_end, __be32 *ip,
>  		     u_int16_t *port, char **ad_beg_p, char **ad_end_p)
>  {
>  	char *tmp;
> +	unsigned long tmp_port;

[Severity: High]
Is unsigned long wide enough here on 32-bit builds?

> @@ -88,7 +89,10 @@ static int parse_dcc(char *data, const char *data_end, __be32 *ip,
>  		data++;
>  	}
>  
> -	*port = simple_strtoul(data, &data, 10);
> +	tmp_port = simple_strtoul(data, &data, 10);
> +	if (tmp_port > 65535)
> +		return -1;
> +	*port = tmp_port;

Does the truncation still happen before the new check runs on ILP32
(i386, ARM32, MIPS32), where unsigned long is 32 bits?

simple_strtoul() itself narrows:

lib/vsprintf.c:
unsigned long simple_strtoul(const char *cp, char **endp, unsigned int base)
{
	return simple_strtoull(cp, endp, base);
}

And _parse_integer_limit() in lib/kstrtox.c only saturates to ULLONG_MAX
when check_mul_overflow()/check_add_overflow() fire, so a value in the
2^32 range is returned exactly as a 64-bit result:

		if (unlikely(res & (~0ull << 60))) {
			if (check_mul_overflow(res, base, &res) ||
			    check_add_overflow(res, val, &res)) {
				res = ULLONG_MAX;
				overflow = KSTRTOX_OVERFLOW;

So for a DCC field of "4294967297", would the 64-bit 0x1_00000001 be
narrowed to 1 by the assignment into tmp_port, making tmp_port > 65535
false and leaving *port == 1?

If so, the path in help() would still be reached with a port that was
never in the command, since the only remaining guard is dcc_port == 0:

net/netfilter/nf_conntrack_irc.c:help() {
	...
			    dcc_port == 0) {
	...
			port = htons(dcc_port);
			nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT,
					  tuple->src.l3num,
					  NULL, &tuple->dst.u3,
					  IPPROTO_TCP, NULL, &port);
	...
}

The input here is remote IRC payload copied into irc_buffer by help(),
and parse_dcc() only requires a '\n' inside the buffer, so a payload of
the form "PRIVMSG x :\1DCC SEND t <ip> 4294967297\1\n" reaches this
parse.

The commit message says this follows nf_conntrack_ftp:

  "The nf_conntrack_ftp helper had the same class of defect and was fixed
  by using a wider accumulator with a range check (get_port checks
  tmp_port > 65535).  Apply the same pattern here"

Is that equivalent, though?  get_port() uses a fixed-width u32 and
re-tests after every digit, so it cannot be affected by the width of
unsigned long:

net/netfilter/nf_conntrack_ftp.c:get_port() {
	u32 tmp_port = 0;
	...
		} else if (data[i] >= '0' && data[i] <= '9') {
			tmp_port = tmp_port*10 + data[i] - '0';
			if (tmp_port > 65535)
				break;
	...
}

Would a width-independent form work better here, for example parsing
with simple_strtoull() into an unsigned long long, using kstrtou16() and
checking for -ERANGE, or the ftp-style per-digit bound?

>  	*ad_end_p = data;
>  
>  	return 0;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001B81958B28F762352EF3DC8B32%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM