vasilito/RedBear-OS
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

review fixes: 2 critical bugs found by comprehensive analysis

Browse Source 
CRITICAL BUG #1: is_syn() IPv4-only
conntrack.rs is_syn() hardcoded offset 33 (IPv4: 20+13).
For IPv6, TCP flags are at offset 53 (40+13). IPv6 SYN packets
were never detected as SYNs, so SYN flood rate limiting was
completely broken for IPv6 traffic.
Fix: pass l3num, compute tcp_offset correctly for both families.
Added ipv6_syn_detection_works regression test with IPv6 SYN packet
at offset 53 — verifies the fix.

CRITICAL BUG #2: netfilter writes all broken
open_path stored paths without leading '/' (e.g. 'rule/add')
but commit() compared against '/rule/add' (with leading '/').
Result: ALL write operations silently failed with EINVAL:
  - rule/add  - never added rules
  - nat/add   - never added NAT
  - rule/del  - never deleted
  - nat/del   - never deleted
  - policy/X  - never set policy
  - reset     - never reset
  - counters/reset - never reset
Fix: align commit() paths with the stored convention (no '/').
Removed dead /rule/del/<id> REST-style path (no open_path arm existed).
Updated stored path convention comment.

Tests: 21 total (added ipv6_syn_detection_works), all passing.
This commit is contained in:
Red Bear OS
2026-07-08 22:07:41 +03:00
parent 75f5480f84
commit 263950aefa
2 changed files with 69 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
netstack/src/filter/conntrack.rs
+62 -15
View File
@@ -20,8 +20,15 @@ use core::hash::{Hash, Hasher};
use smoltcp::time::{Duration, Instant};
use smoltcp::wire::IpAddress;
fn is_syn(ctx: &PacketContext) -> bool {
ctx.packet.len() >= 34 && (ctx.packet[33] & 0x02) != 0 && (ctx.packet[33] & 0x10) == 0
fn is_syn(ctx: &PacketContext, l3num: u8) -> bool {
// TCP flags byte is at offset 13 in the TCP header.
// TCP header starts after the IP header: 20 bytes for IPv4, 40 for IPv6.
let tcp_offset = if l3num == 4 { 20 } else { 40 };
if ctx.packet.len() <= tcp_offset + 13 {
return false;
}
let flags = ctx.packet[tcp_offset + 13];
(flags & 0x02) != 0 && (flags & 0x10) == 0
}
/// ICMP echo request detection (Type 8 for ICMPv4, Type 128 for ICMPv6).
@@ -181,7 +188,7 @@ impl ConntrackTable {
return ConnState::New;
}
if ctx.protocol == 6 && is_syn(ctx) && self.check_syn_limit(ctx.src_addr, now) {
if ctx.protocol == 6 && is_syn(ctx, l3num) && self.check_syn_limit(ctx.src_addr, now) {
self.over_limit_count = self.over_limit_count.saturating_add(1);
return ConnState::OverLimit;
}
@@ -447,19 +454,33 @@ mod tests {
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,
}
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,
}
}
fn make_v6_ctx<'a>(protocol: u8, packet: &'a [u8]) -> PacketContext<'a> {
PacketContext {
hook: Hook::InputLocal,
in_dev: None,
out_dev: None,
src_addr: IpAddress::v6(0xfe80, 0, 0, 0, 0, 0, 0, 1),
dst_addr: IpAddress::v6(0xfe80, 0, 0, 0, 0, 0, 0, 2),
protocol,
src_port: Some(1234),
dst_port: Some(80),
packet,
}
}
#[test]
fn syn_limit_triggers_after_threshold() {
@@ -543,4 +564,30 @@ mod tests {
assert_eq!(over_limit, 0,
"Non-SYN TCP packets must not trigger SYN flood limit");
}
#[test]
fn ipv6_syn_detection_works() {
// Regression test for the IPv4-only `is_syn()` bug.
// Before fix: offset 33 was hardcoded, reading IPv6 header
// bytes instead of TCP flags (which are at offset 53 for IPv6).
let mut ct = ConntrackTable::new();
let mut p = vec![0u8; 80];
// IPv6 header: version 6 at byte 0 (0x60), next header = 6 (TCP) at byte 6
p[0] = 0x60;
p[6] = 6; // next header = TCP
// TCP header at offset 40, flags at offset 53
p[53] = 0x02; // SYN
let now = Instant::from_secs(0);
// The l3num is inferred from ctx.src_addr (v6 → l3num=6).
let mut over_limit_count = 0;
for _i in 0..150 {
let ctx = make_v6_ctx(6, &p);
if ct.track(&ctx, now) == ConnState::OverLimit {
over_limit_count += 1;
}
}
assert!(over_limit_count >= 50,
"IPv6 SYN must trigger rate limit after threshold; got {}",
over_limit_count);
}
}
netstack/src/scheme/netfilter.rs
+7 -15
View File
@@ -253,17 +253,17 @@ impl NetFilterFile {
let path = self.path.clone();
let mut table = table.borrow_mut();
if path == "/rule/add" {
if path == "rule/add" {
let rule = parse_rule(line)
.map_err(|e| SyscallError::new(syscall::EINVAL))?;
let id = table.add(rule);
log::info!("netfilter: added rule id={}", id);
} else if path == "/nat/add" {
} else if path == "nat/add" {
let nat_rule = parse_nat_rule(line)
.map_err(|_| SyscallError::new(syscall::EINVAL))?;
let id = table.nat_table.add(nat_rule);
log::info!("netfilter: added NAT rule id={}", id);
} else if path == "/nat/del" {
} else if path == "nat/del" {
let id: u32 = line
.parse()
.map_err(|_| SyscallError::new(syscall::EINVAL))?;
@@ -271,21 +271,13 @@ impl NetFilterFile {
return Err(SyscallError::new(syscall::ENOENT));
}
log::info!("netfilter: removed NAT rule id={}", id);
} else if let Some(rest) = path.strip_prefix("/rule/del/") {
let id: u32 = rest
.parse()
.map_err(|_| SyscallError::new(syscall::EINVAL))?;
if !table.remove(id) {
return Err(SyscallError::new(syscall::ENOENT));
}
log::info!("netfilter: removed rule id={}", id);
} else if path == "/reset" {
} else if path == "reset" {
*table = FilterTable::new();
log::info!("netfilter: table reset to defaults");
} else if path == "/counters/reset" {
} else if path == "counters/reset" {
table.reset_counters();
log::info!("netfilter: counters reset (rules preserved)");
} else if let Some(rest) = path.strip_prefix("/policy/") {
} else if let Some(rest) = path.strip_prefix("policy/") {
let hook = parse_hook_name(rest).ok_or(SyscallError::new(syscall::EINVAL))?;
let verdict = match line.to_uppercase().as_str() {
"ACCEPT" => Verdict::Accept,
@@ -294,7 +286,7 @@ impl NetFilterFile {
};
table.set_default_policy(hook, verdict);
log::info!("netfilter: set {} policy to {}", rest, verdict.name());
} else if path == "/rule/del" {
} else if path == "rule/del" {
let id: u32 = line
.parse()
.map_err(|_| SyscallError::new(syscall::EINVAL))?;