Re: [PATCH nf-next v4 3/4] netfilter: nf_conntrack_amanda: use nf_ct_helper_parse_port()

[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

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

- [High] amanda_help() in net/netfilter/nf_conntrack_amanda.c still
  silently cuts a port field of six or more digits down to a different,…

--

netfilter: nf_conntrack_amanda: use nf_ct_helper_parse_port()

This patch replaces the simple_strtoul() + htons() port parsing in
amanda_help() with nf_ct_helper_parse_port(). The goal is to reject port 0
and values above 65535 instead of silently truncating them to a u16. The
NAT mangle length is still taken from the parser's end pointer.

> diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
> index 14ae660491f36..f157c96b7d223 100644
> --- a/net/netfilter/nf_conntrack_amanda.c
> +++ b/net/netfilter/nf_conntrack_amanda.c
> @@ -89,7 +89,7 @@ static int amanda_help(struct sk_buff *skb,
>  	struct nf_conntrack_tuple *tuple;
>  	unsigned int dataoff, start, stop, off, i;
>  	char pbuf[sizeof("65535")], *tmp;
> -	u16 len;
> +	u16 len, parsed_port;
>  	__be16 port;
>  	int ret = NF_ACCEPT;
>  	nf_nat_amanda_hook_fn *nf_nat_amanda;
> @@ -132,10 +132,10 @@ static int amanda_help(struct sk_buff *skb,
>  			break;
>  		pbuf[len] = '\0';
>  
> -		port = htons(simple_strtoul(pbuf, &tmp, 10));
> -		len = tmp - pbuf;
> -		if (port == 0 || len > 5)
> +		if (nf_ct_helper_parse_port(pbuf, len, &parsed_port, &tmp))
>  			break;
> +		port = htons(parsed_port);
> +		len = tmp - pbuf;

[Severity: High]
Can a port field with six or more digits still be truncated into a
different valid port here?

pbuf is only 6 bytes, and just before this hunk amanda_help() caps the
copy at 5 bytes:

	len = min_t(unsigned int, sizeof(pbuf) - 1, stop - off);
	if (skb_copy_bits(skb, off, pbuf, len))
		break;
	pbuf[len] = '\0';

So nf_ct_helper_parse_port(pbuf, 5, ...) only ever sees the first five
digits. The range check there only covers the len bytes it is given:

net/netfilter/nf_conntrack_helper.c:nf_ct_helper_parse_port() {
	val = nf_ct_helper_parse_uint(cp, len, &e);
	if (e == cp || val == 0 || val > 65535)
		return -1;
}

Nothing checks whether skb byte off + len is another digit. So a reply
such as "CONNECT ... DATA 123456\n" would be accepted as port 12345. In
the same way, 100000 would become 10000 and 655350 would become 65535.
nf_ct_expect_init() would then install a TCP expectation for a port that
never appeared on the wire.

On the NAT path, len = tmp - pbuf is 5 and is passed as matchlen.
help() in net/netfilter/nf_nat_amanda.c then does:

	if (!nf_nat_mangle_udp_packet(skb, ct, ctinfo,
				      protoff, matchoff, matchlen,
				      buffer, strlen(buffer))) {

Only the first 5 bytes get replaced, so the extra digit stays in the
payload. For example, a NAT-chosen port of 40000 would come out as
"400006", which no longer matches the expectation.

The commit message says:

    The digit string is copied into a 6-byte buffer,
    so at most five digits are parsed, but values 65536-99999 still truncated

and:

    Use nf_ct_helper_parse_port(), which rejects port 0 and values above
    65535 in one call.

With the 5-byte window, is that accurate? Values above 99999 are also
above 65535, and they still truncate through the window instead of being
rejected. The later commit "netfilter: nf_conntrack_sip: use shared
helper parsers" in this series does not touch amanda, so this code is
unchanged at the end of the series.

Would it work to copy one extra byte into pbuf, or to check that skb
byte off + len is not a digit, and reject the field when the digit run
is longer than five characters?

>  
>  		exp = nf_ct_expect_alloc(ct);
>  		if (exp == NULL) {

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923091608.2617604-1-rc%40rexion.ai