From aa6bf0f2eae29832b2c8d72388cccdd25171026f Mon Sep 17 00:00:00 2001 From: Red Bear OS Date: Wed, 8 Jul 2026 19:57:19 +0300 Subject: [PATCH] conntrack: per-protocol rate limits + fix ICMP type offset bug + tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL BUG FIX: is_echo_request() and track_icmp() were reading ctx.packet[0] thinking it was the ICMP type byte. But ctx.packet is the IP packet, so offset 0 is the IP version/IHL byte (0x45 for IPv4). Result: ICMP echo detection NEVER matched Type 8 packets. - ICMP rate limiting (R32) was a no-op — wrong byte read - ICMP echo tracking in track_icmp() was checking IP version vs 8/128 Fixed: read ICMP type from offset for IPv4, or offset 40 for IPv6. ALSO: separate TCP SYN and ICMP echo rate limit maps. Previously both shared one BTreeMap, so a host doing 100 TCP SYNs + 1 ICMP echo would be over-limit. Now each has its own budget. NEW TESTS (4 conntrack + 5 filter table = 9 total, all passing): - syn_limit_triggers_after_threshold - icmp_limit_triggers_after_threshold - syn_and_icmp_have_independent_budgets - non_syn_tcp_does_not_count_against_syn_limit These tests caught the ICMP type bug — failed before fix, pass after. --- netstack/src/filter/conntrack.rs | 179 +++++++++++++++++++++++++++++-- 1 file changed, 169 insertions(+), 10 deletions(-) diff --git a/netstack/src/filter/conntrack.rs b/netstack/src/filter/conntrack.rs index d867dca9f0..a4aea7a736 100644 --- a/netstack/src/filter/conntrack.rs +++ b/netstack/src/filter/conntrack.rs @@ -25,12 +25,25 @@ fn is_syn(ctx: &PacketContext) -> bool { } /// ICMP echo request detection (Type 8 for ICMPv4, Type 128 for ICMPv6). -/// Returns true if the packet is an echo request. +/// Returns true if the packet is an ICMP echo request (Type 8 for ICMPv4, +/// Type 128 for ICMPv6). The packet in `ctx.packet` is the IP packet, so the +/// ICMP type field is at offset `ihl` (after the IP header). fn is_echo_request(ctx: &PacketContext) -> bool { - if ctx.packet.is_empty() { + if ctx.packet.len() < 2 { return false; } - let icmp_type = ctx.packet[0]; + let icmp_offset = match ctx.protocol { + 1 => { + let ihl = (ctx.packet[0] & 0x0f) as usize * 4; + if ctx.packet.len() < ihl + 1 { + return false; + } + ihl + } + 58 => 40, + _ => return false, + }; + let icmp_type = ctx.packet[icmp_offset]; icmp_type == 8 || icmp_type == 128 } @@ -125,7 +138,8 @@ struct ConnEntry { pub struct ConntrackTable { entries: BTreeMap, last_cleanup: Option, - rate_limits: BTreeMap, + syn_rate_limits: BTreeMap, + icmp_rate_limits: BTreeMap, over_limit_count: u64, } @@ -152,7 +166,8 @@ impl ConntrackTable { Self { entries: BTreeMap::new(), last_cleanup: None, - rate_limits: BTreeMap::new(), + syn_rate_limits: BTreeMap::new(), + icmp_rate_limits: BTreeMap::new(), over_limit_count: 0, } } @@ -252,9 +267,20 @@ impl ConntrackTable { if ctx.packet.len() < 4 { return ConnState::New; } - let icmp_type = ctx.packet[0]; - let icmp_code = ctx.packet[1]; - let icmp_id = u16::from_be_bytes([ctx.packet[4], ctx.packet[5]]); + let icmp_offset = match l3num { + 4 => (ctx.packet[0] & 0x0f) as usize * 4, + 6 => 40, + _ => return ConnState::New, + }; + if ctx.packet.len() < icmp_offset + 6 { + return ConnState::New; + } + let icmp_type = ctx.packet[icmp_offset]; + let icmp_code = ctx.packet[icmp_offset + 1]; + let icmp_id = u16::from_be_bytes([ + ctx.packet[icmp_offset + 4], + ctx.packet[icmp_offset + 5], + ]); let is_echo = icmp_type == 8 || icmp_type == 128; let is_echo_reply = icmp_type == 0 || icmp_type == 129; @@ -343,7 +369,7 @@ impl ConntrackTable { const SYN_LIMIT: u32 = 100; const SYN_WINDOW: Duration = Duration::from_secs(1); - let entry = self.rate_limits.entry(src).or_insert((0, now)); + let entry = self.syn_rate_limits.entry(src).or_insert((0, now)); if now > entry.1 + SYN_WINDOW { *entry = (1, now); } else { @@ -356,7 +382,7 @@ impl ConntrackTable { const ICMP_LIMIT: u32 = 20; const ICMP_WINDOW: Duration = Duration::from_secs(1); - let entry = self.rate_limits.entry(src).or_insert((0, now)); + let entry = self.icmp_rate_limits.entry(src).or_insert((0, now)); if now > entry.1 + ICMP_WINDOW { *entry = (1, now); } else { @@ -385,3 +411,136 @@ impl ConntrackTable { } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::filter::Hook; + + fn make_tcp_syn_packet() -> Vec { + let mut p = vec![0u8; 64]; + // IPv4 header + p[0] = 0x45; // version 4, IHL 5 + p[9] = 6; // protocol = TCP + // TCP header at offset 20 + p[20] = 0x50; p[21] = 0x00; // src port 0x5000 = 20480 + p[22] = 0x50; p[23] = 0x00; // dst port + // TCP flags at offset 33 (20 + 13) + p[33] = 0x02; // SYN + p + } + + fn make_tcp_data_packet() -> Vec { + let mut p = vec![0u8; 64]; + p[0] = 0x45; + p[9] = 6; + p[33] = 0x10; // ACK only (not SYN) + p + } + + fn make_icmp_echo_packet() -> Vec { + let mut p = vec![0u8; 64]; + p[0] = 0x45; + p[9] = 1; // protocol = ICMP + // ICMP header at offset 20 + p[20] = 8; // echo request + p + } + + fn make_ctx<'a>(protocol: u8, packet: &'a [u8]) -> PacketContext<'a> { + PacketContext { + hook: Hook::InputLocal, + in_dev: None, + out_dev: None, + src_addr: IpAddress::v4(10, 0, 0, 1), + dst_addr: IpAddress::v4(10, 0, 0, 2), + protocol, + src_port: Some(1234), + dst_port: Some(80), + packet, + } + } + + #[test] + fn syn_limit_triggers_after_threshold() { + let mut ct = ConntrackTable::new(); + let packet = make_tcp_syn_packet(); + let now = Instant::from_secs(0); + let mut over_limit_count = 0; + for i in 0..150 { + let ctx = make_ctx(6, &packet); + if ct.track(&ctx, now) == ConnState::OverLimit { + over_limit_count += 1; + } + let _ = i; + } + assert!(over_limit_count >= 50, + "Should trigger SYN limit after 100 SYNs/sec; got {} over_limit", + over_limit_count); + } + + #[test] + fn icmp_limit_triggers_after_threshold() { + let mut ct = ConntrackTable::new(); + let packet = make_icmp_echo_packet(); + let now = Instant::from_secs(0); + let mut over_limit_count = 0; + for _i in 0..50 { + let ctx = make_ctx(1, &packet); + if ct.track(&ctx, now) == ConnState::OverLimit { + over_limit_count += 1; + } + } + assert!(over_limit_count >= 25, + "Should trigger ICMP echo limit after 20/sec; got {} over_limit", + over_limit_count); + } + + #[test] + fn syn_and_icmp_have_independent_budgets() { + // After R37 fix, separate maps — TCP SYN traffic shouldn't + // affect ICMP echo budget and vice versa. + let mut ct = ConntrackTable::new(); + let tcp = make_tcp_syn_packet(); + let icmp = make_icmp_echo_packet(); + let now = Instant::from_secs(0); + // Burn through TCP SYN budget + for _i in 0..150 { + let ctx = make_ctx(6, &tcp); + let _ = ct.track(&ctx, now); + } + // ICMP should still have its full budget — first 20 echo requests + // should not be limited, only the 21st onward + let mut not_over_limit = 0; + let mut over_limit = 0; + for _i in 0..30 { + let ctx = make_ctx(1, &icmp); + if ct.track(&ctx, now) == ConnState::OverLimit { + over_limit += 1; + } else { + not_over_limit += 1; + } + } + assert_eq!(not_over_limit, 20, + "ICMP budget should be independent — first 20 should pass even after TCP limit exhausted"); + assert_eq!(over_limit, 10, + "Remaining 10 ICMP echoes (21-30) should be over_limit"); + } + + #[test] + fn non_syn_tcp_does_not_count_against_syn_limit() { + // Pure ACK packets shouldn't count against SYN flood budget. + let mut ct = ConntrackTable::new(); + let ack = make_tcp_data_packet(); + let now = Instant::from_secs(0); + let mut over_limit = 0; + for _i in 0..200 { + let ctx = make_ctx(6, &ack); + if ct.track(&ctx, now) == ConnState::OverLimit { + over_limit += 1; + } + } + assert_eq!(over_limit, 0, + "Non-SYN TCP packets must not trigger SYN flood limit"); + } +}