IMAP protocol parser, logger and sticky buffers v3#14995
Conversation
This introduces a parser for IMAP protocol. Ticket OISF#8276
This introduces a logger for IMAP protocol. Ticket OISF#8276
This implement the following sticky buffers for IMAP protocol: - imap.request - imap.response - imap.email.direction - imap.email.header - imap.email.header.name - imap.email.header.value - imap.email.body Ticket OISF#8276
glongo
requested review from
a team,
jasonish,
jufajardini and
victorjulien
as code owners
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14995 +/- ##
==========================================
- Coverage 82.60% 82.58% -0.03%
==========================================
Files 990 993 +3
Lines 271506 273898 +2392
==========================================
+ Hits 224290 226194 +1904
- Misses 47216 47704 +488
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline = 30106 |
|
|
||
| #[derive(AppLayerEvent)] | ||
| enum ImapEvent { | ||
| TooManyTransactions, |
There was a problem hiding this comment.
Do not forget to add a file imap-event.rules for this ;-)
catenacyber
left a comment
catenacyber
left a comment
There was a problem hiding this comment.
Thanks for the work, needs imap-events.rules at least
CI : ✅
Code : will look
Commits segmentation : I would squash more but I guess it is ok
Commit messages : Should also mention the email.* keywords, right ?
Git ID set : looks fine for me
CLA : you already contributed
Doc update : there is a section missing in doc/userguide/output/eve/eve-json-format.rst to describe the eve json output of an imap event. Also should there be lua support ? (code or ticket to track it for later)
Redmine ticket : ok, set to in review. Should we close https://redmine.openinfosecfoundation.org/issues/3244 as duplicate ?
Rustfmt : rustfmt rust/src/imap/*.rs clean :-)
Tests : left somme comments there
Dependencies added: none
|
Do we keywords file |
|
|
||
| if (SCDetectSignatureSetAppProto(s, ALPROTO_SMTP) < 0) | ||
| AppProto alprotos[] = { ALPROTO_SMTP, ALPROTO_IMAP, ALPROTO_UNKNOWN }; | ||
| if (DetectSignatureSetMultiAppProto(s, alprotos) < 0) |
| ) -> *mut DetectUintData<u8> { | ||
| let ft_name: &CStr = CStr::from_ptr(ustr); | ||
| if let Ok(s) = ft_name.to_str() { | ||
| if let Some(ctx) = detect_parse_uint_enum::<u8, ImapEmailDirection>(s) { |
There was a problem hiding this comment.
I would just use the strings, not numbers like 0/1/8
| pub complete: bool, | ||
|
|
||
| pub requests: Vec<ImapMessage>, | ||
| pub responses: Vec<ImapMessage>, |
There was a problem hiding this comment.
Are these stateful structruers bounded ?
Or does suricata OOM if stream.depth is 0 and a server does not stop sending responses to the same request ?
There was a problem hiding this comment.
Yes, look for IMAP_MAX_LINES.
This is one case: 2d15405#diff-e121f740af40d0ca373843803fa4b6231d4ab1ab9ab5026217fa41eef25ee82bR512
| &stream_slice, | ||
| start, | ||
| -1_i64, | ||
| ImapFrameType::Pdu as u8, |
There was a problem hiding this comment.
SV test missing for frames
| pub struct ImapParsedEmail { | ||
| pub direction: u8, | ||
| pub body: Vec<u8>, | ||
| pub headers: Vec<Vec<u8>>, |
There was a problem hiding this comment.
Are these fields bounded as well ?
|
Replaced with #15064 |
4 participants
PR mainly for discussion purposes.
Changes:
Main discussion point is:
Shoud we use
JsonGenericDirFlowLoggerorJsonGenericDirPacketLoggerto log IMAP events?Link to ticket: https://redmine.openinfosecfoundation.org/issues/8276
Previous PR: #14822
SV_BRANCH=OISF/suricata-verify#2908