Detect multiprotocol keywords 7304 v2 #12229

catenacyber · 2024-12-05T10:39:39Z

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

Describe changes:

cleaner code for multi app-layer keywords such as ja4

Is there something to do with DCERPC/SMB stuff ? cc @inashivb

I think file keywords have their own logic and it is ok.

#12149 with better commit messages

such as ja4. Why ? We do not want to see hard-coded protocol constants such as ALPROTO_QUIC directly used in generic code in detect-parse.c How ? From the keyword point of view, this commit adds the function DetectSignatureSetMultiAppProto which is similar to DetectSignatureSetAppProto but takes multiple alprotos. It restricts the signature alprotos to a set of possible alprotos and errors out if the interstion gets empty. The data structure SignatureInitData gets extended with a fixed-length array, as the use case is a sparse number of protocols Ticket: 7304

instead of hardcoding list : removes usage of ALPROTO_QUIC and ALPROTO_TLS in generic SigValidate Ticket: 7304

codecov · 2024-12-05T10:59:51Z

Codecov Report

Attention: Patch coverage is 89.74359% with 12 lines in your changes missing coverage. Please review.

Project coverage is 83.17%. Comparing base (09ba69c) to head (7b24101).
Report is 32 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12229      +/-   ##
==========================================
- Coverage   83.19%   83.17%   -0.02%     
==========================================
  Files         912      912              
  Lines      257166   257267     +101     
==========================================
+ Hits       213938   213991      +53     
- Misses      43228    43276      +48

Flag	Coverage Δ
fuzzcorpus	`61.01% <80.26%> (+<0.01%)`	⬆️
livemode	`19.40% <1.31%> (-0.02%)`	⬇️
pcap	`44.01% <18.42%> (-0.39%)`	⬇️
suricata-verify	`62.74% <26.31%> (-0.05%)`	⬇️
unittests	`59.19% <67.52%> (+0.01%)`	⬆️

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

suricata-qa · 2024-12-06T19:40:03Z

Information: QA ran without warnings.

Pipeline 23713

inashivb · 2024-12-18T09:03:14Z

src/detect-parse.c

