From af3bb804bf59e897a5ec184b0ecd121e28e26474 Mon Sep 17 00:00:00 2001 From: Vladimir Stoilov Date: Thu, 7 Nov 2024 15:52:26 +0200 Subject: [PATCH] Fix reading only the needed TCP/UDP header bytes (#1730) * [windows_kext] Fix reading only the needed TCP/UDP header bytes * [windows_kext] Disable debug mode * [windows_kext] Block all fragment packets * [windows_kext] Improve wording for compiler error --- windows_kext/driver/src/lib.rs | 5 +++++ windows_kext/driver/src/logger.rs | 5 +---- windows_kext/driver/src/packet_callouts.rs | 7 +++++++ windows_kext/driver/src/packet_util.rs | 13 +++++++------ windows_kext/wdk/src/filter_engine/callout_data.rs | 4 ++++ windows_kext/wdk/src/filter_engine/metadata.rs | 14 +++++++++++--- 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/windows_kext/driver/src/lib.rs b/windows_kext/driver/src/lib.rs index 7d9fe3a1d..45bc6c609 100644 --- a/windows_kext/driver/src/lib.rs +++ b/windows_kext/driver/src/lib.rs @@ -22,6 +22,11 @@ mod stream_callouts; use wdk::allocator::WindowsAllocator; +// For consistent behavior during development and production only release mode should be used. +// Certain behavior of the compiler will change and this can result in errors and different behavior in debug and release mode. +#[cfg(debug_assertions)] +compile_error!("Must be built in release mode to ensure consistent behavior and prevent optimization-related issues. Use `cargo build --release`."); + #[cfg(not(test))] use core::panic::PanicInfo; diff --git a/windows_kext/driver/src/logger.rs b/windows_kext/driver/src/logger.rs index 5a0440a25..129f9f7c8 100644 --- a/windows_kext/driver/src/logger.rs +++ b/windows_kext/driver/src/logger.rs @@ -6,11 +6,8 @@ use core::{ }; use protocol::info::{Info, Severity}; -#[cfg(not(debug_assertions))] pub const LOG_LEVEL: u8 = Severity::Warning as u8; - -#[cfg(debug_assertions)] -pub const LOG_LEVEL: u8 = Severity::Trace as u8; +// pub const LOG_LEVEL: u8 = Severity::Trace as u8; pub const MAX_LOG_LINE_SIZE: usize = 150; diff --git a/windows_kext/driver/src/packet_callouts.rs b/windows_kext/driver/src/packet_callouts.rs index fb3ee90b8..4609aad0f 100644 --- a/windows_kext/driver/src/packet_callouts.rs +++ b/windows_kext/driver/src/packet_callouts.rs @@ -110,6 +110,13 @@ fn ip_packet_layer( interface_index: u32, sub_interface_index: u32, ) { + // Block all fragment data. No easy way to keep track of the origin and they are rarely used. + if data.is_fragment_data() { + data.action_block(); + crate::err!("blocked fragment packet"); + return; + } + let Some(device) = crate::entry::get_device() else { return; }; diff --git a/windows_kext/driver/src/packet_util.rs b/windows_kext/driver/src/packet_util.rs index 42b4dac75..6f5b4eb34 100644 --- a/windows_kext/driver/src/packet_util.rs +++ b/windows_kext/driver/src/packet_util.rs @@ -245,8 +245,6 @@ fn print_packet(packet: &[u8]) { /// /// * `Ok(Key)` - A key containing the protocol, local and remote addresses and ports. /// * `Err(String)` - An error message if the function fails to get net_buffer data. -const HEADERS_LEN: usize = smoltcp::wire::IPV4_HEADER_LEN + smoltcp::wire::TCP_HEADER_LEN; - fn get_ports(packet: &[u8], protocol: smoltcp::wire::IpProtocol) -> (u16, u16) { match protocol { smoltcp::wire::IpProtocol::Tcp => { @@ -262,12 +260,13 @@ fn get_ports(packet: &[u8], protocol: smoltcp::wire::IpProtocol) -> (u16, u16) { } pub fn get_key_from_nbl_v4(nbl: &NetBufferList, direction: Direction) -> Result { - // Get bytes - let mut headers = [0; HEADERS_LEN]; + // Get first bytes of the packet. IP header + src port (2 bytes) + dst port (2 bytes) + let mut headers = [0; smoltcp::wire::IPV4_HEADER_LEN + 4]; if nbl.read_bytes(&mut headers).is_err() { return Err("failed to get net_buffer data".to_string()); } + // This will panic in debug mode, probably because of runtime checks. // Parse packet let ip_packet = Ipv4Packet::new_unchecked(&headers); let (src_port, dst_port) = get_ports( @@ -307,11 +306,13 @@ pub fn get_key_from_nbl_v4(nbl: &NetBufferList, direction: Direction) -> Result< /// * `Ok(Key)` - A key containing the protocol, local and remote addresses and ports. /// * `Err(String)` - An error message if the function fails to get net_buffer data. pub fn get_key_from_nbl_v6(nbl: &NetBufferList, direction: Direction) -> Result { - // Get bytes - let mut headers = [0; smoltcp::wire::IPV6_HEADER_LEN + smoltcp::wire::TCP_HEADER_LEN]; + // Get first bytes of the packet. IP header + src port (2 bytes) + dst port (2 bytes) + let mut headers = [0; smoltcp::wire::IPV6_HEADER_LEN + 4]; let Ok(()) = nbl.read_bytes(&mut headers) else { return Err("failed to get net_buffer data".to_string()); }; + + // This will panic in debug mode, probably because of runtime checks. // Parse packet let ip_packet = Ipv6Packet::new_unchecked(&headers); let (src_port, dst_port) = get_ports( diff --git a/windows_kext/wdk/src/filter_engine/callout_data.rs b/windows_kext/wdk/src/filter_engine/callout_data.rs index bb861f84f..abb5e318c 100644 --- a/windows_kext/wdk/src/filter_engine/callout_data.rs +++ b/windows_kext/wdk/src/filter_engine/callout_data.rs @@ -133,6 +133,10 @@ impl<'a> CalloutData<'a> { } } + pub fn is_fragment_data(&self) -> bool { + unsafe { (*self.metadata).is_fragment_data() } + } + pub fn pend_operation( &mut self, packet_list: Option, diff --git a/windows_kext/wdk/src/filter_engine/metadata.rs b/windows_kext/wdk/src/filter_engine/metadata.rs index 632830fab..55b3b7de8 100644 --- a/windows_kext/wdk/src/filter_engine/metadata.rs +++ b/windows_kext/wdk/src/filter_engine/metadata.rs @@ -7,9 +7,9 @@ use windows_sys::Win32::{ NetworkManagement::{ IpHelper::IP_ADDRESS_PREFIX, WindowsFilteringPlatform::{ - FWPS_METADATA_FIELD_COMPLETION_HANDLE, FWPS_METADATA_FIELD_PROCESS_ID, - FWPS_METADATA_FIELD_PROCESS_PATH, FWPS_METADATA_FIELD_REMOTE_SCOPE_ID, - FWPS_METADATA_FIELD_TRANSPORT_CONTROL_DATA, + FWPS_METADATA_FIELD_COMPLETION_HANDLE, FWPS_METADATA_FIELD_FRAGMENT_DATA, + FWPS_METADATA_FIELD_PROCESS_ID, FWPS_METADATA_FIELD_PROCESS_PATH, + FWPS_METADATA_FIELD_REMOTE_SCOPE_ID, FWPS_METADATA_FIELD_TRANSPORT_CONTROL_DATA, FWPS_METADATA_FIELD_TRANSPORT_ENDPOINT_HANDLE, FWP_BYTE_BLOB, FWP_DIRECTION, }, }, @@ -137,6 +137,14 @@ impl FwpsIncomingMetadataValues { None } + pub(crate) fn is_fragment_data(&self) -> bool { + if self.has_field(FWPS_METADATA_FIELD_FRAGMENT_DATA) { + return self.fragment_metadata.fragment_offset != 0; + } + + false + } + pub(crate) unsafe fn get_control_data(&self) -> Option> { if self.has_field(FWPS_METADATA_FIELD_TRANSPORT_CONTROL_DATA) { if self.control_data.is_null() || self.control_data_length == 0 {