The FTP ALG handling for IPv4 and IPv6 is guarded from overwriting the
buffer space by checking the headroom + tailroom against the rewrite
delta. However, when a packet needs to be expanded, only the tailroom
is actually available. The guard itself allows modifications to pass
that may otherwise be flagged.
Fix this by only checking that the tailroom has sufficient space to
handle the packet growth. This is a bit different as we no longer
check the delta, but just the actual expansion room available. This
check is a bit more conservative, so there could be an edge-case
packet that used to pass through the checks but no longer
would.
The FTP ALG unit tests are updated to correct a comment about the
guard design, and test for the new condition.
Fixes: bd5e81a0e596 ("Userspace Datapath: Add ALG infra and FTP.")
Reported-by: VinÃcius Rodrigues <[email protected]>
Assisted-by: Claude Sonnet 4.5
Signed-off-by: Aaron Conole <[email protected]>
---
AUTHORS.rst | 1 +
lib/conntrack.c | 20 +++----
tests/library.at | 4 ++
tests/test-conntrack.c | 122 ++++++++++++++++++++++++++++++++++++++++-
4 files changed, 134 insertions(+), 13 deletions(-)
diff --git a/AUTHORS.rst b/AUTHORS.rst
index 0f7445c80f..92500507d4 100644
--- a/AUTHORS.rst
+++ b/AUTHORS.rst
@@ -797,6 +797,7 @@ Tulio Ribeiro [email protected]
Tytus Kurek [email protected]
Valentin Bud [email protected]
Vasiliy Tolstov [email protected]
+VinÃcius Rodrigues [email protected]
Vinllen Chen [email protected]
Vipul Ashri [email protected]
Vishal Swarankar [email protected]
diff --git a/lib/conntrack.c b/lib/conntrack.c
index f84cdd216a..0809e73f24 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -3294,12 +3294,12 @@ repl_ftp_v4_addr(struct dp_packet *pkt, ovs_be32
v4_addr_rep,
/* 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 >
- dp_packet_get_allocated(pkt)) {
-
+ if (MAX_FTP_V4_NAT_DELTA > dp_packet_tailroom(pkt)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
- VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP V4",
- dp_packet_get_allocated(pkt));
+ VLOG_WARN_RL(&rl,
+ "Oversized packet detected with FTPv4 (%"PRIuSIZE
+ " vs. %"PRIu32")",
+ dp_packet_tailroom(pkt), MAX_FTP_V4_NAT_DELTA);
return 0;
}
@@ -3674,12 +3674,12 @@ repl_ftp_v6_addr(struct dp_packet *pkt, union ct_addr
v6_addr_rep,
/* Do conservative check for pathological MTU usage. */
uint32_t orig_used_size = dp_packet_size(pkt);
- if (orig_used_size + MAX_FTP_V6_NAT_DELTA >
- dp_packet_get_allocated(pkt)) {
-
+ if (MAX_FTP_V6_NAT_DELTA > dp_packet_tailroom(pkt)) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
- VLOG_WARN_RL(&rl, "Unsupported effective MTU %u used with FTP V6",
- dp_packet_get_allocated(pkt));
+ VLOG_WARN_RL(&rl,
+ "Oversized packet detected with FTPv6 (%"PRIuSIZE
+ " vs. %"PRIu32")",
+ dp_packet_tailroom(pkt), MAX_FTP_V6_NAT_DELTA);
return 0;
}
diff --git a/tests/library.at b/tests/library.at
index 80ebe6ed8a..83516bc502 100644
--- a/tests/library.at
+++ b/tests/library.at
@@ -296,3 +296,7 @@ AT_CLEANUP
AT_SETUP([Conntrack Library - FTP ALG parsing])
AT_CHECK([ovstest test-conntrack ftp-alg-large-payload])
AT_CLEANUP
+
+AT_SETUP([Conntrack Library - FTP ALG guard])
+AT_CHECK([ovstest test-conntrack ftp-alg-no-tailroom])
+AT_CLEANUP
diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
index 2babe989c4..cc8f8138a4 100644
--- a/tests/test-conntrack.c
+++ b/tests/test-conntrack.c
@@ -75,9 +75,8 @@ build_eth_ip_packet(struct dp_packet *pkt, struct eth_addr
eth_src,
}
if (pkt == NULL) {
- /* 64-byte extra headroom keeps dp_packet_get_allocated() large enough
- * that the FTP V4 MTU guard (orig_used_size + 8 <= allocated) passes
- * even when the packet is near its maximum size. */
+ /* Allocate a packet with enough room for payload, and reserve
+ * 64-bytes for extra headroom. */
pkt = dp_packet_new_with_headroom(ETH_HEADER_LEN + IP_HEADER_LEN
+ proto_len + payload_alloc, 64);
}
@@ -576,7 +575,119 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx
OVS_UNUSED)
conntrack_destroy(ct);
}
+/* Test FTP ALG tailroom guard.
+ *
+ * This test verifies that the FTP ALG properly rejects packets that don't
+ * have sufficient tailroom for NAT address replacement. It creates a packet
+ * with minimal tailroom (less than MAX_FTP_V4_NAT_DELTA) and confirms the
+ * guard triggers, leaving the packet unmodified. */
+static void
+test_ftp_alg_no_tailroom(struct ovs_cmdl_context *ctx OVS_UNUSED)
+{
+ struct eth_addr eth_src = ETH_ADDR_C(00, 01, 02, 03, 04, 05);
+ struct eth_addr eth_dst = ETH_ADDR_C(00, 06, 07, 08, 09, 0a);
+ ovs_be32 ip_src = inet_addr("10.0.0.100");
+ ovs_be32 ip_dst = inet_addr("10.0.0.1");
+ uint16_t sport = 54321;
+ uint16_t dport = 21;
+
+ /* SNAT configuration */
+ struct nat_action_info_t nat_info;
+ memset(&nat_info, 0, sizeof nat_info);
+ nat_info.nat_action = NAT_ACTION_SRC;
+ nat_info.min_addr.ipv4 = ip_dst;
+ nat_info.max_addr.ipv4 = ip_dst;
+
+ ct = conntrack_init();
+ conntrack_set_tcp_seq_chk(ct, false);
+
+ long long now = time_msec();
+
+ /* Create conntrack entry with SYN */
+ struct dp_packet *syn = build_eth_ip_packet(NULL, eth_src, eth_dst,
+ ip_src, ip_dst,
+ IPPROTO_TCP, 0);
+ build_tcp_packet(syn, sport, dport, TCP_SYN, NULL, 0);
+
+ struct dp_packet_batch syn_batch;
+ dp_packet_batch_init_packet(&syn_batch, syn);
+ conntrack_execute(ct, &syn_batch, htons(ETH_TYPE_IP), false, true, 0,
+ NULL, NULL, "ftp", &nat_info, now, 0);
+ dp_packet_delete_batch(&syn_batch, true);
+
+ /* Create a packet with NO extra headroom - allocate exact size needed.
+ * This ensures tailroom will be insufficient for FTP NAT expansion. */
+ char ftp_cmd[] = "PORT 10,0,0,100,212,53\r\n";
+ size_t ftp_len = strlen(ftp_cmd);
+
+ /* Allocate packet with ZERO extra space beyond what's needed */
+ size_t exact_size = (ETH_HEADER_LEN + IP_HEADER_LEN + TCP_HEADER_LEN +
+ ftp_len);
+ struct dp_packet *pkt = dp_packet_new(exact_size);
+
+ /* Manually build the packet without extra headroom */
+ eth_compose(pkt, eth_src, eth_dst, ETH_TYPE_IP,
+ IP_HEADER_LEN + TCP_HEADER_LEN + ftp_len);
+
+ struct ip_header *iph = dp_packet_l3(pkt);
+ iph->ip_ihl_ver = IP_IHL_VER(5, 4);
+ iph->ip_tot_len = htons(IP_HEADER_LEN + TCP_HEADER_LEN + ftp_len);
+ iph->ip_ttl = 64;
+ iph->ip_proto = IPPROTO_TCP;
+ packet_set_ipv4_addr(pkt, &iph->ip_src, ip_src);
+ packet_set_ipv4_addr(pkt, &iph->ip_dst, ip_dst);
+ iph->ip_csum = csum(iph, IP_HEADER_LEN);
+
+ dp_packet_set_l4(pkt, (char *)iph + IP_HEADER_LEN);
+ struct tcp_header *tcph = dp_packet_l4(pkt);
+ tcph->tcp_src = htons(sport);
+ tcph->tcp_dst = htons(dport);
+ put_16aligned_be32(&tcph->tcp_seq, htonl(1000));
+ put_16aligned_be32(&tcph->tcp_ack, htonl(1000));
+ tcph->tcp_ctl = TCP_CTL(TCP_PSH | TCP_ACK, TCP_HEADER_LEN / 4);
+ tcph->tcp_winsz = htons(65535);
+ tcph->tcp_urg = 0;
+
+ /* Copy FTP payload */
+ memcpy((char *)tcph + TCP_HEADER_LEN, ftp_cmd, ftp_len);
+
+ /* Update checksums */
+ iph->ip_csum = 0;
+ iph->ip_csum = csum(iph, IP_HEADER_LEN);
+ tcph->tcp_csum = 0;
+ uint32_t tcp_csum = packet_csum_pseudoheader(iph);
+ tcph->tcp_csum = csum_finish(
+ csum_continue(tcp_csum, tcph, TCP_HEADER_LEN + ftp_len));
+
+ /* Verify we have insufficient tailroom */
+ size_t tailroom = dp_packet_tailroom(pkt);
+ ovs_assert(tailroom < 8); /* Less than MAX_FTP_V4_NAT_DELTA */
+
+ /* Save original payload for comparison */
+ char original_payload[64];
+ const char *payload_start = (const char *)tcph + TCP_HEADER_LEN;
+ memcpy(original_payload, payload_start, ftp_len);
+
+ /* Process through conntrack - guard should reject modification */
+ struct dp_packet_batch batch;
+ dp_packet_batch_init_packet(&batch, pkt);
+ conntrack_execute(ct, &batch, htons(ETH_TYPE_IP), false, true, 0,
+ NULL, NULL, "ftp", &nat_info, now, 0);
+
+ /* Verify payload was NOT modified (guard prevented it) */
+ tcph = dp_packet_l4(pkt);
+ payload_start = (const char *)tcph + TCP_HEADER_LEN;
+ ovs_assert(!memcmp(payload_start, original_payload, ftp_len));
+
+ /* The original address should still be present, not the SNAT address */
+ ovs_assert(!strncmp(payload_start, "PORT 10,0,0,100,", 16));
+
+ dp_packet_delete_batch(&batch, true);
+ conntrack_destroy(ct);
+}
+
+
static const struct ovs_cmdl_command commands[] = {
/* Connection tracker tests. */
/* Starts 'n_threads' threads. Each thread will send 'n_pkts' packets to
@@ -601,6 +712,11 @@ static const struct ovs_cmdl_command commands[] = {
* is rewritten to the SNAT target rather than causing a crash. */
{"ftp-alg-large-payload", "", 0, 0,
test_ftp_alg_large_payload, OVS_RO},
+ /* Verifies that the FTP ALG tailroom guard properly rejects packets
+ * that don't have sufficient space for NAT address expansion. Creates
+ * a packet with zero tailroom and confirms it's not modified. */
+ {"ftp-alg-no-tailroom", "", 0, 0,
+ test_ftp_alg_no_tailroom, OVS_RO},
{NULL, NULL, 0, 0, NULL, OVS_RO},
};
--
2.55.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
