From: Emmanuel Thompson Date: Wed, 20 May 2020 14:58:34 +0000 (-0400) Subject: detect/asn1: Update relative_offset keyword X-Git-Tag: suricata-6.0.0-beta1~235 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=88601b19932c27e81d9a7d2ca445059bb879d69c;p=thirdparty%2Fsuricata.git detect/asn1: Update relative_offset keyword - To be consistent with recent C version changes - Add checks for over/underflows --- diff --git a/rust/src/asn1/mod.rs b/rust/src/asn1/mod.rs index cadbc0c164..9486ce843d 100644 --- a/rust/src/asn1/mod.rs +++ b/rust/src/asn1/mod.rs @@ -192,36 +192,40 @@ impl Asn1 { /// Decodes Asn1 objects from an input + length while applying the offset /// defined in the asn1 keyword options fn asn1_decode( - input: *const u8, - input_len: u32, + buffer: &'static [u8], + buffer_offset: u32, ad: &DetectAsn1Data, ) -> Result { // Get offset let offset = if let Some(absolute_offset) = ad.absolute_offset { - absolute_offset as isize + absolute_offset } else if let Some(relative_offset) = ad.relative_offset { - relative_offset as isize + // relative offset in regards to the last content match + + // buffer_offset (u32) + relative_offset (i32) => offset (u16) + u16::try_from({ + if relative_offset > 0 { + buffer_offset + .checked_add(u32::try_from(relative_offset)?) + .ok_or(Asn1DecodeError::InvalidKeywordParameter)? + } else { + buffer_offset + .checked_sub(u32::try_from(-relative_offset)?) + .ok_or(Asn1DecodeError::InvalidKeywordParameter)? + } + }) + .or(Err(Asn1DecodeError::InvalidKeywordParameter))? } else { 0 }; - // Make sure we won't read past the end of the buffer - if offset >= input_len as isize { + // Make sure we won't read past the end or front of the buffer + if offset as usize >= buffer.len() { return Err(Asn1DecodeError::InvalidKeywordParameter); } - // Apply offset to input pointer - let input = unsafe { input.offset(offset) }; - - // Adjust the length - let input_len = (input_len as isize) - .checked_sub(offset) - .ok_or(Asn1DecodeError::InvalidKeywordParameter)?; - let input_len = - usize::try_from(input_len).map_err(|_| Asn1DecodeError::InvalidKeywordParameter)?; - - // Get the slice from memory - let slice = build_slice!(input, input_len); + // Get slice from buffer at offset + let slice = &buffer[offset as usize..]; Asn1::from_slice(slice, ad) } @@ -236,16 +240,19 @@ fn asn1_decode( #[no_mangle] pub unsafe extern "C" fn rs_asn1_decode( input: *const u8, - input_len: u32, + input_len: u16, + buffer_offset: u32, ad_ptr: *const DetectAsn1Data, ) -> *mut Asn1 { if input.is_null() || input_len == 0 || ad_ptr.is_null() { return std::ptr::null_mut(); } + let slice = build_slice!(input, input_len as usize); + let ad = &*ad_ptr; - let res = asn1_decode(input, input_len, ad); + let res = asn1_decode(slice, buffer_offset, ad); match res { Ok(asn1) => Box::into_raw(Box::new(asn1)), @@ -296,6 +303,12 @@ pub unsafe extern "C" fn rs_asn1_checks( 0 } +impl From for Asn1DecodeError { + fn from(_e: std::num::TryFromIntError) -> Asn1DecodeError { + Asn1DecodeError::InvalidKeywordParameter + } +} + impl From> for Asn1DecodeError { fn from(e: nom::Err) -> Asn1DecodeError { match e { diff --git a/rust/src/asn1/parse_rules.rs b/rust/src/asn1/parse_rules.rs index 592b8b6cac..a9c3b3f22d 100644 --- a/rust/src/asn1/parse_rules.rs +++ b/rust/src/asn1/parse_rules.rs @@ -19,7 +19,7 @@ use crate::log::*; use nom::branch::alt; use nom::bytes::complete::tag; use nom::character::complete::{digit1, multispace0, multispace1}; -use nom::combinator::{map_res, opt}; +use nom::combinator::{map_res, opt, verify}; use nom::sequence::{separated_pair, tuple}; use nom::IResult; use std::ffi::CStr; @@ -84,7 +84,7 @@ pub struct DetectAsn1Data { pub bitstring_overflow: bool, pub double_overflow: bool, pub oversize_length: Option, - pub absolute_offset: Option, + pub absolute_offset: Option, pub relative_offset: Option, pub max_frames: u16, } @@ -105,6 +105,11 @@ impl Default for DetectAsn1Data { fn parse_u32_number(input: &str) -> IResult<&str, u32> { map_res(digit1, |digits: &str| digits.parse::())(input) } + +fn parse_u16_number(input: &str) -> IResult<&str, u16> { + map_res(digit1, |digits: &str| digits.parse::())(input) +} + fn parse_i32_number(input: &str) -> IResult<&str, i32> { let (rest, negate) = opt(tag("-"))(input)?; let (rest, d) = map_res(digit1, |s: &str| s.parse::())(rest)?; @@ -135,12 +140,18 @@ pub(super) fn asn1_parse_rule(input: &str) -> IResult<&str, DetectAsn1Data> { separated_pair(tag("oversize_length"), multispace1, parse_u32_number)(i) } - fn absolute_offset(i: &str) -> IResult<&str, (&str, u32)> { - separated_pair(tag("absolute_offset"), multispace1, parse_u32_number)(i) + fn absolute_offset(i: &str) -> IResult<&str, (&str, u16)> { + separated_pair(tag("absolute_offset"), multispace1, parse_u16_number)(i) } fn relative_offset(i: &str) -> IResult<&str, (&str, i32)> { - separated_pair(tag("relative_offset"), multispace1, parse_i32_number)(i) + separated_pair( + tag("relative_offset"), + multispace1, + verify(parse_i32_number, |v| { + *v >= -i32::from(std::u16::MAX) && *v <= i32::from(std::u16::MAX) + }), + )(i) } let mut data = DetectAsn1Data::default(); @@ -202,6 +213,18 @@ mod tests { #[test_case("oversize_length 1024", DetectAsn1Data { oversize_length: Some(1024), ..Default::default()}; "check that we parse oversize_length correctly")] + #[test_case("oversize_length 0", + DetectAsn1Data { oversize_length: Some(0), ..Default::default()}; + "check lower bound on oversize_length")] + #[test_case("oversize_length -1", + DetectAsn1Data::default() => panics "Error((\"oversize_length -1\", Verify))"; + "check under lower bound on oversize_length")] + #[test_case("oversize_length 4294967295", + DetectAsn1Data { oversize_length: Some(4294967295), ..Default::default()}; + "check upper bound on oversize_length")] + #[test_case("oversize_length 4294967296", + DetectAsn1Data::default() => panics "Error((\"oversize_length 4294967296\", Verify))"; + "check over upper bound on oversize_length")] #[test_case("oversize_length", DetectAsn1Data::default() => panics "Error((\"oversize_length\", Verify))"; "check that we fail if the needed arg oversize_length is not given")] @@ -209,6 +232,18 @@ mod tests { #[test_case("absolute_offset 1024", DetectAsn1Data { absolute_offset: Some(1024), ..Default::default()}; "check that we parse absolute_offset correctly")] + #[test_case("absolute_offset 0", + DetectAsn1Data { absolute_offset: Some(0), ..Default::default()}; + "check lower bound on absolute_offset")] + #[test_case("absolute_offset -1", + DetectAsn1Data::default() => panics "Error((\"absolute_offset -1\", Verify))"; + "check under lower bound on absolute_offset")] + #[test_case("absolute_offset 65535", + DetectAsn1Data { absolute_offset: Some(65535), ..Default::default()}; + "check upper bound on absolute_offset")] + #[test_case("absolute_offset 65536", + DetectAsn1Data::default() => panics "Error((\"absolute_offset 65536\", Verify))"; + "check over upper bound on absolute_offset")] #[test_case("absolute_offset", DetectAsn1Data::default() => panics "Error((\"absolute_offset\", Verify))"; "check that we fail if the needed arg absolute_offset is not given")] @@ -216,6 +251,18 @@ mod tests { #[test_case("relative_offset 1024", DetectAsn1Data { relative_offset: Some(1024), ..Default::default()}; "check that we parse relative_offset correctly")] + #[test_case("relative_offset -65535", + DetectAsn1Data { relative_offset: Some(-65535), ..Default::default()}; + "check lower bound on relative_offset")] + #[test_case("relative_offset -65536", + DetectAsn1Data::default() => panics "Error((\"relative_offset -65536\", Verify))"; + "check under lower bound on relative_offset")] + #[test_case("relative_offset 65535", + DetectAsn1Data { relative_offset: Some(65535), ..Default::default()}; + "check upper bound on relative_offset")] + #[test_case("relative_offset 65536", + DetectAsn1Data::default() => panics "Error((\"relative_offset 65536\", Verify))"; + "check over upper bound on relative_offset")] #[test_case("relative_offset", DetectAsn1Data::default() => panics "Error((\"relative_offset\", Verify))"; "check that we fail if the needed arg relative_offset is not given")] diff --git a/src/detect-asn1.c b/src/detect-asn1.c index 964987da5c..37c5e290ff 100644 --- a/src/detect-asn1.c +++ b/src/detect-asn1.c @@ -81,7 +81,7 @@ static int DetectAsn1Match(DetectEngineThreadCtx *det_ctx, Packet *p, const DetectAsn1Data *ad = (const DetectAsn1Data *)ctx; - Asn1 *asn1 = rs_asn1_decode(p->payload, p->payload_len, ad); + Asn1 *asn1 = rs_asn1_decode(p->payload, p->payload_len, det_ctx->buffer_offset, ad); ret = rs_asn1_checks(asn1, ad); @@ -223,7 +223,7 @@ static int DetectAsn1TestReal01(void) "content:\"Pablo\"; asn1:absolute_offset 0, " "oversize_length 130; sid:1;)"; sigs[1]= "alert ip any any -> any any (msg:\"Testing id 2\"; " - "content:\"AA\"; asn1:relative_offset 2, " + "content:\"AA\"; asn1:relative_offset 0, " "oversize_length 130; sid:2;)"; sigs[2]= "alert ip any any -> any any (msg:\"Testing id 3\"; " "content:\"lalala\"; asn1: oversize_length 2000; sid:3;)"; @@ -302,7 +302,7 @@ static int DetectAsn1TestReal02(void) "content:\"Pablo\"; asn1:absolute_offset 0, " "oversize_length 140; sid:1;)"; sigs[1]= "alert ip any any -> any any (msg:\"Testing id 2\"; " - "content:\"AA\"; asn1:relative_offset 2, " + "content:\"AA\"; asn1:relative_offset 0, " "oversize_length 140; sid:2;)"; sigs[2]= "alert ip any any -> any any (msg:\"Testing id 3\"; " "content:\"lalala\"; asn1: oversize_length 2000; sid:3;)"; @@ -440,7 +440,7 @@ static int DetectAsn1TestReal04(void) "content:\"Pablo\"; asn1:absolute_offset 0, " "oversize_length 140; sid:1;)"; sigs[1]= "alert ip any any -> any any (msg:\"Testing id 2\"; " - "content:\"John\"; asn1:relative_offset -7, " + "content:\"John\"; asn1:relative_offset -11, " "oversize_length 140; sid:2;)"; sigs[2]= "alert ip any any -> any any (msg:\"Testing id 3\"; " "content:\"lalala\"; asn1: oversize_length 2000; sid:3;)";