+        for (size_t i = 0; i < SIG_ALPROTO_MAX; i++) {
+            if (s->init_data->alprotos[i] == ALPROTO_UNKNOWN) {
+                break;
+            }
+            // first disable the ones that do not match
+            bool found = false;
+            while (*alprotos != ALPROTO_UNKNOWN) {
+                if (s->init_data->alprotos[i] == *alprotos) {
+                    found = true;
+                    break;
+                }
+                alprotos++;


This looks like we'll exhaust alprotos on checks against the first alproto entry of the signature if there was no match. Is this the intention?

Not sure I follow what you mean here...

There is a double loop with the current signature alprotos, and the new ones...

I mean for a say, s->init_data->alprotos = [proto1, proto2, proto3, ALPROTO_UNKNOWN]
and alprotos = [proto4, proto3, ALPROTO_UNKNOWN] coming to this part of the code,

First pass of the for loop looking for proto1 will bring alprotos pointer out of the array. We won't go over proto2 and proto3 in the s->init_data->alprotos, is this possible/intended?

Thanks for spotting this

inashivb · 2024-12-18T09:03:18Z

Is there something to do with DCERPC/SMB stuff ?

As far as I can tell, DCERPC/SMB will always fall in the default DetectSignatureSetAppProto case as there's just one to one protocol compatibility. So, nothing to do specially for that, I think.

inashivb · 2024-12-18T09:10:34Z

Another thought: why do we not just have a protocol compatibility matrix and look through it if a protocol is compatible with another one and what should be the final signature protocol in that case?

catenacyber · 2024-12-18T12:34:18Z

Is there something to do with DCERPC/SMB stuff ?

As far as I can tell, DCERPC/SMB will always fall in the default DetectSignatureSetAppProto case as there's just one to one protocol compatibility. So, nothing to do specially for that, I think.

DCERPC/SMB is not totally default as can be seen in AppProtoCommon :

// ok to have dcerpc keywords in smb sig

My question is like Could we get rid of this hardcoded behavior and have dcerpc keywords just call DetectSignatureSetMultiAppProto with both DCERPC and SMB ? (does it make sense, etc...)

Another thought: why do we not just have a protocol compatibility matrix and look through it if a protocol is compatible with another one and what should be the final signature protocol in that case?

Because of ja3/ja4 which are both quic and tls... (files keywords also, etc...)

inashivb · 2024-12-19T11:19:15Z

Is there something to do with DCERPC/SMB stuff ?

As far as I can tell, DCERPC/SMB will always fall in the default DetectSignatureSetAppProto case as there's just one to one protocol compatibility. So, nothing to do specially for that, I think.

DCERPC/SMB is not totally default as can be seen in AppProtoCommon :

I think I'm misunderstanding what you mean by "default" here. Could you please explain?
I'm purely talking from the perspective of smb rules allowing only dcerpc keywords hence there not being a need for "multiple" protocol compatibility checks.

// ok to have dcerpc keywords in smb sig

My question is like Could we get rid of this hardcoded behavior and have dcerpc keywords just call DetectSignatureSetMultiAppProto with both DCERPC and SMB ? (does it make sense, etc...)

yes, you could just call DetectSignatureSetMultiAppProto and it does make sense.
What I was saying was that even if you do call DetectSignatureSetMultiAppProto with DCERPC and SMB, it will fall into https://github.com/OISF/suricata/pull/12229/files#diff-38b98a1ee88485d56b7f5a35a5cea0bf27481910a7cf16ef4dc58b782ea9ccc6R1787 block of code ultimately calling
AppProtoCommon for this case since there is just one intersection possible.
Is this an incorrect assessment?

catenacyber · 2024-12-19T20:39:19Z

DCERPC/SMB has not a default behavior where default means not a specific case in the switch for AppProtoEquals and AppProtoCommon

Looking into the code to understand what is going on, I find some problems :

Why can we write the first rule and not the second one ?

alert tcp any any -> any any (flow:to_server; smb.version: 2; dcerpc.iface:367abb81-9844-35f1-ad32-98f038001003; dcerpc.opnum:15; sid:1;)
alert tcp any any -> any any (flow:to_server; dcerpc.iface:367abb81-9844-35f1-ad32-98f038001003; dcerpc.opnum:15; smb.version: 2; sid:1;)

Just having the smb keyword first or after dcerpc one...

How can we express that we want a rule to match some dceprc keywords, but only over SMB ? or only over raw non-SMB DCERPC ?

Looking at rules from dcerpc-smb-test-01 which are the same except their beginning : alert tcp alert smb and alert dcerpc...

So is it correct that DetectSignatureSetMultiAppProto could solve these problems ? (are they real problems)

Also wondering why I created ALPROTO_DOH2 when DCERPC over SMB, is kind of the same as DNS over HTTP2...
Maybe I should have rather implemented some AppLayerRegisterMayContain(ALPROTO_HTTP2, ALPROTO_DNS)

catenacyber · 2024-12-19T20:45:38Z

Next in #12310

catenacyber added 2 commits December 5, 2024 11:38

detect/ja: use multi-protocol support

7b24101

instead of hardcoding list : removes usage of ALPROTO_QUIC and ALPROTO_TLS in generic SigValidate Ticket: 7304

catenacyber requested a review from victorjulien as a code owner December 5, 2024 10:39

catenacyber mentioned this pull request Dec 5, 2024

Detect multiprotocol keywords 7304 v1 #12149

Closed

inashivb reviewed Dec 18, 2024

View reviewed changes

catenacyber mentioned this pull request Dec 19, 2024

Detect multiprotocol keywords 7304 v3 #12310

Open

catenacyber closed this Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect multiprotocol keywords 7304 v2 #12229

Detect multiprotocol keywords 7304 v2 #12229

catenacyber commented Dec 5, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading

suricata-qa commented Dec 6, 2024

inashivb Dec 18, 2024

catenacyber Dec 18, 2024

inashivb Dec 19, 2024

catenacyber Dec 19, 2024

catenacyber Dec 19, 2024

inashivb commented Dec 18, 2024

inashivb commented Dec 18, 2024

catenacyber commented Dec 18, 2024

inashivb commented Dec 19, 2024

catenacyber commented Dec 19, 2024

catenacyber commented Dec 19, 2024

Detect multiprotocol keywords 7304 v2 #12229

Detect multiprotocol keywords 7304 v2 #12229

Conversation

catenacyber commented Dec 5, 2024

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

suricata-qa commented Dec 6, 2024

inashivb Dec 18, 2024

Choose a reason for hiding this comment

catenacyber Dec 18, 2024

Choose a reason for hiding this comment

inashivb Dec 19, 2024

Choose a reason for hiding this comment

catenacyber Dec 19, 2024

Choose a reason for hiding this comment

catenacyber Dec 19, 2024

Choose a reason for hiding this comment

inashivb commented Dec 18, 2024

inashivb commented Dec 18, 2024

catenacyber commented Dec 18, 2024

inashivb commented Dec 19, 2024

catenacyber commented Dec 19, 2024

catenacyber commented Dec 19, 2024

codecov bot commented Dec 5, 2024 •

edited

Loading