The FTP ALG handling for IPv4 and IPv6 is guarded from overwriting the
buffer space by checking the headroom + tail-room against the rewrite
delta. However, when a packet needs to be expanded, only the
tail-room is actually available. The guard itself allows
modifications to pass that may otherwise be flagged.
Fix this by only checking that the tail-room 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, Claude Opus
Signed-off-by: Aaron Conole <[email protected]>
---
v1->v2: Fix splats related to the test suite
v2->v3: Fix the test suite after review from Ilya
NOTE: We need to add Vinicius to the AUTHORS file on commit,
because there is an email domain that CloudFlare complains
about.
lib/conntrack.c | 20 ++++----
tests/library.at | 4 ++
tests/test-conntrack.c | 113 +++++++++++++++++++++++++++++++++++++++--
3 files changed, 124 insertions(+), 13 deletions(-)
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..227d1fb433 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], [0], [], [ignore])
+AT_CLEANUP
diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c
index 2babe989c4..e9b690d4b9 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,112 @@ test_ftp_alg_large_payload(struct ovs_cmdl_context *ctx
OVS_UNUSED)
conntrack_destroy(ct);
}
+/* Test FTP ALG tail-room guard.
+ *
+ * This test verifies that the FTP ALG properly rejects packets that don't
+ * have sufficient tail-room for NAT address replacement. It creates a packet
+ * with minimal tail-room (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("192.168.100.200");
+ uint16_t sport = 54321;
+ uint16_t dport = 21;
+
+ 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();
+
+ 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 tail-room 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);
+ enum { EXTRA = 8 }; /* */
+ struct dp_packet *pkt = dp_packet_new(exact_size + 2 + EXTRA);
+ dp_packet_reserve(pkt, 2 + EXTRA);
+ memset(dp_packet_put_uninit(pkt, exact_size), 0, exact_size);
+
+ struct eth_header *eth = dp_packet_data(pkt);
+ eth->eth_dst = eth_dst;
+ eth->eth_src = eth_src;
+ eth->eth_type = htons(ETH_TYPE_IP);
+
+ dp_packet_set_l3(pkt, (char *) dp_packet_data(pkt) + ETH_HEADER_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;
+ put_16aligned_be32(&iph->ip_src, ip_src);
+ put_16aligned_be32(&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);
+
+ /* Copy FTP payload. */
+ memcpy((char *) tcph + TCP_HEADER_LEN, ftp_cmd, ftp_len);
+ tcph->tcp_csum = csum_finish(csum_continue(packet_csum_pseudoheader(iph),
+ tcph, TCP_HEADER_LEN + ftp_len));
+
+ /* The rewrite needs seven extra bytes; the buffer has free space only in
+ * the headroom. */
+ ovs_assert(dp_packet_size(pkt) == exact_size);
+ ovs_assert(dp_packet_tailroom(pkt) == 0);
+ ovs_assert(dp_packet_headroom(pkt) == 2 + EXTRA);
+ ovs_assert(dp_packet_size(pkt) + EXTRA <= dp_packet_get_allocated(pkt));
+
+ /* 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);
+ const char *payload_start = (const char *) tcph + TCP_HEADER_LEN;
+ ovs_assert(!strncmp(payload_start, ftp_cmd, ftp_len));
+
+ 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 +705,9 @@ 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 FTP ALG rejects rewrites without sufficient tail-room. */
+ {"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
