Re: [PATCH net 05/15] ipvs: reject FTP control ports as data ports

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The two new rejection checks in ip_vs_ftp_in() and
  ip_vs_ftp_out() return 0.

--

ipvs: reject FTP control ports as data ports

This patch adds is_control_port() to the IPVS FTP helper and uses it to
refuse a zero port, or a configured FTP control port, as a data port.
ip_vs_ftp_out() refuses such ports in PASV/EPSV replies. ip_vs_ftp_in()
refuses PORT/EPRT commands that carry a zero port, or where vport - 1 is
a control port.

> diff --git a/net/netfilter/ipvs/ip_vs_ftp.c b/net/netfilter/ipvs/ip_vs_ftp.c
> index 9e3e005a82635..4822a1a75212d 100644
> --- a/net/netfilter/ipvs/ip_vs_ftp.c
> +++ b/net/netfilter/ipvs/ip_vs_ftp.c

[ ... ]

> @@ -319,6 +330,10 @@ static int ip_vs_ftp_out(struct ip_vs_app *app, struct ip_vs_conn *cp,
>  		return 1;
>  	}
>  
> +	/* Do not redirect data to control ports */
> +	if (!port || is_control_port(ntohs(port)))
> +		return 0;
> +
>  	/* Now update or create a connection entry for it */
>  	{
>  		struct ip_vs_conn_param p;
> @@ -529,6 +544,9 @@ static int ip_vs_ftp_in(struct ip_vs_app *app, struct ip_vs_conn *cp,
>  		return 1;
>  	}
>  
> +	if (!port || is_control_port(ntohs(cp->vport) - 1))
> +		return 0;
> +

[Severity: Medium]
Does returning 0 here stall the whole FTP control connection, rather than
only refusing the data connection?

The pkt_in/pkt_out contract in include/net/ip_vs.h treats 0 as an error:

    Return: 0=Error, 1=Payload Not Mangled/Mangled but checksum is ok,

On the inbound side, tcp_dnat_handler() passes that result up:

    if (!(ret = ip_vs_app_pkt_in(cp, skb, iph)))
        return 0;

ip_vs_nat_xmit() then takes the tx_error path and frees the skb. The
segment that carries the PORT/EPRT command is dropped.

cp->vport is fixed for the life of the connection, and ip_vs_ftp_in()
keeps no state for this case. Every retransmission of that segment is
parsed the same way and dropped again. Nothing after it in the TCP stream
gets through until the client's TCP gives up (tcp_retries2).

If vport - 1 is also in the ports module parameter (for example
ports=2120,2121), would active mode hang for every client of that service?
A client that sends PORT/EPRT with port 0 would hang its own session the
same way.

The check added to ip_vs_ftp_out() above behaves similarly. It returns 0
while cp->app_data is still IP_VS_FTP_PASV or IP_VS_FTP_EPSV, and
tcp_snat_handler() passes that up:

    if (!(ret = ip_vs_app_pkt_out(cp, skb, iph)))
        return 0;

handle_response() then drops the 227/229 reply. The server retransmits
it, the same port is parsed again, and the reply is dropped again.

The existing ret = 0 path at the end of ip_vs_ftp_out() sets
cp->app_data = IP_VS_FTP_ACTIVE before it returns, so it never re-parses
the reply. The ip_vs_conn_new() failure return is transient, which fits
the existing "hopefully it will succeed on the retransmitted packet"
comment. The new checks reject the same input every time.

Could ip_vs_ftp_in() return 1 here without creating the data connection,
since nothing is mangled on that path anyway? And could ip_vs_ftp_out()
set cp->app_data to IP_VS_FTP_ACTIVE before rejecting, so that only the
data connection fails?

>  	/* Passive mode off */
>  	cp->app_data = (void *) IP_VS_FTP_ACTIVE;
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org