[PATCH] netfilter: conntrack_amanda: fix port value truncation

[PATCH] netfilter: conntrack_amanda: fix port value truncation

[Aamir Ahmed]

amanda_help() uses simple_strtoul() to parse the port number from
Amanda CONNECT replies, passing the result directly through htons()
into a __be16.  simple_strtoul() returns unsigned long, so values
above 65535 are silently truncated by the implicit conversion to u16
inside htons().

The existing check "port == 0 || len > 5" is insufficient: it
catches values that truncate to zero (e.g. 65536) and strings longer
than 5 digits, but misses values 65537-99999 whose u16 truncation is
non-zero.  For example, port 65537 becomes port 1, creating a
conntrack expectation for an unintended port.

Parse into an unsigned long and explicitly reject values above 65535
before the htons() conversion, mirroring the pattern used by the FTP
helper's get_port() and the recent IPVS FTP fix (commit
e625a9477d12).

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
 net/netfilter/nf_conntrack_amanda.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
index 14ae660491f3..057cef7e2a7e 100644
--- a/net/netfilter/nf_conntrack_amanda.c
+++ b/net/netfilter/nf_conntrack_amanda.c
@@ -91,6 +91,7 @@ static int amanda_help(struct sk_buff *skb,
 	char pbuf[sizeof("65535")], *tmp;
 	u16 len;
 	__be16 port;
+	unsigned long tmp_port;
 	int ret = NF_ACCEPT;
 	nf_nat_amanda_hook_fn *nf_nat_amanda;
 
@@ -132,10 +133,11 @@ static int amanda_help(struct sk_buff *skb,
 			break;
 		pbuf[len] = '\0';
 
-		port = htons(simple_strtoul(pbuf, &tmp, 10));
+		tmp_port = simple_strtoul(pbuf, &tmp, 10);
 		len = tmp - pbuf;
-		if (port == 0 || len > 5)
+		if (tmp_port == 0 || tmp_port > 65535 || len > 5)
 			break;
+		port = htons(tmp_port);
 
 		exp = nf_ct_expect_alloc(ct);
 		if (exp == NULL) {
-- 
2.43.0

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Chenguang Zhao]

在 2026/9/7 07:37, Aamir Ahmed 写道:
> amanda_help() uses simple_strtoul() to parse the port number from
> Amanda CONNECT replies, passing the result directly through htons()
> into a __be16.  simple_strtoul() returns unsigned long, so values
> above 65535 are silently truncated by the implicit conversion to u16
> inside htons().
>
> The existing check "port == 0 || len > 5" is insufficient: it
> catches values that truncate to zero (e.g. 65536) and strings longer
> than 5 digits, but misses values 65537-99999 whose u16 truncation is
> non-zero.  For example, port 65537 becomes port 1, creating a
> conntrack expectation for an unintended port.
>
> Parse into an unsigned long and explicitly reject values above 65535
> before the htons() conversion, mirroring the pattern used by the FTP
> helper's get_port() and the recent IPVS FTP fix (commit
> e625a9477d12).
>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
> ---
>  net/netfilter/nf_conntrack_amanda.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
> index 14ae660491f3..057cef7e2a7e 100644
> --- a/net/netfilter/nf_conntrack_amanda.c
> +++ b/net/netfilter/nf_conntrack_amanda.c
> @@ -91,6 +91,7 @@ static int amanda_help(struct sk_buff *skb,
>  	char pbuf[sizeof("65535")], *tmp;
>  	u16 len;
>  	__be16 port;
> +	unsigned long tmp_port;
>  	int ret = NF_ACCEPT;
>  	nf_nat_amanda_hook_fn *nf_nat_amanda;
>  
> @@ -132,10 +133,11 @@ static int amanda_help(struct sk_buff *skb,
>  			break;
>  		pbuf[len] = '\0';
>  
> -		port = htons(simple_strtoul(pbuf, &tmp, 10));
> +		tmp_port = simple_strtoul(pbuf, &tmp, 10);
>  		len = tmp - pbuf;
> -		if (port == 0 || len > 5)
> +		if (tmp_port == 0 || tmp_port > 65535 || len > 5)
len = min_t(unsigned int, sizeof(pbuf) - 1, stop - off) already
limits the length so that it cannot be greater than 5. Could the
len > 5 check here be dropped?
>  			break;
> +		port = htons(tmp_port);
>  
>  		exp = nf_ct_expect_alloc(ct);
>  		if (exp == NULL) {

The subject is "[PATCH] ...". Since this is a bugfix, please use "[PATCH net] ...".

Reviewed-by: Chenguang Zhao <zhaochenguang@kylinos.cn>

Thanks

Chenguang

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Florian Westphal]

Aamir Ahmed <elb12345@hotmail.co.uk> wrote:
> amanda_help() uses simple_strtoul() to parse the port number from
> Amanda CONNECT replies, passing the result directly through htons()
> into a __be16.  simple_strtoul() returns unsigned long, so values
> above 65535 are silently truncated by the implicit conversion to u16
> inside htons().
> 
> The existing check "port == 0 || len > 5" is insufficient: it
> catches values that truncate to zero (e.g. 65536) and strings longer
> than 5 digits, but misses values 65537-99999 whose u16 truncation is
> non-zero.  For example, port 65537 becomes port 1, creating a
> conntrack expectation for an unintended port.
> 
> Parse into an unsigned long and explicitly reject values above 65535
> before the htons() conversion, mirroring the pattern used by the FTP
> helper's get_port() and the recent IPVS FTP fix (commit
> e625a9477d12).

Is amanda still a thing?  Maybe time to retire this helper?

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Pablo Neira Ayuso]

On Mon, Sep 07, 2026 at 02:56:13PM +0800, Chenguang Zhao wrote:
> 
> 在 2026/9/7 07:37, Aamir Ahmed 写道:
> > amanda_help() uses simple_strtoul() to parse the port number from
> > Amanda CONNECT replies, passing the result directly through htons()
> > into a __be16.  simple_strtoul() returns unsigned long, so values
> > above 65535 are silently truncated by the implicit conversion to u16
> > inside htons().
> >
> > The existing check "port == 0 || len > 5" is insufficient: it
> > catches values that truncate to zero (e.g. 65536) and strings longer
> > than 5 digits, but misses values 65537-99999 whose u16 truncation is
> > non-zero.  For example, port 65537 becomes port 1, creating a
> > conntrack expectation for an unintended port.
> >
> > Parse into an unsigned long and explicitly reject values above 65535
> > before the htons() conversion, mirroring the pattern used by the FTP
> > helper's get_port() and the recent IPVS FTP fix (commit
> > e625a9477d12).

