Mqtt frames v7#8513
Conversation
Adds PDU, Header and Data frame to the MQTT parser. Ticket: 5731
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #8513 +/- ##
==========================================
+ Coverage 81.84% 81.99% +0.15%
==========================================
Files 967 967
Lines 278343 278424 +81
==========================================
+ Hits 227799 228305 +506
+ Misses 50544 50119 -425
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
CI Failures for Debian 9 and CentOS 7 are unrelated to PR changes |
jufajardini
left a comment
jufajardini
left a comment
There was a problem hiding this comment.
I'm curious to what the SV tests for truncated mqtt messages will show in the frames. I've left some comments with my considerations as to why...
Apart from that, a couple of (possibly nit) inline comments.
| self.transactions.push_back(tx); | ||
| } | ||
|
|
||
| fn mqtt_frames(&mut self, flow: *const Flow, stream_slice: &StreamSlice, input: &MQTTMessage) { |
There was a problem hiding this comment.
Part of me isn't happy with this function name, because we register the mqtt pdu frame outside of it, and so the name is a bit misleading to me. Do you have some suggestions on how could we make more clear what's going on here?
There was a problem hiding this comment.
I will try to come up with something for this
There was a problem hiding this comment.
how about mqtt_hdr_data_frames ?
There was a problem hiding this comment.
To me, this gives the idea of header data frames, instead of header and data frames 🤔 not sure if this is premature optimization or not.
There was a problem hiding this comment.
how about mqtt_hdr_and_data_frames ?
| MQTTFrameType::Data as u8, | ||
| ); | ||
| SCLogDebug!("mqtt_data Frame {:?}", _mqtt_data); | ||
| } |
| // TODO: It might be useful to also add detection on property presence and | ||
| // content, e.g. mqtt.property: AUTHENTICATION_METHOD. | ||
| #[derive(Debug, PartialEq, PartialOrd)] | ||
| #[derive(Debug, PartialEq, Eq, PartialOrd)] |
There was a problem hiding this comment.
Was this needed by anything?
There was a problem hiding this comment.
this was added due to cargo clippy when I was having the issue. I will try to remove it and see if it works this time.
| let hdr_length = input.skipped_length - msg.header.remaining_length as usize; | ||
| let _mqtt_hdr = Frame::new( | ||
| flow, | ||
| stream_slice, | ||
| hdr, | ||
| hdr_length as i64, | ||
| MQTTFrameType::Header as u8, | ||
| ); |
There was a problem hiding this comment.
I'm curious to see what we will see in these frames.
Analizing the code some more, it seems to me that the Truncated message will have all header info, but no payload parsed by Suricata.
So, I'm thinking that maybe this is a special Frame type where folks only have access to info in the header, or, if we really wanted to give some insight into the (partial) truncated data, we could maybe use the remainder stream of the parsed message to register the data frame, and in that case the length would be mqtt max msg length.
There was a problem hiding this comment.
Now, one of the questions is what is interesting/important for us to expose to mqtt frames...
There was a problem hiding this comment.
so basically as far as I have understood when the length of parsed input message > max_mg_len then there are two conditions
- Where it is greater than the current buffer meaning data exceeds the boundary of parsed as well as the current buffer. here I am trying to parse header and whatever remaining length is in current buffer I call it data
- Where it exceeds the parsed message buffer but still lies within the boundary of the current overall buffer. here I use the skipped length to first extract the header and then remain portion of skipped length is data
There was a problem hiding this comment.
Now, one of the questions is what is interesting/important for us to expose to
mqttframes...
All of this brings me back to your last comment ^
There was a problem hiding this comment.
I have left a review on the tests PR that adds some input to this convo.
| let data = &hdr[hdr_length..current.len()]; | ||
| let _mqtt_data = Frame::new( | ||
| flow, | ||
| stream_slice, | ||
| data, | ||
| rem_length as i64, | ||
| MQTTFrameType::Data as u8, | ||
| ); |
There was a problem hiding this comment.
Following my previous comment, I think that if we really want to offer any insight into this truncated data, we would have to pass rem (https://github.com/OISF/suricata/blob/master/rust/src/mqtt/mqtt.rs#L457) to mqtt_frames_trunc (cf https://github.com/OISF/suricata/blob/master/rust/src/mqtt/parser.rs#L663)... 🤔
There was a problem hiding this comment.
This might work too. but I am passing the current buffer which also represents the full input buffer. So this is another way of exactly using the remainder stream as current = input = msg +rem. I have based my length calculations on this.
Do you still want me to look into this?
There was a problem hiding this comment.
From running the tests locally, your approach seems to be working. So I think we can move on ;)
There was a problem hiding this comment.
Based on your comment on the test frames PR , I will make a new PR for frames to add trunc_data frames
|
followed by #8540 |
2 participants
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5731
Previous PR: #8508
Describe new changes:
suricata-verify-pr: 1065