LDAP protocol parser v5 #11360
LDAP protocol parser v5 #11360
Conversation
This implementation adds types and filters specified in the LDAP RFC to work with the ldap_parser. Although using the parser directly would be best, strange behavior has been observed during transaction logging. It appears that C pointers are being overwritten, leading to incorrect output when LDAP fields are logged.
glongo
requested review from
jasonish,
jufajardini,
victorjulien and
a team
as code owners
| #[derive(Debug, PartialEq, Eq, Clone, Copy)] | ||
| pub struct Operation(pub u32); | ||
|
|
||
| impl Display for Operation { |
There was a problem hiding this comment.
There is the EnumStringU32 derive, would it be relevant here ?
| } | ||
|
|
||
| impl Display for ProtocolOp { | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { |
There was a problem hiding this comment.
Why do we need this impl ?
There was a problem hiding this comment.
It's needed for this:
let protocol_op_str = req.protocol_op.to_string();
| ldap_parser::ldap::ProtocolOp::IntermediateResponse(msg) => { | ||
| Self::from_intermediate_response(msg) | ||
| } | ||
| _ => ProtocolOp::Unknown, |
There was a problem hiding this comment.
Which ones are missing ?
There was a problem hiding this comment.
Only abandon request operation, see rusticata/ldap-parser#6
There was a problem hiding this comment.
Maybe, I will make it explicit in a follow-up commit as a nit improvement
| } | ||
| } | ||
|
|
||
| pub fn is_response(&self) -> bool { |
There was a problem hiding this comment.
Is this not the opposite of is_request ?
There was a problem hiding this comment.
Maybe I will make it explicit in a follow-up commit as a nit improvement, by returning !self.is_request() instead of listing the types again
| if let Filter::Present(val) = &msg.filter { | ||
| js.open_object("filter")?; | ||
| js.set_string("type", "present")?; | ||
| js.set_string("value", &val.0.to_string())?; |
There was a problem hiding this comment.
Does a ldap filter have a maximum size ?
|
|
||
| // written by Giuseppe Longo <[email protected]> | ||
|
|
||
| use crate::applayer::{self, *}; |
There was a problem hiding this comment.
Would prefer to see a precise list instead of *
|
|
||
| #[derive(AppLayerEvent)] | ||
| enum LdapEvent { | ||
| TooManyTransactions, |
There was a problem hiding this comment.
Should there be one for a failed parsing ?
| let mut tx = LdapTransaction::new(); | ||
| self.tx_id += 1; | ||
| tx.tx_id = self.tx_id; | ||
| if self.transactions.len() > unsafe { LDAP_MAX_TX } { |
There was a problem hiding this comment.
Another option would be to fail the allocation of the new transaction...
| start = rem; | ||
|
|
||
| let response = LdapMessage::from(msg); | ||
| if let Some(tx) = self.find_request(response.message_id) { |
There was a problem hiding this comment.
Id we do not find the associated request, we should still mark the tx complete, right ?
There was a problem hiding this comment.
Yes, and I think an event should be set.
| get_state_data: SCLdapGetStateData, | ||
| apply_tx_config: None, | ||
| flags: APP_LAYER_PARSER_OPT_ACCEPT_GAPS, | ||
| truncate: None, |
There was a problem hiding this comment.
no longer needed on rebase
|
Replaced with #11513 |
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)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
(if applicable)
Link to ticket: https://redmine.openinfosecfoundation.org/issues/1199
Describe changes:
I still haven't made any changes regarding logging (#11163 (comment)).
The protocol itself seems to be very verbose, so I will leave this point open for discussion.
SV_BRANCH=OISF/suricata-verify#1860