detect: add ldap operation keywords - draft v2 #12343
Conversation
ldap.request.operation matches on Lightweight Directory Access Protocol request operations ldap.responses.operation matches on Lightweight Directory Access Protocol response operations Both are unsigned 8-bit integer Don't support prefiltering Ticket: OISF#7453
|
I'm not quite sure about these values I used for the constants. Should I approach it differently to be able to handle negative indexes, just like vlan.id? |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12343 +/- ##
==========================================
- Coverage 83.23% 83.23% -0.01%
==========================================
Files 912 913 +1
Lines 257647 257821 +174
==========================================
+ Hits 214450 214588 +138
- Misses 43197 43233 +36
Flags with carried forward coverage won't be shown. Click here to find out more. |
| 3 search_request | ||
| 4 search_result_entry | ||
| 5 search_result_done | ||
| 19 search_result_reference |
There was a problem hiding this comment.
Maybe we should keep the code ordered... @jufajardini ?
|
|
||
| .. container:: example-rule | ||
|
|
||
| alert tcp any any -> any any (msg:"Test LDAP bind request"; :example-rule-emphasis:`ldap.request.operation:0;` sid:1;) |
There was a problem hiding this comment.
You should also say somewhere that you can use the strings...
| ldap.responses.operation uses :ref:`unsigned 8-bit integer <rules-integer-keywords>`. | ||
|
|
||
| An LDAP request operation can receive multiple responses. By default, the ldap.responses.operation | ||
| keyword matches all indices, but it is possible to specify a particular index for matching |
There was a problem hiding this comment.
By default, should be any, not all
| [default] Match all indexes | ||
| all Match only if all indexes match | ||
| any Match all indexes | ||
| 0>= Match specific index |
There was a problem hiding this comment.
Need also negative indexes
|
|
||
| .. container:: example-rule | ||
|
|
||
| alert tcp any any -> any any (msg:"Test LDAP search response"; :example-rule-emphasis:`ldap.responses.operation:search_result_entry,all;` sid:1;) |
There was a problem hiding this comment.
Also need a ldap.responses.count keyword :-)
| ModDnRequest = 12, | ||
| ModDnResponse = 13, | ||
| CompareRequest = 14, | ||
| CompareResponse = 15, |
There was a problem hiding this comment.
AbandonRequest is also needed...
Can you at least put a comment about it ?
| #[derive(Debug, PartialEq)] | ||
| pub struct DetectLdapRespData { | ||
| pub response: DetectUintData<u8>, //ldap response code | ||
| pub index: i8, |
There was a problem hiding this comment.
May we should support indexes bigger than 255==u8::MAX
Let's try i32
There was a problem hiding this comment.
And if we can do pure rust here, you can have the struct not public, and having the index have a type that is an enum that is either Any, All, or Index(i32)
| use std::os::raw::{c_int, c_void}; | ||
| use std::str::FromStr; | ||
|
|
||
| pub const DETECT_LDAP_RESP_ANY: i8 = -1; |
There was a problem hiding this comment.
These values -1 and -2 should mean last and before last index...
| return std::ptr::null_mut(); | ||
| } | ||
|
|
||
| pub fn aux_ldap_parse_protocol_resp_op(s: &str) -> Option<DetectLdapRespData> { |
There was a problem hiding this comment.
This function does not need to be pub
| let tx = cast_pointer!(tx, LdapTransaction); | ||
| let ctx = cast_pointer!(ctx, DetectUintData<u8>); | ||
| if let Some(request) = &tx.request { | ||
| let option: u8 = request.protocol_op.to_u8(); |
There was a problem hiding this comment.
Do we need the : u8 here ?
| } | ||
| let response: &LdapMessage = &tx.responses[index]; | ||
| let option: u8 = response.protocol_op.to_u8(); | ||
| if rs_detect_u8_match(option, &ctx.response) == 1 { |
There was a problem hiding this comment.
return rs_detect_u8_match(option, &ctx.response) saves 2 lines ;-)
| as *const libc::c_char, | ||
| AppLayerTxMatch: Some(ldap_detect_responses_operation_match), | ||
| Setup: ldap_detect_responses_operation_setup, | ||
| Free: Some(ldap_detect_operation_free), |
There was a problem hiding this comment.
ldap_detect_operation_free frees a DetectUintData<u8>when it should free a DetectLdapRespData
I am afraid you need 2 different free functions
There was a problem hiding this comment.
Thanks Alice, nice work
Main things that remain to do
- negative indices
- AbandonRequest support
- ldap.reponses.count keyword
CI : 🟢
Code : cool
Commits segmentation : ok
Commit messages : nice
Git ID set : looks fine for me
CLA : you already contributed
Doc update : good
Redmine ticket : ok
Rustfmt : ok
Tests : ok, I left some remarks in SV
Dependencies added: none
Ticket: #7453
Contribution style:
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html
Our Contribution agreements:
https://suricata.io/about/contribution-agreement/ (note: this is only required once)
Changes (if applicable):
(including schema descriptions)
https://redmine.openinfosecfoundation.org/projects/suricata/issues
Link to ticket: https://redmine.openinfosecfoundation.org/issues/7453
Description:
ldap.request.operationandldap.responses.operationkeywords.detect.rs changes:
anyandall: line 32, line 33DetectLdapRespDatato storeldap.responses.operationdata: line 36ldap.responses.operationvalues: line 59indexvalue: line 169ldap-keywords.rst changes:
all,anyandindex: line 74SV_BRANCH=OISF/suricata-verify#2216
Previous PR= #12321