From df0e9f106b8bdfa8ceae52382890f406e5f545e1 Mon Sep 17 00:00:00 2001 From: Shivani Bhardwaj Date: Thu, 20 Aug 2026 13:53:25 +0530 Subject: [PATCH] mqtt: add too_many_properties event and limit So far, the properties parsed across various different types of MQTT messages was unbounded and could be exploited. Add a limit of 255 on the maximum number of properties that any message type can store. Additionally, raise an event for too many properties indicating an anomaly in the network traffic. Ticket: 8839 --- rules/mqtt-events.rules | 1 + rust/src/mqtt/detect.rs | 4 +++ rust/src/mqtt/mqtt.rs | 44 ++++++++++++++++++++---- rust/src/mqtt/mqtt_message.rs | 12 ++++++- rust/src/mqtt/parser.rs | 64 +++++++++++++++++++++++++---------- 5 files changed, 100 insertions(+), 25 deletions(-) diff --git a/rules/mqtt-events.rules b/rules/mqtt-events.rules index e4c8f6b10e..23ba39c478 100644 --- a/rules/mqtt-events.rules +++ b/rules/mqtt-events.rules @@ -14,3 +14,4 @@ alert mqtt any any -> any any (msg:"SURICATA MQTT unassigned message type (0 or alert mqtt any any -> any any (msg:"SURICATA MQTT too many transactions"; app-layer-event:mqtt.too_many_transactions; classtype:protocol-command-decode; sid:2229009; rev:1;) alert mqtt any any -> any any (msg:"SURICATA MQTT malformed traffic"; app-layer-event:mqtt.malformed_traffic; classtype:protocol-command-decode; sid:2229010; rev:1;) alert mqtt any any -> any any (msg:"SURICATA MQTT too many messages"; app-layer-event:mqtt.too_many_messages; classtype:protocol-command-decode; sid:2229011; rev:1;) +alert mqtt any any -> any any (msg:"SURICATA MQTT too many properties"; app-layer-event:mqtt.too_many_properties; classtype:protocol-command-decode; sid:2229012; rev:1;) diff --git a/rust/src/mqtt/detect.rs b/rust/src/mqtt/detect.rs index 36a0d65e4e..8007173311 100644 --- a/rust/src/mqtt/detect.rs +++ b/rust/src/mqtt/detect.rs @@ -1293,6 +1293,7 @@ mod test { message_id: 1, topics: vec!["foo".to_string(), "baar".to_string()], properties: None, + properties_truncated: false, }), }, Direction::ToServer, @@ -1309,6 +1310,7 @@ mod test { message_id: 1, topics: vec!["fieee".to_string(), "baaaaz".to_string()], properties: None, + properties_truncated: false, }), }); let mut s: *const u8 = std::ptr::null_mut(); @@ -1359,6 +1361,7 @@ mod test { }, ], properties: None, + properties_truncated: false, }), }, Direction::ToServer, @@ -1384,6 +1387,7 @@ mod test { }, ], properties: None, + properties_truncated: false, }), }); let mut s: *const u8 = std::ptr::null_mut(); diff --git a/rust/src/mqtt/mqtt.rs b/rust/src/mqtt/mqtt.rs index e1f3e9f9c5..5a57046b4e 100644 --- a/rust/src/mqtt/mqtt.rs +++ b/rust/src/mqtt/mqtt.rs @@ -48,6 +48,30 @@ static mut MQTT_MAX_TX: usize = 1024; static mut MQTT_MAX_MSGS: usize = 1024; +pub(super) const MQTT_MAX_PROPERTIES: usize = 256; + +fn msg_has_too_many_properties(op: &MQTTOperation) -> bool { + match op { + MQTTOperation::CONNECT(c) => c.properties_truncated, + MQTTOperation::CONNACK(c) => c.properties_truncated, + MQTTOperation::PUBLISH(c) => c.properties_truncated, + MQTTOperation::PUBACK(c) + | MQTTOperation::PUBREC(c) + | MQTTOperation::PUBREL(c) + | MQTTOperation::PUBCOMP(c) => c.properties_truncated, + MQTTOperation::SUBSCRIBE(c) => c.properties_truncated, + MQTTOperation::SUBACK(c) => c.properties_truncated, + MQTTOperation::UNSUBSCRIBE(c) => c.properties_truncated, + MQTTOperation::UNSUBACK(c) => c.properties_truncated, + MQTTOperation::AUTH(c) => c.properties_truncated, + MQTTOperation::DISCONNECT(c) => c.properties_truncated, + MQTTOperation::UNASSIGNED + | MQTTOperation::PINGREQ + | MQTTOperation::PINGRESP + | MQTTOperation::TRUNCATED(_) => false, + } +} + pub(super) static mut ALPROTO_MQTT: AppProto = ALPROTO_UNKNOWN; #[derive(AppLayerFrameType)] @@ -71,6 +95,7 @@ pub enum MQTTEvent { TooManyTransactions, MalformedTraffic, TooManyMessages, + TooManyProperties, } #[derive(Debug)] @@ -88,10 +113,17 @@ pub struct MQTTTransaction { impl MQTTTransaction { pub fn new(msg: MQTTMessage, direction: Direction) -> MQTTTransaction { let mut m = MQTTTransaction::new_empty(direction); - m.msg.push(msg); + m.push_msg(msg); return m; } + fn push_msg(&mut self, msg: MQTTMessage) { + if msg_has_too_many_properties(&msg.op) { + self.tx_data.set_event(MQTTEvent::TooManyProperties as u8); + } + self.msg.push(msg); + } + pub fn new_empty(direction: Direction) -> MQTTTransaction { return MQTTTransaction { tx_id: 0, @@ -308,7 +340,7 @@ impl MQTTState { } MQTTOperation::CONNACK(ref _connack) => { if let Some(tx) = self.get_tx_by_pkt_id(MQTT_CONNECT_PKT_ID) { - tx.msg.push(msg); + tx.push_msg(msg); tx.complete = true; tx.pkt_id = None; self.connected = true; @@ -324,7 +356,7 @@ impl MQTTState { if tx.msg.len() >= unsafe { MQTT_MAX_MSGS } { tx.tx_data.set_event(MQTTEvent::TooManyMessages as u8); } else { - tx.msg.push(msg); + tx.push_msg(msg); } } else { let mut tx = self.new_tx(msg, toclient); @@ -338,7 +370,7 @@ impl MQTTState { } MQTTOperation::PUBACK(ref v) | MQTTOperation::PUBCOMP(ref v) => { if let Some(tx) = self.get_tx_by_pkt_id(v.message_id as u32) { - tx.msg.push(msg); + tx.push_msg(msg); tx.complete = true; tx.pkt_id = None; } else { @@ -353,7 +385,7 @@ impl MQTTState { } MQTTOperation::SUBACK(ref suback) => { if let Some(tx) = self.get_tx_by_pkt_id(suback.message_id as u32) { - tx.msg.push(msg); + tx.push_msg(msg); tx.complete = true; tx.pkt_id = None; } else { @@ -368,7 +400,7 @@ impl MQTTState { } MQTTOperation::UNSUBACK(ref unsuback) => { if let Some(tx) = self.get_tx_by_pkt_id(unsuback.message_id as u32) { - tx.msg.push(msg); + tx.push_msg(msg); tx.complete = true; tx.pkt_id = None; } else { diff --git a/rust/src/mqtt/mqtt_message.rs b/rust/src/mqtt/mqtt_message.rs index 2ba42273ec..8526a3af09 100644 --- a/rust/src/mqtt/mqtt_message.rs +++ b/rust/src/mqtt/mqtt_message.rs @@ -103,6 +103,7 @@ pub struct MQTTConnectData { pub username: Option, pub password: Option>, pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, pub will_properties: Option>, // MQTT 5.0 } @@ -111,6 +112,7 @@ pub struct MQTTConnackData { pub return_code: u8, pub session_present: bool, // MQTT 3.1.1 pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] @@ -119,6 +121,7 @@ pub struct MQTTPublishData { pub message_id: Option, pub message: Vec, pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] @@ -126,6 +129,7 @@ pub struct MQTTMessageIdOnly { pub message_id: u16, pub reason_code: Option, // MQTT 5.0 pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] @@ -139,6 +143,7 @@ pub struct MQTTSubscribeData { pub message_id: u16, pub topics: Vec, pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] @@ -146,6 +151,7 @@ pub struct MQTTSubackData { pub message_id: u16, pub qoss: Vec, pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] @@ -153,25 +159,29 @@ pub struct MQTTUnsubscribeData { pub message_id: u16, pub topics: Vec, pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] pub struct MQTTUnsubackData { pub message_id: u16, pub properties: Option>, // MQTT 5.0 - pub reason_codes: Option>, // MQTT 5.0 + pub properties_truncated: bool, + pub reason_codes: Option>, // MQTT 5.0 } #[derive(Debug)] pub struct MQTTAuthData { pub reason_code: u8, // MQTT 5.0 pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] pub struct MQTTDisconnectData { pub reason_code: Option, // MQTT 5.0 pub properties: Option>, // MQTT 5.0 + pub properties_truncated: bool, } #[derive(Debug)] diff --git a/rust/src/mqtt/parser.rs b/rust/src/mqtt/parser.rs index 39da6f96fa..236fab18eb 100644 --- a/rust/src/mqtt/parser.rs +++ b/rust/src/mqtt/parser.rs @@ -18,6 +18,7 @@ // written by Sascha Steinbiss use crate::common::nom8::bits; +use crate::mqtt::mqtt::MQTT_MAX_PROPERTIES; use crate::mqtt::mqtt_message::*; use crate::mqtt::mqtt_property::*; use nom8::bits::streaming::take as take_bits; @@ -98,33 +99,40 @@ fn parse_property(i: &[u8]) -> IResult<&[u8], MQTTProperty> { } #[inline] -fn parse_properties(input: &[u8], precond: bool) -> IResult<&[u8], Option>> { +fn parse_properties( + input: &[u8], precond: bool, +) -> IResult<&[u8], (Option>, bool)> { // do not try to parse anything when precondition is not met if !precond { - return Ok((input, None)); + return Ok((input, (None, false))); } // parse properties length match parse_mqtt_variable_integer(input) { Ok((rem, proplen)) => { if proplen == 0 { // no properties - return Ok((rem, None)); + return Ok((rem, (None, false))); } - // parse properties + // Parse properties, keeping at most MQTT_MAX_PROPERTIES of them let mut props = Vec::::new(); + let mut truncated = false; let (rem, mut newrem) = take(proplen as usize)(rem)?; while !newrem.is_empty() { match parse_property(newrem) { Ok((rem2, val)) => { if val != MQTTProperty::UNKNOWN { - props.push(val); + if props.len() < MQTT_MAX_PROPERTIES { + props.push(val); + } else { + truncated = true; + } } newrem = rem2; } Err(e) => return Err(e), } } - return Ok((rem, Some(props))); + return Ok((rem, (Some(props), truncated))); } Err(e) => return Err(e), } @@ -183,9 +191,10 @@ fn parse_connect(i: &[u8]) -> IResult<&[u8], MQTTConnectData> { let (i, protocol_version) = be_u8.parse(i)?; let (i, rawflags) = be_u8.parse(i)?; let (i, keepalive) = be_u16.parse(i)?; - let (i, properties) = parse_properties(i, protocol_version == 5)?; + let (i, (properties, properties_truncated)) = parse_properties(i, protocol_version == 5)?; let (i, client_id) = parse_mqtt_string(i)?; - let (i, will_properties) = parse_properties(i, protocol_version == 5 && rawflags & 0x4 != 0)?; + let (i, (will_properties, will_properties_truncated)) = + parse_properties(i, protocol_version == 5 && rawflags & 0x4 != 0)?; let (i, will_topic) = cond(rawflags & 0x4 != 0, parse_mqtt_string).parse(i)?; let (i, will_message) = cond(rawflags & 0x4 != 0, parse_mqtt_binary_data).parse(i)?; let (i, username) = cond(rawflags & 0x80 != 0, parse_mqtt_string).parse(i)?; @@ -210,6 +219,7 @@ fn parse_connect(i: &[u8]) -> IResult<&[u8], MQTTConnectData> { password, properties, will_properties, + properties_truncated: properties_truncated || will_properties_truncated, }, )) } @@ -219,13 +229,14 @@ fn parse_connack(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQTTC move |i: &[u8]| { let (i, topic_name_compression_response) = be_u8(i)?; let (i, return_code) = be_u8(i)?; - let (i, properties) = parse_properties(i, protocol_version == 5)?; + let (i, (properties, properties_truncated)) = parse_properties(i, protocol_version == 5)?; Ok(( i, MQTTConnackData { session_present: (topic_name_compression_response & 1) != 0, return_code, properties, + properties_truncated, }, )) } @@ -238,7 +249,8 @@ fn parse_publish( move |i: &[u8]| { let (i, topic) = parse_mqtt_string(i)?; let (i, message_id) = cond(has_id, be_u16).parse(i)?; - let (message, properties) = parse_properties(i, protocol_version == 5)?; + let (message, (properties, properties_truncated)) = + parse_properties(i, protocol_version == 5)?; Ok(( i, MQTTPublishData { @@ -246,6 +258,7 @@ fn parse_publish( message_id, message: message.to_vec(), properties, + properties_truncated, }, )) } @@ -273,6 +286,7 @@ fn parse_msgidonly(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQT message_id, reason_code: Some(0), properties: None, + properties_truncated: false, }, )); } @@ -288,17 +302,19 @@ fn parse_msgidonly(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQT message_id, reason_code: Some(reason_code), properties: None, + properties_truncated: false, }, )); } match parse_properties(rem, true) { - Ok((rem, properties)) => { + Ok((rem, (properties, properties_truncated))) => { return Ok(( rem, MQTTMessageIdOnly { message_id, reason_code: Some(reason_code), properties, + properties_truncated, }, )); } @@ -322,6 +338,7 @@ fn parse_msgidonly_v3(i: &[u8]) -> IResult<&[u8], MQTTMessageIdOnly> { message_id, reason_code: None, properties: None, + properties_truncated: false, }, )) } @@ -337,7 +354,7 @@ fn parse_subscribe_topic(i: &[u8]) -> IResult<&[u8], MQTTSubscribeTopicData> { fn parse_subscribe(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQTTSubscribeData> { move |i: &[u8]| { let (i, message_id) = be_u16.parse(i)?; - let (i, properties) = parse_properties(i, protocol_version == 5)?; + let (i, (properties, properties_truncated)) = parse_properties(i, protocol_version == 5)?; let (i, topics) = many1(complete(parse_subscribe_topic)).parse(i)?; Ok(( i, @@ -345,6 +362,7 @@ fn parse_subscribe(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQT message_id, topics, properties, + properties_truncated, }, )) } @@ -354,13 +372,15 @@ fn parse_subscribe(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQT fn parse_suback(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQTTSubackData> { move |i: &[u8]| { let (i, message_id) = be_u16(i)?; - let (qoss, properties) = parse_properties(i, protocol_version == 5)?; + let (qoss, (properties, properties_truncated)) = + parse_properties(i, protocol_version == 5)?; Ok(( i, MQTTSubackData { message_id, qoss: qoss.to_vec(), properties, + properties_truncated, }, )) } @@ -372,7 +392,7 @@ fn parse_unsubscribe( ) -> impl Fn(&[u8]) -> IResult<&[u8], MQTTUnsubscribeData> { move |i: &[u8]| { let (i, message_id) = be_u16.parse(i)?; - let (i, properties) = parse_properties(i, protocol_version == 5)?; + let (i, (properties, properties_truncated)) = parse_properties(i, protocol_version == 5)?; let (i, topics) = many0(complete(parse_mqtt_string)).parse(i)?; Ok(( i, @@ -380,6 +400,7 @@ fn parse_unsubscribe( message_id, topics, properties, + properties_truncated, }, )) } @@ -389,7 +410,7 @@ fn parse_unsubscribe( fn parse_unsuback(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQTTUnsubackData> { move |i: &[u8]| { let (i, message_id) = be_u16.parse(i)?; - let (i, properties) = parse_properties(i, protocol_version == 5)?; + let (i, (properties, properties_truncated)) = parse_properties(i, protocol_version == 5)?; let (i, reason_codes) = many0(complete(be_u8)).parse(i)?; Ok(( i, @@ -397,6 +418,7 @@ fn parse_unsuback(protocol_version: u8) -> impl Fn(&[u8]) -> IResult<&[u8], MQTT message_id, properties, reason_codes: Some(reason_codes), + properties_truncated, }, )) } @@ -413,6 +435,7 @@ fn parse_disconnect( MQTTDisconnectData { reason_code: None, properties: None, + properties_truncated: false, }, )); } @@ -425,6 +448,7 @@ fn parse_disconnect( MQTTDisconnectData { reason_code: Some(0), properties: None, + properties_truncated: false, }, )); } @@ -439,16 +463,18 @@ fn parse_disconnect( MQTTDisconnectData { reason_code: Some(0), properties: None, + properties_truncated: false, }, )); } match parse_properties(rem, true) { - Ok((rem, properties)) => { + Ok((rem, (properties, properties_truncated))) => { return Ok(( rem, MQTTDisconnectData { reason_code: Some(reason_code), properties, + properties_truncated, }, )); } @@ -463,12 +489,13 @@ fn parse_disconnect( #[inline] fn parse_auth(i: &[u8]) -> IResult<&[u8], MQTTAuthData> { let (i, reason_code) = be_u8(i)?; - let (i, properties) = parse_properties(i, true)?; + let (i, (properties, properties_truncated)) = parse_properties(i, true)?; Ok(( i, MQTTAuthData { reason_code, properties, + properties_truncated, }, )) } @@ -773,7 +800,7 @@ mod tests { let result = parse_properties(&buf, true); match result { - Ok((remainder, message)) => { + Ok((remainder, (message, _truncated))) => { let res = message.unwrap(); assert_eq!(res[0], MQTTProperty::RECEIVE_MAXIMUM(20)); assert_eq!(remainder.len(), 17); @@ -786,6 +813,7 @@ mod tests { } } } + #[test] fn test_parse_connect() { let buf = [