On Wed, Aug 13, 2025 at 4:23 PM Aaron Conole <[email protected]> wrote:
>
> Mike Pattrick via dev <[email protected]> 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 <[email protected]>
> > Signed-off-by: Mike Pattrick <[email protected]>
> > ---
> > 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
