imap: extend detection patterns - v5 #10740
Conversation
Ticket: OISF#2886 Signed-off-by: mmaatuq <mahmoudmatook.mm@gmail.com>
|
NOTE: This PR may contain new authors. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10740 +/- ##
==========================================
+ Coverage 78.52% 82.73% +4.20%
==========================================
Files 926 927 +1
Lines 247464 247679 +215
==========================================
+ Hits 194331 204905 +10574
+ Misses 53133 42774 -10359
Flags with carried forward coverage won't be shown. Click here to find out more. |
| * | ||
| */ | ||
|
|
||
| #ifndef __APP_LAYER_IMAP_H__ |
There was a problem hiding this comment.
@jasonish is this the right guard style ?
There was a problem hiding this comment.
Not anymore, we've updated the style so this should now be SURICATA_APP_LAYER_IMAP_H.
|
|
||
| #include "app-layer.h" | ||
| #include "app-layer-detect-proto.h" | ||
| #include "rust-bindings.h" |
There was a problem hiding this comment.
Why do we need rust here ?
| * there is no official document that limits the length of the tag | ||
| * some practical implementations limit it to 20 characters | ||
| * but keeping depth equal to 31 fails unit tests such AppLayerTest10 | ||
| * so keeping dpeth 17 for now to pass unit tests, that might miss some detections |
| /** | ||
| * there is no official document that limits the length of the tag | ||
| * some practical implementations limit it to 20 characters | ||
| * but keeping depth equal to 31 fails unit tests such AppLayerTest10 |
There was a problem hiding this comment.
AppLayerTest10 fails because it expects protocol detection to be completed with only 17 bytes as input, and with this new pattern, we would need more bytes to finish protocol detection...
@victorjulien do you have an idea about this ? Is it good to have a needing 31 bytes pattern for protocol detection ?
There was a problem hiding this comment.
@catenacyber should I incorporate the notes mentioned here and submit a new PR or wait for Victor's response.
There was a problem hiding this comment.
My advice is to submit a new PR with the notes mentioned here.
For the length, I suggest to keep 17, and update the comment to add my elements.
There was a problem hiding this comment.
And my biggest remark is in the Suricata-veridy PR ;-)
There was a problem hiding this comment.
Thanks for the work :-)
- CI : 🟢
- Code : Looking good for me
- Commits segmentation : ok
- Commit messages : ok for me
- Git ID set : looks fine for me
- CLA : I do not have the access to check it
- Doc update : not needed
- Redmine ticket : ok
- Rustfmt : not needed
- Tests : 🟠 Left one remark there
- Dependencies added: none
Ticket: #2886
Make sure these boxes are signed before submitting your Pull Request -- thank you.
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Link to redmine ticket:2886
Describe changes:
SV_BRANCH=OISF/suricata-verify#1723