From c0ce53f487644d96fba23f589ba353ec6dbbcd0e Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 18 Jul 2024 10:08:39 -0400 Subject: [PATCH 1/2] Avoid panic in RouteNextHopBuffer length checks If the next-hops length is u16::MAX, adding 8 (the PAYLOAD_OFFSET) to it would cause integer overflow and lead to a panic. Rather than switching to `saturating_add`, remove the addition entirely. It was unnecessary, as the provided length already accounts for the RTA struct. --- src/route/next_hops.rs | 2 +- src/route/tests/route_flags.rs | 14 +++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/route/next_hops.rs b/src/route/next_hops.rs index 724c1c01..af23a8dd 100644 --- a/src/route/next_hops.rs +++ b/src/route/next_hops.rs @@ -63,7 +63,7 @@ impl> RouteNextHopBuffer { return Err(format!( "invalid RouteNextHopBuffer: length {} < {}", len, - 8 + self.length() + self.length(), ) .into()); } diff --git a/src/route/tests/route_flags.rs b/src/route/tests/route_flags.rs index 2c67ed0c..7e9e8ecd 100644 --- a/src/route/tests/route_flags.rs +++ b/src/route/tests/route_flags.rs @@ -8,7 +8,7 @@ use netlink_packet_utils::traits::{Emitable, Parseable}; use crate::route::flags::RouteFlags; use crate::route::{ RouteAttribute, RouteHeader, RouteMessage, RouteMessageBuffer, - RouteProtocol, RouteScope, RouteType, + RouteNextHopBuffer, RouteProtocol, RouteScope, RouteType, }; use crate::AddressFamily; @@ -54,3 +54,15 @@ fn test_ipv6_add_route_onlink() { assert_eq!(buf, raw); } + +// Verify that [`RouteNextHopBuffer`] rejects the buffer when provided with +// an invalid length. +#[test] +fn test_next_hop_max_buffer_len() { + // Route next-hop buffer layout: + // |byte0|byte1|byte2|byte3|byte4|byte5|byte6|byte7|bytes8+| + // |-----|-----|-----|-----|-----|-----|-----|-----|-------| + // | length |flags|hops | Interface Index |Payload| + let buffer = [0xff, 0xff, 0, 0, 0, 0, 0, 0]; + assert!(RouteNextHopBuffer::new_checked(buffer).is_err()); +} From d89f8efb87fbf79e8dde0e85e79c218f48aaab30 Mon Sep 17 00:00:00 2001 From: Jeff Martin Date: Thu, 18 Jul 2024 11:27:18 -0400 Subject: [PATCH 2/2] Avoid panic in TcU32Selector parsing `TcU32SelectorBuffer::new_checked` returns an error when the underlying buffer is not large enough to contain `nkeys`. This avoids an index-out-of-bounds panic in `TcU32Selector::parse`, when extracting keys from the underlying buffer. --- src/tc/filters/cls_u32.rs | 31 ++++++++++++++++++++++++++++++- src/tc/filters/mod.rs | 1 + src/tc/mod.rs | 3 ++- src/tc/tests/filter_u32.rs | 21 +++++++++++++++++++++ 4 files changed, 54 insertions(+), 2 deletions(-) diff --git a/src/tc/filters/cls_u32.rs b/src/tc/filters/cls_u32.rs index 06327f5b..e4a4a585 100644 --- a/src/tc/filters/cls_u32.rs +++ b/src/tc/filters/cls_u32.rs @@ -177,7 +177,7 @@ pub struct TcU32Selector { pub keys: Vec, } -buffer!(TcU32SelectorBuffer(TC_U32_SEL_BUF_LEN) { +buffer!(TcU32SelectorBuffer { flags: (u8, 0), offshift: (u8, 1), nkeys: (u8, 2), @@ -190,6 +190,35 @@ buffer!(TcU32SelectorBuffer(TC_U32_SEL_BUF_LEN) { keys: (slice, TC_U32_SEL_BUF_LEN..), }); +impl> TcU32SelectorBuffer { + pub fn new_checked(buffer: T) -> Result { + let packet = Self::new(buffer); + packet.check_buffer_length()?; + Ok(packet) + } + + fn check_buffer_length(&self) -> Result<(), DecodeError> { + let len = self.buffer.as_ref().len(); + if len < TC_U32_SEL_BUF_LEN { + return Err(format!( + "invalid TcU32SelectorBuffer: length {len} < {TC_U32_SEL_BUF_LEN}" + ) + .into()); + } + // Expect the buffer to be large enough to hold `nkeys`. + let expected_len = + ((self.nkeys() as usize) * TC_U32_KEY_BUF_LEN) + TC_U32_SEL_BUF_LEN; + if len < expected_len { + return Err(format!( + "invalid RouteNextHopBuffer: length {} < {}", + len, expected_len, + ) + .into()); + } + Ok(()) + } +} + impl Emitable for TcU32Selector { fn buffer_len(&self) -> usize { TC_U32_SEL_BUF_LEN + (self.nkeys as usize * TC_U32_KEY_BUF_LEN) diff --git a/src/tc/filters/mod.rs b/src/tc/filters/mod.rs index 6d8efc5d..9e665a24 100644 --- a/src/tc/filters/mod.rs +++ b/src/tc/filters/mod.rs @@ -6,6 +6,7 @@ mod u32_flags; pub use self::cls_u32::{ TcFilterU32, TcFilterU32Option, TcU32Key, TcU32Selector, + TcU32SelectorBuffer, }; pub use self::matchall::{TcFilterMatchAll, TcFilterMatchAllOption}; pub use u32_flags::{TcU32OptionFlags, TcU32SelectorFlags}; diff --git a/src/tc/mod.rs b/src/tc/mod.rs index a083b968..9eb2cf2f 100644 --- a/src/tc/mod.rs +++ b/src/tc/mod.rs @@ -20,7 +20,8 @@ pub use self::actions::{ pub use self::attribute::TcAttribute; pub use self::filters::{ TcFilterMatchAll, TcFilterMatchAllOption, TcFilterU32, TcFilterU32Option, - TcU32Key, TcU32OptionFlags, TcU32Selector, TcU32SelectorFlags, + TcU32Key, TcU32OptionFlags, TcU32Selector, TcU32SelectorBuffer, + TcU32SelectorFlags, }; pub use self::header::{TcHandle, TcHeader, TcMessageBuffer}; pub use self::message::TcMessage; diff --git a/src/tc/tests/filter_u32.rs b/src/tc/tests/filter_u32.rs index 30f124b3..ce99cabd 100644 --- a/src/tc/tests/filter_u32.rs +++ b/src/tc/tests/filter_u32.rs @@ -9,6 +9,7 @@ use crate::{ filters::{TcU32OptionFlags, TcU32SelectorFlags}, TcAttribute, TcFilterU32Option, TcHandle, TcHeader, TcMessage, TcMessageBuffer, TcOption, TcU32Key, TcU32Selector, + TcU32SelectorBuffer, }, AddressFamily, }; @@ -112,3 +113,23 @@ fn test_get_filter_u32() { assert_eq!(buf, raw); } + +// Verify that [`TcU32Selector`] fails to parse a buffer with an +// invalid number of keys. +#[test] +fn test_tcu32_selector_invalid_nkeys() { + // TC u32 selector buffer layout: + // |byte0|byte1|byte2|byte3|byte4|byte5|byte6|byte7| + // |-----|-----|-----|-----|-----|-----|-----|-----| + // |flags|shift|nkeys|pad | offmask | off | + // |-----|-----|-----|-----|-----|-----|-----|-----| + // | offoff | hoff | hmask | + // |-----|-----|-----|-----|-----|-----|-----|-----| + // | keys | + // | ... | + let buffer = [ + 0x00, 0x00, 0xff, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, + ]; + assert!(TcU32SelectorBuffer::new_checked(buffer).is_err()); +}