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 <[email protected]>
> Signed-off-by: Mike Pattrick <[email protected]>
> ---
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
