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.
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
