Skip to content

Comments

Mqtt frames v6#1177

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

Mqtt frames v6#1177
hsadia538 wants to merge 2 commits intoOISF:masterfrom
hsadia538:mqtt-frames-v6

Conversation

@hsadia538
Copy link
Contributor

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

Previous PR: #1172

Describe changes:

  • Incorporate nit comments

Requires: OISF/suricata#8730

@hsadia538 hsadia538 mentioned this pull request Apr 17, 2023
alert mqtt any any -> any any (msg:"mqtt Frame 7"; frame:data; content:"|0a|"; sid:7;)

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

@inashivb inashivb Apr 18, 2023

Choose a reason for hiding this comment

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

Following the conversation from the last PR. ref #1172 (comment)

I meant that one byte is generally not a unique indicator of the data.

  1. Does |51| only occur once in the entire pcap?
    • Yes. Then sure, this is good.
    • No. Then, how do you know that the alert happened at the correct |51| byte? A unique sequence is usually a much better indicator. So, you could have say three or two bytes before this |51| to make sure it is the exact sequence that you intended to match with.

@inashivb inashivb added requires suricata pr Depends on a PR in Suricata outreachy Contributions made by Outreachy applicants labels Apr 21, 2023
@jufajardini jufajardini self-assigned this May 10, 2023
@catenacyber catenacyber removed the requires suricata pr Depends on a PR in Suricata label Jun 9, 2023
@catenacyber
Copy link
Collaborator

Suricata PR was merged

@inashivb inashivb added the question Further information is requested label Jun 9, 2023
@inashivb
Copy link
Member

inashivb commented Jun 9, 2023

Hi @hsadia538 !
Thank you for your work. Could you please follow up on the comments on the PR or let us know if there are any issues that you may be facing. We'll be happy to help :)
Edit: Even if you are unable to commit to completing this, please let us know so we'll pick it up and finish it. :)

@victorjulien victorjulien added the tests pass These new tests should pass label Jun 28, 2023
@catenacyber
Copy link
Collaborator

Replaced by #1365

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

Labels

outreachy Contributions made by Outreachy applicants question Further information is requested tests pass These new tests should pass

Development

Successfully merging this pull request may close these issues.

5 participants