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

@@ -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;)

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();

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 {

rust/src/mqtt/mqtt_message.rs

@@ -103,6 +103,7 @@ pub struct MQTTConnectData {
     pub username: Option<String>,
     pub password: Option<Vec<u8>>,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
     pub will_properties: Option<Vec<MQTTProperty>>, // 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<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
@@ -119,6 +121,7 @@ pub struct MQTTPublishData {
     pub message_id: Option<u16>,
     pub message: Vec<u8>,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
@@ -126,6 +129,7 @@ pub struct MQTTMessageIdOnly {
     pub message_id: u16,
     pub reason_code: Option<u8>,               // MQTT 5.0
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
@@ -139,6 +143,7 @@ pub struct MQTTSubscribeData {
     pub message_id: u16,
     pub topics: Vec<MQTTSubscribeTopicData>,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
@@ -146,6 +151,7 @@ pub struct MQTTSubackData {
     pub message_id: u16,
     pub qoss: Vec<u8>,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
@@ -153,25 +159,29 @@ pub struct MQTTUnsubscribeData {
     pub message_id: u16,
     pub topics: Vec<String>,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
 pub struct MQTTUnsubackData {
     pub message_id: u16,
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
-    pub reason_codes: Option<Vec<u8>>,         // MQTT 5.0
+    pub properties_truncated: bool,
+    pub reason_codes: Option<Vec<u8>>, // MQTT 5.0
 }
 
 #[derive(Debug)]
 pub struct MQTTAuthData {
     pub reason_code: u8,                       // MQTT 5.0
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]
 pub struct MQTTDisconnectData {
     pub reason_code: Option<u8>,               // MQTT 5.0
     pub properties: Option<Vec<MQTTProperty>>, // MQTT 5.0
+    pub properties_truncated: bool,
 }
 
 #[derive(Debug)]

rust/src/mqtt/parser.rs

@@ -18,6 +18,7 @@
 // written by Sascha Steinbiss <sascha@steinbiss.name>
 
 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<Vec<MQTTProperty>>> {
+fn parse_properties(
+    input: &[u8], precond: bool,
+) -> IResult<&[u8], (Option<Vec<MQTTProperty>>, 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::<MQTTProperty>::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 = [