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).
Report is 5 commits behind head on master.

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

Following up on what was commented during today's call: I suggest that we move this table to a subsection within LDAP Keywords could be titled LDAP Request and Response operations, and then in each section we can refer to the table/ link to it.

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 ?

I didn't find anything about 17, 16 refers to the option AbandonRequest that is present in etc/schema.json as abandon_request but I didn't see this option in the code

https://metacpan.org/release/GBARR/perl-ldap-0.13/view/lib/Net/LDAP/BER.pod#REQ_EXTEND says application 17 is req extend 🤔

But 0x17 = 23

So, only 16 seems missing cf ldap_ProtocolOp_vals in Wireshark source code

So, we need only to add support for 16 = AbandonRequest

That means

Create a ticket about it

Create a SV test with a ldap abandon request pcap (with detection support)

upgrading ldap-parser crate to 0.4.1 in Cargo.toml.in (dedicated commit) cf https://github.com/rusticata/ldap-parser/blob/master/CHANGELOG.md#041 (recompile + mv Cargo.lock Cargo.lock.in)

Then fix uses of abandon cf `git grep -i abandon rust/src/ldap/

Add support for detection

And for AbandonRequest (and other cases not handled by match &response.protocol_op { in logger.rs ) we should log something, and we can use he functionality offered by EnumStringU8

It would be nice if the switch cases were complete...

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...

Is there any example that I could follow to implement ProtocolOp::Unknown(u) => u.opcode,?

No example, this requires code investigation, and maybe big changes...

Your version should create no unknown (ProtocolOp::Unknown will no longer exist)

And you should document that we are not able to match on some unknown ldap operation like 17.

Maybe we should have a ticket + SV test for it, but let's not bother with this first...

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

rust/src/ldap/detect.rs

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

AkakiAlice · 2024-12-30T17:40:10Z

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

Should I send ldap.response.operation and ldap.request.operation in the same PR (as they are on the same ticket)?

catenacyber · 2024-12-30T17:55:59Z

Yes one PR for ldap

catenacyber · 2025-01-07T08:44:41Z

Replaced by #12343 right ?

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

Closed

inashivb reviewed Dec 24, 2024

View reviewed changes

catenacyber reviewed Dec 24, 2024

View reviewed changes

rust/src/ldap/detect.rs Show resolved Hide resolved

catenacyber reviewed Dec 24, 2024

View reviewed changes

catenacyber requested changes Dec 24, 2024

View reviewed changes

This was referenced Jan 3, 2025

detect: add test for ldap.request.operation - draft v2 OISF/suricata-verify#2211

Closed

detect: add ldap operation keywords - draft v2 #12343

Draft

catenacyber closed this Jan 7, 2025

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 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

inashivb Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber commented Dec 24, 2024

catenacyber commented Dec 24, 2024

catenacyber Dec 24, 2024

jufajardini Jan 2, 2025

catenacyber Dec 24, 2024

AkakiAlice Dec 31, 2024 •

edited

Loading

catenacyber Jan 2, 2025

catenacyber Jan 2, 2025

catenacyber Jan 2, 2025

catenacyber Jan 2, 2025

catenacyber Jan 2, 2025 •

edited

Loading

catenacyber Dec 24, 2024

AkakiAlice Dec 31, 2024

catenacyber Jan 2, 2025

catenacyber Jan 2, 2025

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber Dec 24, 2024

catenacyber left a comment

AkakiAlice commented Dec 30, 2024

catenacyber commented Dec 30, 2024

catenacyber commented Jan 7, 2025


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

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

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

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

Conversation

AkakiAlice commented Dec 23, 2024 • edited Loading

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 • edited Loading

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

AkakiAlice Dec 31, 2024 • edited Loading

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 Jan 2, 2025 • edited Loading

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

AkakiAlice commented Dec 30, 2024

catenacyber commented Dec 30, 2024

catenacyber commented Jan 7, 2025

AkakiAlice commented Dec 23, 2024 •

edited

Loading

codecov bot commented Dec 23, 2024 •

edited

Loading

AkakiAlice Dec 31, 2024 •

edited

Loading

catenacyber Jan 2, 2025 •

edited

Loading