Please, specify target tree. This must go to nf-next.

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Aamir Ahmed]

On Mon, Sep 07, 2026 at 02:56:13PM +0800, Chenguang Zhao wrote:
> len = min_t(unsigned int, sizeof(pbuf) - 1, stop - off) already
> limits the length so that it cannot be greater than 5. Could the
> len > 5 check here be dropped?

Yes, that is correct actually - will drop it in v2. I will carry your
Reviewed-by on v2 if that is OK with the change.

On Mon, Sep 07, 2026 at 06:37:43PM +0200, Pablo Neira Ayuso wrote:
> Please, specify target tree. This must go to nf-next.

Will resend as [PATCH nf-next v2], without the stable Cc.

Kind Regards

Aamir A.

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[netdev-bot+sashiko]

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

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

- [High] net/netfilter/nf_conntrack_amanda.c:amanda_help() — the new
  range check closes only the htons() truncation window for numbers that…

--

netfilter: conntrack_amanda: fix port value truncation

The patch parses the Amanda CONNECT port into an unsigned long and rejects
values above 65535 before the htons() conversion, instead of relying on the
old "port == 0 || len > 5" test.  A new tmp_port variable holds the
simple_strtoul() result.

> diff --git a/net/netfilter/nf_conntrack_amanda.c b/net/netfilter/nf_conntrack_amanda.c
> index 14ae660491f36..057cef7e2a7ed 100644
> --- a/net/netfilter/nf_conntrack_amanda.c
> +++ b/net/netfilter/nf_conntrack_amanda.c
> @@ -91,6 +91,7 @@ static int amanda_help(struct sk_buff *skb,
>  	char pbuf[sizeof("65535")], *tmp;
>  	u16 len;
>  	__be16 port;
> +	unsigned long tmp_port;
>  	int ret = NF_ACCEPT;
>  	nf_nat_amanda_hook_fn *nf_nat_amanda;
>  
> @@ -132,10 +133,11 @@ static int amanda_help(struct sk_buff *skb,
>  			break;
>  		pbuf[len] = '\0';
>  
> -		port = htons(simple_strtoul(pbuf, &tmp, 10));
> +		tmp_port = simple_strtoul(pbuf, &tmp, 10);
>  		len = tmp - pbuf;
> -		if (port == 0 || len > 5)
> +		if (tmp_port == 0 || tmp_port > 65535 || len > 5)
>  			break;
> +		port = htons(tmp_port);

[Severity: High]
Can the new tmp_port > 65535 test ever fire, and does it close the case
described in the changelog?

In amanda_help(), the digit string is copied into a 6-byte buffer and the
copy length is clamped before parsing:

net/netfilter/nf_conntrack_amanda.c:amanda_help() {
	char pbuf[sizeof("65535")], *tmp;
	...
		len = min_t(unsigned int, sizeof(pbuf) - 1, stop - off);
		if (skb_copy_bits(skb, off, pbuf, len))
			break;
		pbuf[len] = '\0';
}

So simple_strtoul() sees at most 5 characters, which bounds tmp_port at
99999 and bounds len = tmp - pbuf at 5.

For a reply payload of "CONNECT DATA 123456\n", pbuf becomes "12345",
tmp_port becomes 12345 and len becomes 5, so all three clauses of

		if (tmp_port == 0 || tmp_port > 65535 || len > 5)

pass and an expectation is installed for TCP port 12345 even though the
advertised value was 123456.  Isn't that the same "conntrack expectation
for an unintended port" the changelog is trying to eliminate, just via the
fixed-size copy rather than the u16 narrowing?

Would it make sense to also reject the token when the digit run is not
terminated inside the examined region, for example bailing out when
stop - off > sizeof(pbuf) - 1, or when the character following the parsed
digits is still a digit, the way the FTP helper's parsers require an
explicit terminator?

There is also a NAT-side effect of the clamp.  amanda_help() hands len to
the NAT hook as matchlen:

			ret = nf_nat_amanda(skb, ct, ctinfo, protoff,
					    off - dataoff, len, exp);

and net/netfilter/nf_nat_amanda.c:help() replaces exactly matchlen bytes:

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

With a 6-digit advertisement, matchlen is 5, so only 5 of the 6 digit bytes
are rewritten and a trailing digit stays in the forwarded payload.  Should
the over-long digit run be rejected before reaching the NAT hook?

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

Separately, the changelog states:

  "The existing check "port == 0 || len > 5" is insufficient: it
   catches values that truncate to zero (e.g. 65536) and strings longer
   than 5 digits"

Given the min_t() clamp above, len can never exceed 5, so the len > 5
clause looks like it was already unreachable and cannot catch strings
longer than 5 digits.  Could that sentence be reworded, and should the
now-dead len > 5 clause be dropped or replaced by a real over-long check?

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

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Florian Westphal]

netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider:
> 
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
> 
> - [High] net/netfilter/nf_conntrack_amanda.c:amanda_help() — the new
>   range check closes only the htons() truncation window for numbers that…

There is queued patch in the backlog that added a unified port parser
for use by all helpers:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260503083220.630655-2-rc@rexion.ai/

I think it would be good if the author of that series would rebase and
resend a new version.

Re: [PATCH] netfilter: conntrack_amanda: fix port value truncation

[Aamir Ahmed]

On Wed, Sep 09, 2026 at 02:14:20PM +0200, Florian Westphal wrote:
> There is queued patch in the backlog that added a unified port parser
> for use by all helpers:
>
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20260503083220.630655-2-rc@rexion.ai/
>
> I think it would be good if the author of that series would rebase and
> resend a new version.

Yes, that seems a better overall solution. I will drop this patch for
now. If anything further is needed from me, happy to engage.

Kind Regards

Aamir A.