| Message ID | 20250801123815.1111230-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Delegated to: | aaron conole |
| Headers | show |
| Series | [ovs-dev] 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/1/25 2: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. This patch adds > support for these extended commands. > > Reported-at: https://issues.redhat.com/browse/FDP-907 > Reported-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/conntrack.c | 189 +++++++++++++++++++++++----------- > tests/system-common-macros.at | 4 +- > 2 files changed, 133 insertions(+), 60 deletions(-) Thanks, Mike! I'll leave the core of the patch for Aaron or Eelco to review, but see a few comments below. First, we probably need to add a NEWS entry for this. And maybe expand the commit message a bit explaining what these commands are. > > diff --git a/lib/conntrack.c b/lib/conntrack.c > index 00346a0be..cb8bda306 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 */ nit: Period at the end. > + 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] > ) Looks like this removes coverage for packets with the other set of commands. We should probably add a test with these options disabled to avoid regressions. Best regards, Ilya Maximets.
diff --git a/lib/conntrack.c b/lib/conntrack.c index 00346a0be..cb8bda306 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()
Recent versions of Curl combine the EPSV and EPRT commands with IPv4 connections instead of the PASV and PORT commands. This patch adds support for these extended commands. Reported-at: https://issues.redhat.com/browse/FDP-907 Reported-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/conntrack.c | 189 +++++++++++++++++++++++----------- tests/system-common-macros.at | 4 +- 2 files changed, 133 insertions(+), 60 deletions(-)