From a71051d09b1113e23263a809bf0677776405491c Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 16 Jan 2025 09:26:30 +0100 Subject: [PATCH 1/6] protodetect: finish probing parser sooner Ticket: 7495 We want to finish also if we tested all the expected protocols in mask, or if we tested even more. There can be one more protocol coming from pe0, which can be the protocol already found in the other direction. (cherry picked from commit b5094b00b62ada4d8825f3c45898fc4c5e9d5b1f) --- src/app-layer-detect-proto.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c index c7f902edc22f..2341ab6fe140 100644 --- a/src/app-layer-detect-proto.c +++ b/src/app-layer-detect-proto.c @@ -614,7 +614,7 @@ static AppProto AppLayerProtoDetectPPGetProto(Flow *f, const uint8_t *buf, uint3 else if (pp_port_sp) mask = pp_port_sp->alproto_mask; - if (alproto_masks[0] == mask) { + if ((alproto_masks[0] & mask) == mask) { FLOW_SET_PP_DONE(f, dir); SCLogDebug("%s, mask is now %08x, needed %08x, so done", (dir == STREAM_TOSERVER) ? "toserver":"toclient", From bfcbe48e727761cc1d2ba4754e347676a6ee1601 Mon Sep 17 00:00:00 2001 From: Giuseppe Longo Date: Mon, 14 Aug 2023 20:10:36 +0200 Subject: [PATCH 2/6] rust/sip: rustfmt sip module cherry-picked from commit 8ff80cb84d0e42f0fec92128e629eb040959fbcb --- rust/src/sip/detect.rs | 29 ++++------------ rust/src/sip/log.rs | 2 +- rust/src/sip/sip.rs | 79 +++++++++++++++++++++++------------------- 3 files changed, 51 insertions(+), 59 deletions(-) diff --git a/rust/src/sip/detect.rs b/rust/src/sip/detect.rs index 63f636529a34..91df4fb29932 100644 --- a/rust/src/sip/detect.rs +++ b/rust/src/sip/detect.rs @@ -23,9 +23,7 @@ use std::ptr; #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_method( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.request { let m = &r.method; @@ -44,9 +42,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_method( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_uri( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.request { let p = &r.path; @@ -65,10 +61,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_uri( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_protocol( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, - direction: u8, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, direction: u8, ) -> u8 { match direction.into() { Direction::ToServer => { @@ -101,9 +94,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_protocol( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_stat_code( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.response { let c = &r.code; @@ -122,9 +113,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_stat_code( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_stat_msg( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.response { let re = &r.reason; @@ -143,9 +132,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_stat_msg( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_request_line( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.request_line { if !r.is_empty() { @@ -163,9 +150,7 @@ pub unsafe extern "C" fn rs_sip_tx_get_request_line( #[no_mangle] pub unsafe extern "C" fn rs_sip_tx_get_response_line( - tx: &mut SIPTransaction, - buffer: *mut *const u8, - buffer_len: *mut u32, + tx: &mut SIPTransaction, buffer: *mut *const u8, buffer_len: *mut u32, ) -> u8 { if let Some(ref r) = tx.response_line { if !r.is_empty() { diff --git a/rust/src/sip/log.rs b/rust/src/sip/log.rs index 792acfa49021..a7a98d5eb1b3 100644 --- a/rust/src/sip/log.rs +++ b/rust/src/sip/log.rs @@ -51,4 +51,4 @@ fn log(tx: &SIPTransaction, js: &mut JsonBuilder) -> Result<(), JsonError> { #[no_mangle] pub extern "C" fn rs_sip_log_json(tx: &mut SIPTransaction, js: &mut JsonBuilder) -> bool { log(tx, js).is_ok() -} \ No newline at end of file +} diff --git a/rust/src/sip/sip.rs b/rust/src/sip/sip.rs index 4e86f5ea476d..d1aefde548be 100755 --- a/rust/src/sip/sip.rs +++ b/rust/src/sip/sip.rs @@ -17,10 +17,10 @@ // written by Giuseppe Longo -use crate::frames::*; use crate::applayer::{self, *}; use crate::core; use crate::core::{AppProto, Flow, ALPROTO_UNKNOWN}; +use crate::frames::*; use crate::sip::parser::*; use nom7::Err; use std; @@ -96,10 +96,7 @@ impl SIPState { } fn free_tx(&mut self, tx_id: u64) { - let tx = self - .transactions - .iter() - .position(|tx| tx.id == tx_id + 1); + let tx = self.transactions.iter().position(|tx| tx.id == tx_id + 1); debug_assert!(tx.is_some()); if let Some(idx) = tx { let _ = self.transactions.remove(idx); @@ -149,7 +146,13 @@ impl SIPState { fn parse_response(&mut self, flow: *const core::Flow, stream_slice: StreamSlice) -> bool { let input = stream_slice.as_slice(); - let _pdu = Frame::new(flow, &stream_slice, input, input.len() as i64, SIPFrameType::Pdu as u8); + let _pdu = Frame::new( + flow, + &stream_slice, + input, + input.len() as i64, + SIPFrameType::Pdu as u8, + ); SCLogDebug!("tc: pdu {:?}", _pdu); match sip_parse_response(input) { @@ -224,20 +227,40 @@ fn sip_frames_ts(flow: *const core::Flow, stream_slice: &StreamSlice, r: &Reques fn sip_frames_tc(flow: *const core::Flow, stream_slice: &StreamSlice, r: &Response) { let oi = stream_slice.as_slice(); - let _f = Frame::new(flow, stream_slice, oi, r.response_line_len as i64, SIPFrameType::ResponseLine as u8); - let hi = &oi[r.response_line_len as usize ..]; + let _f = Frame::new( + flow, + stream_slice, + oi, + r.response_line_len as i64, + SIPFrameType::ResponseLine as u8, + ); + let hi = &oi[r.response_line_len as usize..]; SCLogDebug!("tc: response_line {:?}", _f); - let _f = Frame::new(flow, stream_slice, hi, r.headers_len as i64, SIPFrameType::ResponseHeaders as u8); + let _f = Frame::new( + flow, + stream_slice, + hi, + r.headers_len as i64, + SIPFrameType::ResponseHeaders as u8, + ); SCLogDebug!("tc: response_headers {:?}", _f); if r.body_len > 0 { - let bi = &oi[r.body_offset as usize ..]; - let _f = Frame::new(flow, stream_slice, bi, r.body_len as i64, SIPFrameType::ResponseBody as u8); + let bi = &oi[r.body_offset as usize..]; + let _f = Frame::new( + flow, + stream_slice, + bi, + r.body_len as i64, + SIPFrameType::ResponseBody as u8, + ); SCLogDebug!("tc: response_body {:?}", _f); } } #[no_mangle] -pub extern "C" fn rs_sip_state_new(_orig_state: *mut std::os::raw::c_void, _orig_proto: AppProto) -> *mut std::os::raw::c_void { +pub extern "C" fn rs_sip_state_new( + _orig_state: *mut std::os::raw::c_void, _orig_proto: AppProto, +) -> *mut std::os::raw::c_void { let state = SIPState::new(); let boxed = Box::new(state); return Box::into_raw(boxed) as *mut _; @@ -251,8 +274,7 @@ pub extern "C" fn rs_sip_state_free(state: *mut std::os::raw::c_void) { #[no_mangle] pub unsafe extern "C" fn rs_sip_state_get_tx( - state: *mut std::os::raw::c_void, - tx_id: u64, + state: *mut std::os::raw::c_void, tx_id: u64, ) -> *mut std::os::raw::c_void { let state = cast_pointer!(state, SIPState); match state.get_tx_by_id(tx_id) { @@ -275,8 +297,7 @@ pub unsafe extern "C" fn rs_sip_state_tx_free(state: *mut std::os::raw::c_void, #[no_mangle] pub extern "C" fn rs_sip_tx_get_alstate_progress( - _tx: *mut std::os::raw::c_void, - _direction: u8, + _tx: *mut std::os::raw::c_void, _direction: u8, ) -> std::os::raw::c_int { 1 } @@ -285,11 +306,7 @@ static mut ALPROTO_SIP: AppProto = ALPROTO_UNKNOWN; #[no_mangle] pub unsafe extern "C" fn rs_sip_probing_parser_ts( - _flow: *const Flow, - _direction: u8, - input: *const u8, - input_len: u32, - _rdir: *mut u8, + _flow: *const Flow, _direction: u8, input: *const u8, input_len: u32, _rdir: *mut u8, ) -> AppProto { let buf = build_slice!(input, input_len as usize); if sip_parse_request(buf).is_ok() { @@ -300,11 +317,7 @@ pub unsafe extern "C" fn rs_sip_probing_parser_ts( #[no_mangle] pub unsafe extern "C" fn rs_sip_probing_parser_tc( - _flow: *const Flow, - _direction: u8, - input: *const u8, - input_len: u32, - _rdir: *mut u8, + _flow: *const Flow, _direction: u8, input: *const u8, input_len: u32, _rdir: *mut u8, ) -> AppProto { let buf = build_slice!(input, input_len as usize); if sip_parse_response(buf).is_ok() { @@ -315,11 +328,8 @@ pub unsafe extern "C" fn rs_sip_probing_parser_tc( #[no_mangle] pub unsafe extern "C" fn rs_sip_parse_request( - flow: *const core::Flow, - state: *mut std::os::raw::c_void, - _pstate: *mut std::os::raw::c_void, - stream_slice: StreamSlice, - _data: *const std::os::raw::c_void, + flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + stream_slice: StreamSlice, _data: *const std::os::raw::c_void, ) -> AppLayerResult { let state = cast_pointer!(state, SIPState); state.parse_request(flow, stream_slice).into() @@ -327,11 +337,8 @@ pub unsafe extern "C" fn rs_sip_parse_request( #[no_mangle] pub unsafe extern "C" fn rs_sip_parse_response( - flow: *const core::Flow, - state: *mut std::os::raw::c_void, - _pstate: *mut std::os::raw::c_void, - stream_slice: StreamSlice, - _data: *const std::os::raw::c_void, + flow: *const core::Flow, state: *mut std::os::raw::c_void, _pstate: *mut std::os::raw::c_void, + stream_slice: StreamSlice, _data: *const std::os::raw::c_void, ) -> AppLayerResult { let state = cast_pointer!(state, SIPState); state.parse_response(flow, stream_slice).into() From aabaa959133a3b6a40636a140d2d49722addf174 Mon Sep 17 00:00:00 2001 From: Giuseppe Longo Date: Tue, 1 Aug 2023 20:50:17 +0200 Subject: [PATCH 3/6] sip/parser: accept valid chars Accepts valid characters as defined in RFC3261. cherry-picked from commit 7e993d5081c35d62eada429bad43430417267e5a --- rust/src/sip/parser.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/rust/src/sip/parser.rs b/rust/src/sip/parser.rs index a34bc2615e53..c7d616a498b4 100644 --- a/rust/src/sip/parser.rs +++ b/rust/src/sip/parser.rs @@ -15,7 +15,7 @@ * 02110-1301, USA. */ -// written by Giuseppe Longo +// written by Giuseppe Longo use nom7::bytes::streaming::{take, take_while, take_while1}; use nom7::character::streaming::{char, crlf}; @@ -57,9 +57,13 @@ pub struct Response { pub body_len: u16, } +/** + * Valid tokens and chars are defined in RFC3261: + * https://www.rfc-editor.org/rfc/rfc3261#section-25.1 + */ #[inline] fn is_token_char(b: u8) -> bool { - is_alphanumeric(b) || b"!%'*+-._`".contains(&b) + is_alphanumeric(b) || b"!%'*+-._`~".contains(&b) } #[inline] @@ -69,7 +73,7 @@ fn is_method_char(b: u8) -> bool { #[inline] fn is_request_uri_char(b: u8) -> bool { - is_alphanumeric(b) || is_token_char(b) || b"~#@:".contains(&b) + is_alphanumeric(b) || is_token_char(b) || b"~#@:;=?+&$,/".contains(&b) } #[inline] From b3e6a8f15d94a00018d6f749292791636f9d2044 Mon Sep 17 00:00:00 2001 From: Giuseppe Longo Date: Sat, 25 Nov 2023 09:39:54 +0100 Subject: [PATCH 4/6] sip/parser: enforce valid chars for sip version The `is_version_char` function incorrectly allowed characters that are not part of the valid SIP version "SIP/2.0". For instance, 'HTTP/1.1' was mistakenly accepted as a valid SIP version, although it's not. This commit fixes the issue by updating the condition to strictly check for the correct version string. cherry-picked from commit 69f841c9981147f55ec9f76d44f7ac138e726304 --- rust/src/sip/parser.rs | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/rust/src/sip/parser.rs b/rust/src/sip/parser.rs index c7d616a498b4..cdf5fb136cfe 100644 --- a/rust/src/sip/parser.rs +++ b/rust/src/sip/parser.rs @@ -17,9 +17,9 @@ // written by Giuseppe Longo -use nom7::bytes::streaming::{take, take_while, take_while1}; +use nom7::bytes::streaming::{tag, take, take_while, take_while1}; use nom7::character::streaming::{char, crlf}; -use nom7::character::{is_alphabetic, is_alphanumeric, is_space}; +use nom7::character::{is_alphabetic, is_alphanumeric, is_digit, is_space}; use nom7::combinator::map_res; use nom7::sequence::delimited; use nom7::{Err, IResult, Needed}; @@ -78,7 +78,7 @@ fn is_request_uri_char(b: u8) -> bool { #[inline] fn is_version_char(b: u8) -> bool { - is_alphanumeric(b) || b"./".contains(&b) + is_digit(b) || b".".contains(&b) } #[inline] @@ -111,7 +111,7 @@ pub fn sip_parse_request(oi: &[u8]) -> IResult<&[u8], Request> { Request { method: method.into(), path: path.into(), - version: version.into(), + version, headers, request_line_len: request_line_len as u16, @@ -137,7 +137,7 @@ pub fn sip_parse_response(oi: &[u8]) -> IResult<&[u8], Response> { Ok(( bi, Response { - version: version.into(), + version, code: code.into(), reason: reason.into(), @@ -160,8 +160,10 @@ fn parse_request_uri(i: &[u8]) -> IResult<&[u8], &str> { } #[inline] -fn parse_version(i: &[u8]) -> IResult<&[u8], &str> { - map_res(take_while1(is_version_char), std::str::from_utf8)(i) +fn parse_version(i: &[u8]) -> IResult<&[u8], String> { + let (i, prefix) = map_res(tag("SIP/"), std::str::from_utf8)(i)?; + let (i, version) = map_res(take_while1(is_version_char), std::str::from_utf8)(i)?; + Ok((i, format!("{}{}", prefix, version))) } #[inline] @@ -326,4 +328,20 @@ mod tests { } } } + + #[test] + fn test_parse_invalid_version() { + let buf: &[u8] = "HTTP/1.1\r\n".as_bytes(); + + // This test must fail if 'HTTP/1.1' is accepted + assert!(parse_version(buf).is_err()); + } + + #[test] + fn test_parse_valid_version() { + let buf: &[u8] = "SIP/2.0\r\n".as_bytes(); + + let (_rem, result) = parse_version(buf).unwrap(); + assert_eq!(result, "SIP/2.0"); + } } From 7ffd985828b56e9b8dd39c7c354dce55b16fec20 Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Mon, 20 Jan 2025 08:46:39 -0500 Subject: [PATCH 5/6] detect/csum: rm interaction btw stream setting/csum Issue: 7467 Stream checksum validation no longer has a side effect of setting PKT_IGNORE_CHECKSUM and thus, no longer affects csum keyword checks. (cherry picked from commit 758da982f087dce249012304de7d3077adf9bade) --- src/stream-tcp.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/stream-tcp.c b/src/stream-tcp.c index 46b379f7de16..b847fb6f8a00 100644 --- a/src/stream-tcp.c +++ b/src/stream-tcp.c @@ -5815,11 +5815,7 @@ TmEcode StreamTcp (ThreadVars *tv, Packet *p, void *data, PacketQueueNoLock *pq) StatsIncr(tv, stt->counter_tcp_invalid_checksum); return TM_ECODE_OK; } - } else { - p->flags |= PKT_IGNORE_CHECKSUM; } - } else { - p->flags |= PKT_IGNORE_CHECKSUM; //TODO check that this is set at creation } AppLayerProfilingReset(stt->ra_ctx->app_tctx); From d56c078193e4a4c357d6250da00e38cb0f3ad6a7 Mon Sep 17 00:00:00 2001 From: Jeff Lucovsky Date: Tue, 21 Jan 2025 09:21:24 -0500 Subject: [PATCH 6/6] doc/csum: Stream checksum validation change Describe the change of behavior between the stream.checksum-validation setting and checksum-based rule keywords. (cherry picked from commit cfbf8fda94771461844b0fc805af5476f92328ce) --- doc/userguide/upgrade.rst | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index 6da52adb1f88..2cc195fd260b 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -39,7 +39,7 @@ Upgrading to 7.0.8 - Unknown requirements in the ``requires`` keyword will now be treated as unsatisfied requirements, causing the rule to not be loaded. See :ref:`keyword_requires`. To opt out of this change and to ignore - uknown requirements, effectively treating them as satified the + unknown requirements, effectively treating them as satisfied the ``ignore-unknown-requirements`` configuration option can be used. Command line example:: @@ -66,6 +66,13 @@ Upgrading to 7.0.8 the engine will NOT log any transaction metadata if there is more than one live transaction, to reduce the chances of logging unrelated data.** This may lead to what looks like a regression in behavior, but it is a considered choice. +- The configuration setting controlling stream checksum checks no longer affects + checksum keyword validation. In previous Suricata versions, when ``stream.checksum-validation`` + was set to ``no``, the checksum keywords (e.g., ``ipv4-csum``, ``tcpv4-csum``, etc) + will always consider it valid; e.g., ``tcpv4-csum: invalid`` will never match. Now, + ``stream.checksum-validation`` no longer affects the checksum rule keywords. + E.g., ``ipv4-csum: valid`` will only match if the check sum is valid, even when engine + checksum validations are disabled. Upgrading 6.0 to 7.0 --------------------