imap: extend detection patterns - v2

[mmaatuq]

Ticket: #2886

Make sure these boxes are signed before submitting your Pull Request -- thank you.

• [x] I have read the contributing guide lines at https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
• [x] I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/ (note: this is only required once)
• [x] I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:2886

Describe changes:

• extend detection patterns for imap protocol as per rfc9051
• compared to this previous PR this one is simpler and way less expensive than the other.
• this is not comprehensive and might create more false positives, but i think this tradeoff is acceptable, and we can overcome these limitations when we add a complete parser.

SV_BRANCH=https://github.com/OISF/suricata-verify/pull/1620

[github-actions[bot]]

NOTE: This PR may contain new authors.

[mmaatuq]

Hi @jufajardini, I can see that a label was removed at this PR, is that what causes workflow failure or because of wrong format for SV_BRANCH in the description.

[inashivb]

because of wrong format for SV_BRANCH in the description.

Yes. You don't need to write markdown for the link to render in the format OISF/suricat..., that is done automagically by GitHub. You just have to paste the link in SV_BRANCH. :)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 70.27027% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 82.31%. Comparing base (244a35d) to head (38fb01b). Report is 107 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #10292       +/-   ##
===========================================
+ Coverage   73.31%   82.31%    +8.99%     
===========================================
  Files         895      979       +84     
  Lines      148215   272056   +123841     
===========================================
+ Hits       108666   223932   +115266     
- Misses      39549    48124     +8575     

Flag	Coverage Δ
fuzzcorpus	63.49% <70.27%> (+0.01%)	:arrow_up:
suricata-verify	61.50% <70.27%> (-0.03%)	:arrow_down:
unittests	62.84% <70.27%> (?)

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

[jufajardini]

Hi @jufajardini, I can see that a label was removed at this PR, is that what causes workflow failure or because of wrong format for SV_BRANCH in the description.

Hi there, I removed the link because as all checks were passing in the SV PR, it seemed to me that that PR could be merged even if its Suricata counter part hadn't. If the SV tests should check for changes that are introduced by your PR, then, indeed, I must add the label back - and we should check what is missing in the SV tests.

[inashivb]

Hi there, I removed the link because as all checks were passing in the SV PR, it seemed to me that that PR could be merged even if its Suricata counter part hadn't. If the SV tests should check for changes that are introduced by your PR, then, indeed, I must add the label back - and we should check what is missing in the SV tests.

The s-v test indeed depends on this. 😉 It passes bc it is skipped at the moment so merging the test w/o this PR would mean it would always be skipped and would be futile.

[jufajardini]

Hi there, I removed the link because as all checks were passing in the SV PR, it seemed to me that that PR could be merged even if its Suricata counter part hadn't. If the SV tests should check for changes that are introduced by your PR, then, indeed, I must add the label back - and we should check what is missing in the SV tests.

The s-v test indeed depends on this. 😉 It passes bc it is skipped at the moment so merging the test w/o this PR would mean it would always be skipped and would be futile.

I definitely missed that. Sorry for the noise!

[inashivb]

I definitely missed that. Sorry for the noise!

Don't worry. It was fairly hidden. Wondering if a new test is skipped that should be somehow flagged in the CI.

[mmaatuq]

Hi Team, Is there anything required from my side for this PR.

[jufajardini]

I definitely missed that. Sorry for the noise!

Don't worry. It was fairly hidden. Wondering if a new test is skipped that should be somehow flagged in the CI.

Might be a good idea to highlight that, somehow, so expectations are set.