| Message ID | 20250806203831.1815150-1-mkp@redhat.com |
|---|---|
| State | Superseded |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev,v2] conntrack: Add support for IPv4 EPRT and EPSV. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/cirrus-robot | success | cirrus build: passed |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 8/6/25 10:38 PM, Mike Pattrick via dev wrote: > Recent versions of Curl combine the EPSV and EPRT commands with IPv4 > connections instead of the PASV and PORT commands. EPSV and EPRT were > added to FTP for IPv6 support but these commands also support IPv4. Most > software still uses the old PASV and PORT commands in IPv4 connections > but recent versions of curl will default to EPSV and EPRT for all > connections. This patch adds support for these extended commands, and > added tests for both with and without them. > > Reported-at: https://issues.redhat.com/browse/FDP-907 > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > NEWS | 3 + > lib/conntrack.c | 189 +++++++++++++++++++++++----------- > tests/system-common-macros.at | 4 +- > tests/system-traffic.at | 10 +- > 4 files changed, 142 insertions(+), 64 deletions(-) > > diff --git a/NEWS b/NEWS > index b5d565ecc..72782f9cf 100644 > --- a/NEWS > +++ b/NEWS > @@ -48,6 +48,9 @@ v3.6.0 - xx xxx xxxx > the OVS distribution in the 3.0 release and is no longer present in > any supported versions of OVS. The remaining documentation of this > kernel module relates to topics for older releases of OVS. > + - Userspace datapath: > + * Conntrack now supports the FTP commands EPSV and EPRT with IPv4 > + connections, instead of limiting these commands to IPv6 only. Note: this entry should be in the Post-v3.6.0 section instead. Best regards, Ilya Maximets.
Mike Pattrick via dev <ovs-dev@openvswitch.org> writes: > Recent versions of Curl combine the EPSV and EPRT commands with IPv4 > connections instead of the PASV and PORT commands. EPSV and EPRT were > added to FTP for IPv6 support but these commands also support IPv4. Most > software still uses the old PASV and PORT commands in IPv4 connections > but recent versions of curl will default to EPSV and EPRT for all > connections. This patch adds support for these extended commands, and > added tests for both with and without them. > > Reported-at: https://issues.redhat.com/browse/FDP-907 > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > NEWS | 3 + > lib/conntrack.c | 189 +++++++++++++++++++++++----------- > tests/system-common-macros.at | 4 +- > tests/system-traffic.at | 10 +- > 4 files changed, 142 insertions(+), 64 deletions(-) > > diff --git a/NEWS b/NEWS > index b5d565ecc..72782f9cf 100644 > --- a/NEWS > +++ b/NEWS > @@ -48,6 +48,9 @@ v3.6.0 - xx xxx xxxx > the OVS distribution in the 3.0 release and is no longer present in > any supported versions of OVS. The remaining documentation of this > kernel module relates to topics for older releases of OVS. > + - Userspace datapath: > + * Conntrack now supports the FTP commands EPSV and EPRT with IPv4 > + connections, instead of limiting these commands to IPv6 only. > > > v3.5.0 - 17 Feb 2025 > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 00346a0be..9234cace1 100644 > --- a/lib/conntrack.c > +++ b/lib/conntrack.c > @@ -192,6 +192,8 @@ static alg_helper alg_helpers[] = { > #define FTP_PORT_CMD "PORT" > /* FTP pasv string used in passive mode. */ > #define FTP_PASV_REPLY_CODE "227" > +/* FTP epsv string used in passive mode. */ > +#define FTP_EPSV_REPLY_CODE "229" > /* Maximum decimal digits for port in FTP command. > * The port is represented as two 3 digit numbers with the > * high part a multiple of 256. */ > @@ -203,6 +205,8 @@ static alg_helper alg_helpers[] = { > #define FTP_EPSV_REPLY "EXTENDED PASSIVE" > /* Maximum decimal digits for port in FTP extended command. */ > #define MAX_EXT_FTP_PORT_DGTS 5 > +/* FTP extended command code for IPv4. */ > +#define FTP_AF_V4 '1' > /* FTP extended command code for IPv6. */ > #define FTP_AF_V6 '2' > /* Used to indicate a wildcard L4 source port number for ALGs. > @@ -3269,10 +3273,16 @@ static int > repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep, > char *ftp_data_start, > size_t addr_offset_from_ftp_data_start, > - size_t addr_size OVS_UNUSED) > + size_t addr_size) > { > enum { MAX_FTP_V4_NAT_DELTA = 8 }; > > + /* EPSV mode. */ > + if (addr_offset_from_ftp_data_start == 0 && > + addr_size == 0) { > + return 0; > + } > + > /* Do conservative check for pathological MTU usage. */ > uint32_t orig_used_size = dp_packet_size(pkt); > if (orig_used_size + MAX_FTP_V4_NAT_DELTA > > @@ -3345,8 +3355,11 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx, > } > } else { > if (strncasecmp(ftp_msg, FTP_PORT_CMD, strlen(FTP_PORT_CMD)) && > + strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD)) && > strncasecmp(ftp_msg, FTP_PASV_REPLY_CODE, > - strlen(FTP_PASV_REPLY_CODE))) { > + strlen(FTP_PASV_REPLY_CODE)) && > + strncasecmp(ftp_msg, FTP_EPSV_REPLY_CODE, > + strlen(FTP_EPSV_REPLY_CODE))) { > return CT_FTP_CTL_OTHER; > } > } > @@ -3370,11 +3383,23 @@ process_ftp_ctl_v4(struct conntrack *ct, > char ftp_msg[LARGEST_FTP_MSG_OF_INTEREST + 1] = {0}; > get_ftp_ctl_msg(pkt, ftp_msg); > char *ftp = ftp_msg; > + struct in_addr ip_addr; > enum ct_alg_mode mode; > + bool extended = false; > + char delim = 0; > > if (!strncasecmp(ftp, FTP_PORT_CMD, strlen(FTP_PORT_CMD))) { > ftp = ftp_msg + strlen(FTP_PORT_CMD); > mode = CT_FTP_MODE_ACTIVE; > + } else if (!strncasecmp(ftp, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD))) { > + ftp = ftp_msg + strlen(FTP_EPRT_CMD); > + mode = CT_FTP_MODE_ACTIVE; > + extended = true; > + } else if (!strncasecmp(ftp, FTP_EPSV_REPLY_CODE, > + strlen(FTP_EPSV_REPLY_CODE))) { > + ftp = ftp_msg + strlen(FTP_EPSV_REPLY_CODE); > + mode = CT_FTP_MODE_PASSIVE; > + extended = true; > } else { > ftp = ftp_msg + strlen(FTP_PASV_REPLY_CODE); > mode = CT_FTP_MODE_PASSIVE; > @@ -3392,71 +3417,117 @@ process_ftp_ctl_v4(struct conntrack *ct, > return CT_FTP_CTL_INVALID; > } > > - char *ip_addr_start = ftp; > - *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; > + /* EPRT, skip address family. */ We're not exactly just blindly skipping - maybe 'verify'. > + if (extended && mode == CT_FTP_MODE_ACTIVE) { > + if (*ftp != FTP_AF_V4) { I think we should also add check for '|| isdigit(ftp[1])' RFC 2428 only specifies [1, 2] as values for now. But a future revision that somehow adds '10+' would pass this check. > + return CT_FTP_CTL_INVALID; > + } > + delim = ftp[1]; > + ftp = skip_non_digits(ftp + 1); > + if (*ftp == 0) { > + return CT_FTP_CTL_INVALID; > + } > + } > + > + if (!extended || mode == CT_FTP_MODE_ACTIVE) { > + char *ip_addr_start = ftp; > + *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; > > - uint8_t comma_count = 0; > - while (comma_count < 4 && *ftp) { > - if (*ftp == ',') { > - comma_count++; > - if (comma_count == 4) { > - *ftp = 0; > + uint8_t delim_count = 0; > + while (delim_count < 4 && *ftp) { > + if (extended) { > + if (*ftp == '.') { Maybe a nit - feel free to not do it, but this use of '.' and ',' to mean the delimiter open coded are a bit difficult to understand on first glance. Especially because this seems like a re-implementation of repl_bytes and we just want the count. Maybe a useful preparation patch would be to update repl_bytes to support this to reduce the amount of open coding here. But that would need an additional patch in front of this. > + delim_count++; > + } else if (*ftp == delim) { > + delim_count++; > + *ftp = 0; > + ftp++; > + break; > + } > } else { > - *ftp = '.'; > + if (*ftp == ',') { > + delim_count++; > + if (delim_count == 4) { > + *ftp = 0; > + } else { > + *ftp = '.'; > + } > + } > } > + ftp++; > + } > + if (delim_count != 4) { > + return CT_FTP_CTL_INVALID; > } > - ftp++; > - } > - if (comma_count != 4) { > - return CT_FTP_CTL_INVALID; > - } > > - struct in_addr ip_addr; > - int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); > - if (rc2 != 1) { > - return CT_FTP_CTL_INVALID; > + int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); > + if (rc2 != 1) { > + return CT_FTP_CTL_INVALID; > + } > + > + *addr_size = ftp - ip_addr_start - 1; > + } else { > + *addr_size = 0; > + *addr_offset_from_ftp_data_start = 0; > } > > - *addr_size = ftp - ip_addr_start - 1; > char *save_ftp = ftp; > - ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); > - if (!ftp) { > - return CT_FTP_CTL_INVALID; > - } > - int value; > - if (!str_to_int(save_ftp, 10, &value)) { > - return CT_FTP_CTL_INVALID; > - } > + uint16_t port_hs; > > - /* This is derived from the L4 port maximum is 65535. */ > - if (value > 255) { > - return CT_FTP_CTL_INVALID; > - } > + if (!extended) { > + ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); > + if (!ftp) { > + return CT_FTP_CTL_INVALID; > + } > + int value; > + if (!str_to_int(save_ftp, 10, &value)) { > + return CT_FTP_CTL_INVALID; > + } > > - uint16_t port_hs = value; > - port_hs <<= 8; > + /* This is derived from the L4 port maximum is 65535. */ > + if (value > 255) { > + return CT_FTP_CTL_INVALID; > + } > > - /* Skip over comma. */ > - ftp++; > - save_ftp = ftp; > - bool digit_found = false; > - while (isdigit(*ftp)) { > + port_hs = value; > + port_hs <<= 8; > + > + /* Skip over comma. */ > ftp++; > - digit_found = true; > - } > - if (!digit_found) { > - return CT_FTP_CTL_INVALID; > - } > - *ftp = 0; > - if (!str_to_int(save_ftp, 10, &value)) { > - return CT_FTP_CTL_INVALID; > - } > + save_ftp = ftp; > + bool digit_found = false; > + while (isdigit(*ftp)) { > + ftp++; > + digit_found = true; > + } > + if (!digit_found) { > + return CT_FTP_CTL_INVALID; > + } > + *ftp = 0; > + if (!str_to_int(save_ftp, 10, &value)) { > + return CT_FTP_CTL_INVALID; > + } > > - if (value > 255) { > - return CT_FTP_CTL_INVALID; > + if (value > 255) { > + return CT_FTP_CTL_INVALID; > + } > + > + port_hs |= value; > + } else { > + ftp = terminate_number_str(ftp, MAX_EXT_FTP_PORT_DGTS); > + if (!ftp) { > + return CT_FTP_CTL_INVALID; > + } > + int value; > + if (!str_to_int(save_ftp, 10, &value)) { > + return CT_FTP_CTL_INVALID; > + } > + if (value > UINT16_MAX) { > + return CT_FTP_CTL_INVALID; > + } > + port_hs = (uint16_t) value; > } > > - port_hs |= value; > ovs_be16 port = htons(port_hs); > ovs_be32 conn_ipv4_addr; > > @@ -3478,12 +3549,14 @@ process_ftp_ctl_v4(struct conntrack *ct, > OVS_NOT_REACHED(); > } > > - ovs_be32 ftp_ipv4_addr; > - ftp_ipv4_addr = ip_addr.s_addr; > - /* Although most servers will block this exploit, there may be some > - * less well managed. */ > - if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { > - return CT_FTP_CTL_INVALID; > + if (!extended || mode == CT_FTP_MODE_ACTIVE) { Does the 'extended' check matter here? If I remember right, PASV / EPASV commands don't provide an address for the server to connect - only PORT/EPORT (or active mode connections). Also, this previously was being checked for all connection modes (active / passive), and now we will isolate only EPASV connections. But maybe we should just not change this part for now. WDYT? > + ovs_be32 ftp_ipv4_addr; > + ftp_ipv4_addr = ip_addr.s_addr; > + /* Although most servers will block this exploit, there may be some > + * less well managed. */ > + if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { > + return CT_FTP_CTL_INVALID; > + } > } > > expectation_create(ct, port, conn_for_expectation, > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 7f029dbb0..e4862231a 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -280,7 +280,7 @@ m4_define([OVS_GET_HTTP], > # > m4_define([OVS_GET_FTP], > [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused \ > - --disable-epsv -v $2] > + -v $2] > ) > > # OVS_GET_FTP_ACTIVE([url], [optional_curl_arguments]) > @@ -289,7 +289,7 @@ m4_define([OVS_GET_FTP], > # > m4_define([OVS_GET_FTP_ACTIVE], > [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused -v \ > - --ftp-port - --disable-eprt $2] > + --ftp-port - $2] > ) > > # OVS_CHECK_FIREWALL() > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 0f3acef3f..e5a1131e6 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -8204,8 +8204,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2 >/dev/null]) > > OVS_START_L7([at_ns1], [ftp]) > > -dnl FTP requests from p0->p1 should work fine. > -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2]), [0], [ignore], [ignore]) > +dnl FTP requests from p0->p1 should work fine, with and without EPSV. > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--epsv]), [0], [ignore], [ignore]) > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--disable-epsv]), [0], [ignore], [ignore]) > > dnl Discards CLOSE_WAIT and CLOSING > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl > @@ -8384,8 +8385,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null]) > > OVS_START_L7([at_ns1], [ftp]) > > -dnl FTP requests from p0->p1 should work fine. > -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2]), [0], [ignore], [ignore]) > +dnl FTP requests from p0->p1 should work fine with and without EPRT. > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--eprt]), [0], [ignore], [ignore]) > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--disable-eprt]), [0], [ignore], [ignore]) > > dnl Discards CLOSE_WAIT and CLOSING > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
On Wed, Aug 13, 2025 at 4:23 PM Aaron Conole <aconole@redhat.com> wrote: > > Mike Pattrick via dev <ovs-dev@openvswitch.org> writes: > > > Recent versions of Curl combine the EPSV and EPRT commands with IPv4 > > connections instead of the PASV and PORT commands. EPSV and EPRT were > > added to FTP for IPv6 support but these commands also support IPv4. Most > > software still uses the old PASV and PORT commands in IPv4 connections > > but recent versions of curl will default to EPSV and EPRT for all > > connections. This patch adds support for these extended commands, and > > added tests for both with and without them. > > > > Reported-at: https://issues.redhat.com/browse/FDP-907 > > Reported-by: Eelco Chaudron <echaudro@redhat.com> > > Signed-off-by: Mike Pattrick <mkp@redhat.com> > > --- > > NEWS | 3 + > > lib/conntrack.c | 189 +++++++++++++++++++++++----------- > > tests/system-common-macros.at | 4 +- > > tests/system-traffic.at | 10 +- > > 4 files changed, 142 insertions(+), 64 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index b5d565ecc..72782f9cf 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -48,6 +48,9 @@ v3.6.0 - xx xxx xxxx > > the OVS distribution in the 3.0 release and is no longer present in > > any supported versions of OVS. The remaining documentation of this > > kernel module relates to topics for older releases of OVS. > > + - Userspace datapath: > > + * Conntrack now supports the FTP commands EPSV and EPRT with IPv4 > > + connections, instead of limiting these commands to IPv6 only. > > > > > > v3.5.0 - 17 Feb 2025 > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 00346a0be..9234cace1 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -192,6 +192,8 @@ static alg_helper alg_helpers[] = { > > #define FTP_PORT_CMD "PORT" > > /* FTP pasv string used in passive mode. */ > > #define FTP_PASV_REPLY_CODE "227" > > +/* FTP epsv string used in passive mode. */ > > +#define FTP_EPSV_REPLY_CODE "229" > > /* Maximum decimal digits for port in FTP command. > > * The port is represented as two 3 digit numbers with the > > * high part a multiple of 256. */ > > @@ -203,6 +205,8 @@ static alg_helper alg_helpers[] = { > > #define FTP_EPSV_REPLY "EXTENDED PASSIVE" > > /* Maximum decimal digits for port in FTP extended command. */ > > #define MAX_EXT_FTP_PORT_DGTS 5 > > +/* FTP extended command code for IPv4. */ > > +#define FTP_AF_V4 '1' > > /* FTP extended command code for IPv6. */ > > #define FTP_AF_V6 '2' > > /* Used to indicate a wildcard L4 source port number for ALGs. > > @@ -3269,10 +3273,16 @@ static int > > repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep, > > char *ftp_data_start, > > size_t addr_offset_from_ftp_data_start, > > - size_t addr_size OVS_UNUSED) > > + size_t addr_size) > > { > > enum { MAX_FTP_V4_NAT_DELTA = 8 }; > > > > + /* EPSV mode. */ > > + if (addr_offset_from_ftp_data_start == 0 && > > + addr_size == 0) { > > + return 0; > > + } > > + > > /* Do conservative check for pathological MTU usage. */ > > uint32_t orig_used_size = dp_packet_size(pkt); > > if (orig_used_size + MAX_FTP_V4_NAT_DELTA > > > @@ -3345,8 +3355,11 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx, > > } > > } else { > > if (strncasecmp(ftp_msg, FTP_PORT_CMD, strlen(FTP_PORT_CMD)) && > > + strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD)) && > > strncasecmp(ftp_msg, FTP_PASV_REPLY_CODE, > > - strlen(FTP_PASV_REPLY_CODE))) { > > + strlen(FTP_PASV_REPLY_CODE)) && > > + strncasecmp(ftp_msg, FTP_EPSV_REPLY_CODE, > > + strlen(FTP_EPSV_REPLY_CODE))) { > > return CT_FTP_CTL_OTHER; > > } > > } > > @@ -3370,11 +3383,23 @@ process_ftp_ctl_v4(struct conntrack *ct, > > char ftp_msg[LARGEST_FTP_MSG_OF_INTEREST + 1] = {0}; > > get_ftp_ctl_msg(pkt, ftp_msg); > > char *ftp = ftp_msg; > > + struct in_addr ip_addr; > > enum ct_alg_mode mode; > > + bool extended = false; > > + char delim = 0; > > > > if (!strncasecmp(ftp, FTP_PORT_CMD, strlen(FTP_PORT_CMD))) { > > ftp = ftp_msg + strlen(FTP_PORT_CMD); > > mode = CT_FTP_MODE_ACTIVE; > > + } else if (!strncasecmp(ftp, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD))) { > > + ftp = ftp_msg + strlen(FTP_EPRT_CMD); > > + mode = CT_FTP_MODE_ACTIVE; > > + extended = true; > > + } else if (!strncasecmp(ftp, FTP_EPSV_REPLY_CODE, > > + strlen(FTP_EPSV_REPLY_CODE))) { > > + ftp = ftp_msg + strlen(FTP_EPSV_REPLY_CODE); > > + mode = CT_FTP_MODE_PASSIVE; > > + extended = true; > > } else { > > ftp = ftp_msg + strlen(FTP_PASV_REPLY_CODE); > > mode = CT_FTP_MODE_PASSIVE; > > @@ -3392,71 +3417,117 @@ process_ftp_ctl_v4(struct conntrack *ct, > > return CT_FTP_CTL_INVALID; > > } > > > > - char *ip_addr_start = ftp; > > - *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; > > + /* EPRT, skip address family. */ > > We're not exactly just blindly skipping - maybe 'verify'. > > > + if (extended && mode == CT_FTP_MODE_ACTIVE) { > > + if (*ftp != FTP_AF_V4) { > > I think we should also add check for '|| isdigit(ftp[1])' > > RFC 2428 only specifies [1, 2] as values for now. But a future revision > that somehow adds '10+' would pass this check. > > > + return CT_FTP_CTL_INVALID; > > + } > > + delim = ftp[1]; > > + ftp = skip_non_digits(ftp + 1); > > + if (*ftp == 0) { > > + return CT_FTP_CTL_INVALID; > > + } > > + } > > + > > + if (!extended || mode == CT_FTP_MODE_ACTIVE) { > > + char *ip_addr_start = ftp; > > + *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; > > > > - uint8_t comma_count = 0; > > - while (comma_count < 4 && *ftp) { > > - if (*ftp == ',') { > > - comma_count++; > > - if (comma_count == 4) { > > - *ftp = 0; > > + uint8_t delim_count = 0; > > + while (delim_count < 4 && *ftp) { > > + if (extended) { > > + if (*ftp == '.') { > > Maybe a nit - feel free to not do it, but this use of '.' and ',' to > mean the delimiter open coded are a bit difficult to understand on first > glance. Especially because this seems like a re-implementation of > repl_bytes and we just want the count. Maybe a useful preparation patch > would be to update repl_bytes to support this to reduce the amount of > open coding here. > > But that would need an additional patch in front of this. > > > + delim_count++; > > + } else if (*ftp == delim) { > > + delim_count++; > > + *ftp = 0; > > + ftp++; > > + break; > > + } > > } else { > > - *ftp = '.'; > > + if (*ftp == ',') { > > + delim_count++; > > + if (delim_count == 4) { > > + *ftp = 0; > > + } else { > > + *ftp = '.'; > > + } > > + } > > } > > + ftp++; > > + } > > + if (delim_count != 4) { > > + return CT_FTP_CTL_INVALID; > > } > > - ftp++; > > - } > > - if (comma_count != 4) { > > - return CT_FTP_CTL_INVALID; > > - } > > > > - struct in_addr ip_addr; > > - int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); > > - if (rc2 != 1) { > > - return CT_FTP_CTL_INVALID; > > + int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); > > + if (rc2 != 1) { > > + return CT_FTP_CTL_INVALID; > > + } > > + > > + *addr_size = ftp - ip_addr_start - 1; > > + } else { > > + *addr_size = 0; > > + *addr_offset_from_ftp_data_start = 0; > > } > > > > - *addr_size = ftp - ip_addr_start - 1; > > char *save_ftp = ftp; > > - ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); > > - if (!ftp) { > > - return CT_FTP_CTL_INVALID; > > - } > > - int value; > > - if (!str_to_int(save_ftp, 10, &value)) { > > - return CT_FTP_CTL_INVALID; > > - } > > + uint16_t port_hs; > > > > - /* This is derived from the L4 port maximum is 65535. */ > > - if (value > 255) { > > - return CT_FTP_CTL_INVALID; > > - } > > + if (!extended) { > > + ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); > > + if (!ftp) { > > + return CT_FTP_CTL_INVALID; > > + } > > + int value; > > + if (!str_to_int(save_ftp, 10, &value)) { > > + return CT_FTP_CTL_INVALID; > > + } > > > > - uint16_t port_hs = value; > > - port_hs <<= 8; > > + /* This is derived from the L4 port maximum is 65535. */ > > + if (value > 255) { > > + return CT_FTP_CTL_INVALID; > > + } > > > > - /* Skip over comma. */ > > - ftp++; > > - save_ftp = ftp; > > - bool digit_found = false; > > - while (isdigit(*ftp)) { > > + port_hs = value; > > + port_hs <<= 8; > > + > > + /* Skip over comma. */ > > ftp++; > > - digit_found = true; > > - } > > - if (!digit_found) { > > - return CT_FTP_CTL_INVALID; > > - } > > - *ftp = 0; > > - if (!str_to_int(save_ftp, 10, &value)) { > > - return CT_FTP_CTL_INVALID; > > - } > > + save_ftp = ftp; > > + bool digit_found = false; > > + while (isdigit(*ftp)) { > > + ftp++; > > + digit_found = true; > > + } > > + if (!digit_found) { > > + return CT_FTP_CTL_INVALID; > > + } > > + *ftp = 0; > > + if (!str_to_int(save_ftp, 10, &value)) { > > + return CT_FTP_CTL_INVALID; > > + } > > > > - if (value > 255) { > > - return CT_FTP_CTL_INVALID; > > + if (value > 255) { > > + return CT_FTP_CTL_INVALID; > > + } > > + > > + port_hs |= value; > > + } else { > > + ftp = terminate_number_str(ftp, MAX_EXT_FTP_PORT_DGTS); > > + if (!ftp) { > > + return CT_FTP_CTL_INVALID; > > + } > > + int value; > > + if (!str_to_int(save_ftp, 10, &value)) { > > + return CT_FTP_CTL_INVALID; > > + } > > + if (value > UINT16_MAX) { > > + return CT_FTP_CTL_INVALID; > > + } > > + port_hs = (uint16_t) value; > > } > > > > - port_hs |= value; > > ovs_be16 port = htons(port_hs); > > ovs_be32 conn_ipv4_addr; > > > > @@ -3478,12 +3549,14 @@ process_ftp_ctl_v4(struct conntrack *ct, > > OVS_NOT_REACHED(); > > } > > > > - ovs_be32 ftp_ipv4_addr; > > - ftp_ipv4_addr = ip_addr.s_addr; > > - /* Although most servers will block this exploit, there may be some > > - * less well managed. */ > > - if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { > > - return CT_FTP_CTL_INVALID; > > + if (!extended || mode == CT_FTP_MODE_ACTIVE) { > > Does the 'extended' check matter here? If I remember right, PASV / > EPASV commands don't provide an address for the server to connect - only > PORT/EPORT (or active mode connections). Also, this previously was > being checked for all connection modes (active / passive), and now we > will isolate only EPASV connections. But maybe we should just not > change this part for now. A PASV response looks like: Entering Passive Mode (h1,h2,h3,h4,p1,p2). An EPSV response looks like: Entering Extended Passive Mode (|||port|) So they really aren't compatible, epsv is the only command in the series that doesn't include the address. So the different handling is required, but I could invert the logic to make that more clear and leave a comment in the next version. The other requests sound fine, will send a new revision. -M > > WDYT? > > > + ovs_be32 ftp_ipv4_addr; > > + ftp_ipv4_addr = ip_addr.s_addr; > > + /* Although most servers will block this exploit, there may be some > > + * less well managed. */ > > + if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { > > + return CT_FTP_CTL_INVALID; > > + } > > } > > > > expectation_create(ct, port, conn_for_expectation, > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > > index 7f029dbb0..e4862231a 100644 > > --- a/tests/system-common-macros.at > > +++ b/tests/system-common-macros.at > > @@ -280,7 +280,7 @@ m4_define([OVS_GET_HTTP], > > # > > m4_define([OVS_GET_FTP], > > [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused \ > > - --disable-epsv -v $2] > > + -v $2] > > ) > > > > # OVS_GET_FTP_ACTIVE([url], [optional_curl_arguments]) > > @@ -289,7 +289,7 @@ m4_define([OVS_GET_FTP], > > # > > m4_define([OVS_GET_FTP_ACTIVE], > > [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused -v \ > > - --ftp-port - --disable-eprt $2] > > + --ftp-port - $2] > > ) > > > > # OVS_CHECK_FIREWALL() > > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > > index 0f3acef3f..e5a1131e6 100644 > > --- a/tests/system-traffic.at > > +++ b/tests/system-traffic.at > > @@ -8204,8 +8204,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2 >/dev/null]) > > > > OVS_START_L7([at_ns1], [ftp]) > > > > -dnl FTP requests from p0->p1 should work fine. > > -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2]), [0], [ignore], [ignore]) > > +dnl FTP requests from p0->p1 should work fine, with and without EPSV. > > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--epsv]), [0], [ignore], [ignore]) > > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--disable-epsv]), [0], [ignore], [ignore]) > > > > dnl Discards CLOSE_WAIT and CLOSING > > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl > > @@ -8384,8 +8385,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null]) > > > > OVS_START_L7([at_ns1], [ftp]) > > > > -dnl FTP requests from p0->p1 should work fine. > > -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2]), [0], [ignore], [ignore]) > > +dnl FTP requests from p0->p1 should work fine with and without EPRT. > > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--eprt]), [0], [ignore], [ignore]) > > +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--disable-eprt]), [0], [ignore], [ignore]) > > > > dnl Discards CLOSE_WAIT and CLOSING > > AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl >
diff --git a/NEWS b/NEWS index b5d565ecc..72782f9cf 100644 --- a/NEWS +++ b/NEWS @@ -48,6 +48,9 @@ v3.6.0 - xx xxx xxxx the OVS distribution in the 3.0 release and is no longer present in any supported versions of OVS. The remaining documentation of this kernel module relates to topics for older releases of OVS. + - Userspace datapath: + * Conntrack now supports the FTP commands EPSV and EPRT with IPv4 + connections, instead of limiting these commands to IPv6 only. v3.5.0 - 17 Feb 2025 diff --git a/lib/conntrack.c b/lib/conntrack.c index 00346a0be..9234cace1 100644 --- a/lib/conntrack.c +++ b/lib/conntrack.c @@ -192,6 +192,8 @@ static alg_helper alg_helpers[] = { #define FTP_PORT_CMD "PORT" /* FTP pasv string used in passive mode. */ #define FTP_PASV_REPLY_CODE "227" +/* FTP epsv string used in passive mode. */ +#define FTP_EPSV_REPLY_CODE "229" /* Maximum decimal digits for port in FTP command. * The port is represented as two 3 digit numbers with the * high part a multiple of 256. */ @@ -203,6 +205,8 @@ static alg_helper alg_helpers[] = { #define FTP_EPSV_REPLY "EXTENDED PASSIVE" /* Maximum decimal digits for port in FTP extended command. */ #define MAX_EXT_FTP_PORT_DGTS 5 +/* FTP extended command code for IPv4. */ +#define FTP_AF_V4 '1' /* FTP extended command code for IPv6. */ #define FTP_AF_V6 '2' /* Used to indicate a wildcard L4 source port number for ALGs. @@ -3269,10 +3273,16 @@ static int repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32 v4_addr_rep, char *ftp_data_start, size_t addr_offset_from_ftp_data_start, - size_t addr_size OVS_UNUSED) + size_t addr_size) { enum { MAX_FTP_V4_NAT_DELTA = 8 }; + /* EPSV mode. */ + if (addr_offset_from_ftp_data_start == 0 && + addr_size == 0) { + return 0; + } + /* Do conservative check for pathological MTU usage. */ uint32_t orig_used_size = dp_packet_size(pkt); if (orig_used_size + MAX_FTP_V4_NAT_DELTA > @@ -3345,8 +3355,11 @@ detect_ftp_ctl_type(const struct conn_lookup_ctx *ctx, } } else { if (strncasecmp(ftp_msg, FTP_PORT_CMD, strlen(FTP_PORT_CMD)) && + strncasecmp(ftp_msg, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD)) && strncasecmp(ftp_msg, FTP_PASV_REPLY_CODE, - strlen(FTP_PASV_REPLY_CODE))) { + strlen(FTP_PASV_REPLY_CODE)) && + strncasecmp(ftp_msg, FTP_EPSV_REPLY_CODE, + strlen(FTP_EPSV_REPLY_CODE))) { return CT_FTP_CTL_OTHER; } } @@ -3370,11 +3383,23 @@ process_ftp_ctl_v4(struct conntrack *ct, char ftp_msg[LARGEST_FTP_MSG_OF_INTEREST + 1] = {0}; get_ftp_ctl_msg(pkt, ftp_msg); char *ftp = ftp_msg; + struct in_addr ip_addr; enum ct_alg_mode mode; + bool extended = false; + char delim = 0; if (!strncasecmp(ftp, FTP_PORT_CMD, strlen(FTP_PORT_CMD))) { ftp = ftp_msg + strlen(FTP_PORT_CMD); mode = CT_FTP_MODE_ACTIVE; + } else if (!strncasecmp(ftp, FTP_EPRT_CMD, strlen(FTP_EPRT_CMD))) { + ftp = ftp_msg + strlen(FTP_EPRT_CMD); + mode = CT_FTP_MODE_ACTIVE; + extended = true; + } else if (!strncasecmp(ftp, FTP_EPSV_REPLY_CODE, + strlen(FTP_EPSV_REPLY_CODE))) { + ftp = ftp_msg + strlen(FTP_EPSV_REPLY_CODE); + mode = CT_FTP_MODE_PASSIVE; + extended = true; } else { ftp = ftp_msg + strlen(FTP_PASV_REPLY_CODE); mode = CT_FTP_MODE_PASSIVE; @@ -3392,71 +3417,117 @@ process_ftp_ctl_v4(struct conntrack *ct, return CT_FTP_CTL_INVALID; } - char *ip_addr_start = ftp; - *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; + /* EPRT, skip address family. */ + if (extended && mode == CT_FTP_MODE_ACTIVE) { + if (*ftp != FTP_AF_V4) { + return CT_FTP_CTL_INVALID; + } + delim = ftp[1]; + ftp = skip_non_digits(ftp + 1); + if (*ftp == 0) { + return CT_FTP_CTL_INVALID; + } + } + + if (!extended || mode == CT_FTP_MODE_ACTIVE) { + char *ip_addr_start = ftp; + *addr_offset_from_ftp_data_start = ip_addr_start - ftp_msg; - uint8_t comma_count = 0; - while (comma_count < 4 && *ftp) { - if (*ftp == ',') { - comma_count++; - if (comma_count == 4) { - *ftp = 0; + uint8_t delim_count = 0; + while (delim_count < 4 && *ftp) { + if (extended) { + if (*ftp == '.') { + delim_count++; + } else if (*ftp == delim) { + delim_count++; + *ftp = 0; + ftp++; + break; + } } else { - *ftp = '.'; + if (*ftp == ',') { + delim_count++; + if (delim_count == 4) { + *ftp = 0; + } else { + *ftp = '.'; + } + } } + ftp++; + } + if (delim_count != 4) { + return CT_FTP_CTL_INVALID; } - ftp++; - } - if (comma_count != 4) { - return CT_FTP_CTL_INVALID; - } - struct in_addr ip_addr; - int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); - if (rc2 != 1) { - return CT_FTP_CTL_INVALID; + int rc2 = inet_pton(AF_INET, ip_addr_start, &ip_addr); + if (rc2 != 1) { + return CT_FTP_CTL_INVALID; + } + + *addr_size = ftp - ip_addr_start - 1; + } else { + *addr_size = 0; + *addr_offset_from_ftp_data_start = 0; } - *addr_size = ftp - ip_addr_start - 1; char *save_ftp = ftp; - ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); - if (!ftp) { - return CT_FTP_CTL_INVALID; - } - int value; - if (!str_to_int(save_ftp, 10, &value)) { - return CT_FTP_CTL_INVALID; - } + uint16_t port_hs; - /* This is derived from the L4 port maximum is 65535. */ - if (value > 255) { - return CT_FTP_CTL_INVALID; - } + if (!extended) { + ftp = terminate_number_str(ftp, MAX_FTP_PORT_DGTS); + if (!ftp) { + return CT_FTP_CTL_INVALID; + } + int value; + if (!str_to_int(save_ftp, 10, &value)) { + return CT_FTP_CTL_INVALID; + } - uint16_t port_hs = value; - port_hs <<= 8; + /* This is derived from the L4 port maximum is 65535. */ + if (value > 255) { + return CT_FTP_CTL_INVALID; + } - /* Skip over comma. */ - ftp++; - save_ftp = ftp; - bool digit_found = false; - while (isdigit(*ftp)) { + port_hs = value; + port_hs <<= 8; + + /* Skip over comma. */ ftp++; - digit_found = true; - } - if (!digit_found) { - return CT_FTP_CTL_INVALID; - } - *ftp = 0; - if (!str_to_int(save_ftp, 10, &value)) { - return CT_FTP_CTL_INVALID; - } + save_ftp = ftp; + bool digit_found = false; + while (isdigit(*ftp)) { + ftp++; + digit_found = true; + } + if (!digit_found) { + return CT_FTP_CTL_INVALID; + } + *ftp = 0; + if (!str_to_int(save_ftp, 10, &value)) { + return CT_FTP_CTL_INVALID; + } - if (value > 255) { - return CT_FTP_CTL_INVALID; + if (value > 255) { + return CT_FTP_CTL_INVALID; + } + + port_hs |= value; + } else { + ftp = terminate_number_str(ftp, MAX_EXT_FTP_PORT_DGTS); + if (!ftp) { + return CT_FTP_CTL_INVALID; + } + int value; + if (!str_to_int(save_ftp, 10, &value)) { + return CT_FTP_CTL_INVALID; + } + if (value > UINT16_MAX) { + return CT_FTP_CTL_INVALID; + } + port_hs = (uint16_t) value; } - port_hs |= value; ovs_be16 port = htons(port_hs); ovs_be32 conn_ipv4_addr; @@ -3478,12 +3549,14 @@ process_ftp_ctl_v4(struct conntrack *ct, OVS_NOT_REACHED(); } - ovs_be32 ftp_ipv4_addr; - ftp_ipv4_addr = ip_addr.s_addr; - /* Although most servers will block this exploit, there may be some - * less well managed. */ - if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { - return CT_FTP_CTL_INVALID; + if (!extended || mode == CT_FTP_MODE_ACTIVE) { + ovs_be32 ftp_ipv4_addr; + ftp_ipv4_addr = ip_addr.s_addr; + /* Although most servers will block this exploit, there may be some + * less well managed. */ + if (ftp_ipv4_addr != conn_ipv4_addr && ftp_ipv4_addr != *v4_addr_rep) { + return CT_FTP_CTL_INVALID; + } } expectation_create(ct, port, conn_for_expectation, diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at index 7f029dbb0..e4862231a 100644 --- a/tests/system-common-macros.at +++ b/tests/system-common-macros.at @@ -280,7 +280,7 @@ m4_define([OVS_GET_HTTP], # m4_define([OVS_GET_FTP], [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused \ - --disable-epsv -v $2] + -v $2] ) # OVS_GET_FTP_ACTIVE([url], [optional_curl_arguments]) @@ -289,7 +289,7 @@ m4_define([OVS_GET_FTP], # m4_define([OVS_GET_FTP_ACTIVE], [curl ftp://$1 --retry 3 --max-time 1 --retry-connrefused -v \ - --ftp-port - --disable-eprt $2] + --ftp-port - $2] ) # OVS_CHECK_FIREWALL() diff --git a/tests/system-traffic.at b/tests/system-traffic.at index 0f3acef3f..e5a1131e6 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -8204,8 +8204,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.2 >/dev/null]) OVS_START_L7([at_ns1], [ftp]) -dnl FTP requests from p0->p1 should work fine. -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2]), [0], [ignore], [ignore]) +dnl FTP requests from p0->p1 should work fine, with and without EPSV. +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--epsv]), [0], [ignore], [ignore]) +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP([10.1.1.2], [--disable-epsv]), [0], [ignore], [ignore]) dnl Discards CLOSE_WAIT and CLOSING AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl @@ -8384,8 +8385,9 @@ OVS_WAIT_UNTIL([ip netns exec at_ns0 ping -c 1 10.1.1.240 >/dev/null]) OVS_START_L7([at_ns1], [ftp]) -dnl FTP requests from p0->p1 should work fine. -NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2]), [0], [ignore], [ignore]) +dnl FTP requests from p0->p1 should work fine with and without EPRT. +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--eprt]), [0], [ignore], [ignore]) +NS_CHECK_EXEC([at_ns0], OVS_GET_FTP_ACTIVE([10.1.1.2], [--disable-eprt]), [0], [ignore], [ignore]) dnl Discards CLOSE_WAIT and CLOSING AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
Recent versions of Curl combine the EPSV and EPRT commands with IPv4 connections instead of the PASV and PORT commands. EPSV and EPRT were added to FTP for IPv6 support but these commands also support IPv4. Most software still uses the old PASV and PORT commands in IPv4 connections but recent versions of curl will default to EPSV and EPRT for all connections. This patch adds support for these extended commands, and added tests for both with and without them. Reported-at: https://issues.redhat.com/browse/FDP-907 Reported-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- NEWS | 3 + lib/conntrack.c | 189 +++++++++++++++++++++++----------- tests/system-common-macros.at | 4 +- tests/system-traffic.at | 10 +- 4 files changed, 142 insertions(+), 64 deletions(-)