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

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.

[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