Skip to content

Mqtt frames v5#1172

Closed
hsadia538 wants to merge 2 commits intoOISF:masterfrom
hsadia538:mqtt-frames-v5
Closed

Mqtt frames v5#1172
hsadia538 wants to merge 2 commits intoOISF:masterfrom
hsadia538:mqtt-frames-v5

Conversation

@hsadia538
Copy link
Contributor

@hsadia538 hsadia538 commented Apr 15, 2023

Ticket: https://redmine.openinfosecfoundation.org/issues/5731

Previous PR: #1123

Describe changes:

  • Remove tests for mqtt trunc.data frames for truncated messages
  • Add boundary tests for truncated data

Requires: OISF/suricata#8730

@hsadia538 hsadia538 mentioned this pull request Apr 15, 2023
Copy link
Member

@inashivb inashivb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice work, @hsadia538 ! :)
Some questions and nits inline. Thank you for your work!

alert mqtt any any -> any any (msg:"mqtt truncated Frame 7"; frame:data; content:"|0a|"; sid:7;)

# At boundary test for truncated data
alert mqtt any any -> any any (msg:"mqtt truncated Frame 8"; frame:data; content:"|51|"; sid:8;)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a few more bytes to ensure it is the truncated data that we want to be looking at. Unless 51 is a very unique character that only happens once at the boundary for MQTT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what do you mean by adding more bytes. For this purpose of this rule, I counted the bytes up till a point where there is a boundary. Since after last update to frames, we are no longer making frames for truncated data so after this specific byte we don't get a match

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we get a match even after this specific byte cf #1365

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. did think that might happen. Thanks for picking this up! :)

alert mqtt any any -> any any (msg:"mqtt truncated Frame 8"; frame:data; content:"|51|"; sid:8;)

# post-boundary test for truncated data
alert mqtt any any -> any any (msg:"mqtt truncated Frame 9"; frame:data; content:"|5b|"; sid:9;) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comments as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can add more bytes to the post boundary test

@@ -0,0 +1,17 @@
alert mqtt any any -> any any (msg:"mqtt truncated Frame 1"; frame:pdu; content:"|10 1c|"; startswith; sid:1;)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: msg indicates all these are truncated frames which from the matches does not seem to be true?

min-version: 7

args:
- -k none
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: README mentions the pcap already exists. Can we try to deduplicate it w the pcap key here?

@hsadia538 hsadia538 mentioned this pull request Apr 17, 2023
@hsadia538
Copy link
Contributor Author

followed by #1177

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants