Skip to content

Mqtt frames v8#8540

Closed
hsadia538 wants to merge 2 commits into
OISF:masterfrom
hsadia538:mqtt-frames-v8
Closed

Mqtt frames v8#8540
hsadia538 wants to merge 2 commits into
OISF:masterfrom
hsadia538:mqtt-frames-v8

Conversation

@hsadia538
Copy link
Copy Markdown
Contributor

Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/5731

Previous PR: #8508

Describe new changes:

  • Add trunc.data frames to cater for truncated packets

suricata-verify-pr: 1123

Adds PDU, Header and Data frame to the MQTT parser.
Ticket: 5731
@hsadia538 hsadia538 requested a review from jasonish as a code owner February 13, 2023 10:30
This was referenced Feb 13, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 13, 2023

Codecov Report

Merging #8540 (214ea8b) into master (d9e6301) will increase coverage by 0.07%.
The diff coverage is 79.13%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8540      +/-   ##
==========================================
+ Coverage   81.84%   81.91%   +0.07%     
==========================================
  Files         967      967              
  Lines      278343   278480     +137     
==========================================
+ Hits       227799   228122     +323     
+ Misses      50544    50358     -186     
Flag Coverage Δ
fuzzcorpus 64.09% <78.26%> (+0.15%) ⬆️
suricata-verify 59.82% <79.13%> (+0.16%) ⬆️
unittests 63.33% <1.73%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@jufajardini jufajardini added the outreachy Contributions made by Outreachy applicants label Feb 13, 2023
Comment thread rust/src/mqtt/mqtt.rs
Pdu,
Header,
Data,
TruncData,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Following the discussion that happened privately, this data frame type does not seem to be necessary as we cannot annotate something that does not exist. Even if the data is truncated, the parsed parts can be tagged with Pdu, so, we can get rid of this. Make sure to add tests corresponding to truncated data. e.g.

aaaaaaaaaaaaaaabbbbbbbbbb 

where a..a == data exactly the length limit
b..b == extra data that should ideally be truncated by the parser.
So, you have three tests.

  1. Well within the boundary.
    check for say first 4B of data in Pdu, it should match.
  2. Boundary test. Should match.
    check for all aaaa exactly up to the length limit.
  3. Post boundary test. Should not match.
    check for aaaa...bbb i.e. good data + truncated data

This was referenced Apr 15, 2023
@hsadia538
Copy link
Copy Markdown
Contributor Author

followed by #8730

@hsadia538 hsadia538 closed this Apr 15, 2023
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

Development

Successfully merging this pull request may close these issues.

3 participants