| Message ID | b5c5a01607eabe55a6a5a27a9ed40a0ac8ec65fe.1790146910.git.zihanx@nebusec.ai |
|---|---|
| State | New |
| Headers | show |
| Series | ipvs: avoid stack overflow from recursive connection expiration | expand |
Thank you for your contribution! Sashiko AI review found more severe issue(s) in other patches of this series. This mail is only an FYI, in case you want to address the nit picks below when you respin. Low priority / nit picks for your consideration: - [Low] The commit message is imprecise, and for a Cc: stable fix it leaves out the impact. -- ipvs: reject FTP control ports as data ports With this patch, ip_vs_ftp_out() will not create a passive-mode data connection if the port advertised by the server is zero or is one of the configured FTP control ports. It also changes ip_vs_ftp_in() so that active-mode handling is refused when the client port is zero or when vport - 1 is a configured control port. > ip_vs_ftp_out() creates a wildcard data connection from the > server-advertised passive port. If that port is one of the configured FTP > control ports, ip_vs_conn_new() binds the FTP helper to the new connection > again. A subsequent wildcard lookup can then extend a controlled-connection > chain. > > Reject zero and configured control ports before creating passive > connections. For active mode, reject a zero client port and a data port > derived from a configured control port. [Severity: Low] This is tagged for stable. Could the commit message say what goes wrong once the controlled-connection chain gets extended? The earlier patch in this series, "ipvs: avoid stack overflow from recursive connection expiration", points to the actual consequence. That consequence is chains of controlled connections pinned through n_control, and stack exhaustion from recursive ip_vs_conn_expire(). This message does not mention that, or any dependency on that patch. The message also doesn't say where the trigger comes from. In passive mode the port comes from the real server's 227/229 reply. The active case needs an admin configuration where vport - 1 is also a configured FTP port. Is the active-mode sentence accurate? Every active-mode data port is derived from a configured control port. The data port is vport - 1, and vport is always a configured FTP port, because tcp_app_conn_bind() only binds the helper when: if (inc->port == cp->vport) { Read literally, the sentence says all active transfers are rejected. The code in ip_vs_ftp_in() only rejects them when the derived port is itself a configured control port: if (!port || is_control_port(ntohs(cp->vport) - 1)) return 0; Could this be reworded along the lines of "reject a derived data port (vport - 1) that is itself a configured 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 @@ -62,6 +62,17 @@ static unsigned short ports[IP_VS_APP_MAX_PORTS] = {21, 0}; module_param_array(ports, ushort, &ports_count, 0444); MODULE_PARM_DESC(ports, "Ports to monitor for FTP control commands"); +static bool is_control_port(u16 port) +{ + unsigned int i; + + for (i = 0; i < ports_count; i++) { + if (ports[i] == port) + return true; + } + return false; +} + static char *ip_vs_ftp_data_ptr(struct sk_buff *skb, struct ip_vs_iphdr *ipvsh) { @@ -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; + /* Passive mode off */ cp->app_data = (void *) IP_VS_FTP_ACTIVE;
ip_vs_ftp_out() creates a wildcard data connection from the server-advertised passive port. If that port is one of the configured FTP control ports, ip_vs_conn_new() binds the FTP helper to the new connection again. A subsequent wildcard lookup can then extend a controlled-connection chain. Reject zero and configured control ports before creating passive connections. For active mode, reject a zero client port and a data port derived from a configured control port. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Reported-by: Vega <vega@nebusec.ai> Assisted-by: LLM Co-developed-by: Luxing Yin <root@tr0jan.top> Signed-off-by: Luxing Yin <root@tr0jan.top> Signed-off-by: Zihan Xi <zihanx@nebusec.ai> --- changes in v4: - Resend the FTP helper fix as patch 3/3 with the generic cleanup fixes. - v3 Link: https://lore.kernel.org/all/cover.1789877273.git.zihanx@nebusec.ai/ changes in v3: - Keep the active-mode guard for configured FTP control ports. - v2 Link: https://lore.kernel.org/all/cover.1789435989.git.zihanx@nebusec.ai/ changes in v2: - Add the active-mode check for a data port derived from a configured control port. - v1 Link: https://lore.kernel.org/all/cover.1789110326.git.zihanx@nebusec.ai/ net/netfilter/ipvs/ip_vs_ftp.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)