detect: add ldap.request.option keyword - v1 #12321

AkakiAlice · 2024-12-23T11:35:33Z

Ticket: #7453

Contribution style:

I have read the contributing guide lines at
https://docs.suricata.io/en/latest/devguide/contributing/contribution-process.html

Our Contribution agreements:

I have signed the Open Information Security Foundation contribution agreement at
https://suricata.io/about/contribution-agreement/ (note: this is only required once)

Changes (if applicable):

I have updated the User Guide (in doc/userguide/) to reflect the changes made
I have updated the JSON schema (in etc/schema.json) to reflect all logging changes
(including schema descriptions)
I have created a ticket at
https://redmine.openinfosecfoundation.org/projects/suricata/issues

Link to ticket: https://redmine.openinfosecfoundation.org/issues/7453

Description:

Add ldap.request.operation keyword

ldap.rs

Change static mut to pub(super) static mut so I can use ALPROTO_LDAP on file ldap/detect.rs

mod.rs

Include detect

types.rs

Implement function to convert enum to u8. I used the existing ldap pcaps in S-V and RFC 4511 to set the values

detect-engine-register.c

Include ScDetectLdapRegister()

detect.rs

Create new file to implement ldap keywords

ldap-keywords.rst

Create new file to document ldap keywords

index.rst

Include ldap-keywords

SV_BRANCH=OISF/suricata-verify#2206

ldap.request.operation matches on Lightweight Directory Access Protocol request operations It is an unsigned 8-bit integer Doesn't support prefiltering Ticket: OISF#7453

codecov · 2024-12-23T12:05:12Z

Codecov Report

Attention: Patch coverage is 77.02703% with 17 lines in your changes missing coverage. Please review.

Project coverage is 83.24%. Comparing base (6f937c7) to head (68fbc10).

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12321      +/-   ##
==========================================
- Coverage   83.26%   83.24%   -0.03%     
==========================================
  Files         912      913       +1     
  Lines      257643   257717      +74     
==========================================
+ Hits       214521   214528       +7     
- Misses      43122    43189      +67

Flag	Coverage Δ
fuzzcorpus	`61.19% <24.32%> (+0.05%)`	⬆️
livemode	`19.40% <24.32%> (+<0.01%)`	⬆️
pcap	`44.39% <24.32%> (-0.03%)`	⬇️
suricata-verify	`62.88% <77.02%> (+0.02%)`	⬆️
unittests	`59.18% <24.32%> (-0.01%)`	⬇️

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

inashivb · 2024-12-24T06:56:52Z

doc/userguide/rules/ldap-keywords.rst

+
+Syntax::
+
+ ldap.request.operation: operation;


Looking good from a brief overview! Would like other opinions on the name of the keyword. This looks good to me but perhaps too long?

For info, this name comes from json schema

catenacyber · 2024-12-24T09:47:52Z

rustfmt rust/src/ldap/mod.rs does a little change ;-)

catenacyber · 2024-12-24T09:48:28Z

Please put the full clickable link to redmine ticket https://redmine.openinfosecfoundation.org/issues/7453 ;-)

catenacyber · 2024-12-24T09:49:26Z

doc/userguide/rules/ldap-keywords.rst

+
+.. table:: **Operation values for ldap.request.operation keyword**
+
+    ====  ================================================


When you implement ldap.response.operation, the same table should be used for both

catenacyber · 2024-12-24T09:50:33Z

rust/src/ldap/types.rs

+            ProtocolOp::ModDnRequest(_) => 12,
+            ProtocolOp::ModDnResponse(_) => 13,
+            ProtocolOp::CompareRequest(_) => 14,
+            ProtocolOp::CompareResponse(_) => 15,


Do you know why is there a hole for 16, 17... ?

Maybe something that was implement in LDAP v1 and is obsolete in LDAP v3 but we should still recognize ?

catenacyber · 2024-12-24T09:51:37Z

rust/src/ldap/types.rs

+            ProtocolOp::ExtendedRequest(_) => 23,
+            ProtocolOp::ExtendedResponse(_) => 24,
+            ProtocolOp::IntermediateResponse(_) => 25,
+            ProtocolOp::Unknown => 255,


This is not ideal
There should be some documentation at least
Or even better, we should have ProtocolOp::Unknown(u) => u.opcode, but this may require big changes...

catenacyber · 2024-12-24T09:52:26Z

rust/src/ldap/types.rs

@@ -300,6 +300,34 @@ pub enum ProtocolOp {
    Unknown,
 }

+impl ProtocolOp {


@jasonish what do you think about this rust code that works ?

catenacyber · 2024-12-24T09:52:45Z

rust/src/ldap/detect.rs

+    G_LDAP_REQUEST_OPERATION_BUFFER_ID = DetectHelperBufferRegister(
+        b"ldap.request.operation\0".as_ptr() as *const libc::c_char,
+        ALPROTO_LDAP,
+        false,


Please comment meaning of these true/false values

catenacyber · 2024-12-24T09:53:35Z

rust/src/ldap/detect.rs

+    let ctx = cast_pointer!(ctx, DetectUintData<u8>);
+    let option: u8 = match &tx.request {
+        Some(request) => request.protocol_op.to_u8(),
+        None => 0,


We should return 0, instead of having option=0

Otherwise, we match on ldap.request.operation: 0 for responses

Better call rs_detect_u8_match(option, ctx) only if let Some(request) = &tx.request{ and return 0 by default at the end of function

catenacyber · 2024-12-24T09:55:36Z

rust/src/ldap/detect.rs

+    }
+    let ctx = rs_detect_u8_parse(raw) as *mut c_void;
+    if ctx.is_null() {
+        return -1;


You can implement a protocol_op from_str and call it there if rs_detect_u8_parse fails

catenacyber

Really good, you have a first code working :-)

Design seems good, a few bugs to fix, and you can expand on it with ldap.response.operation

detect: add ldap.request.option keyword

68fbc10

ldap.request.operation matches on Lightweight Directory Access Protocol request operations It is an unsigned 8-bit integer Doesn't support prefiltering Ticket: OISF#7453

AkakiAlice requested review from jasonish, jufajardini and victorjulien as code owners December 23, 2024 11:35

AkakiAlice mentioned this pull request Dec 23, 2024

test: add test for ldap.request.operation - v1 OISF/suricata-verify#2206

Open

inashivb reviewed Dec 24, 2024

View reviewed changes

catenacyber reviewed Dec 24, 2024

View reviewed changes

catenacyber requested changes Dec 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect: add ldap.request.option keyword - v1 #12321

detect: add ldap.request.option keyword - v1 #12321

AkakiAlice commented Dec 23, 2024

codecov bot commented Dec 23, 2024

inashivb Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber commented Dec 24, 2024

catenacyber commented Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber left a comment


		.. table:: Operation values for ldap.request.operation keyword

		==== ================================================

detect: add ldap.request.option keyword - v1 #12321

Are you sure you want to change the base?

detect: add ldap.request.option keyword - v1 #12321

Conversation

AkakiAlice commented Dec 23, 2024

Contribution style:

Our Contribution agreements:

Changes (if applicable):

Description:

ldap.rs

mod.rs

types.rs

detect-engine-register.c

detect.rs

ldap-keywords.rst

index.rst

codecov bot commented Dec 23, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Dec 24, 2024

catenacyber commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber left a comment

Choose a reason for hiding this comment