Detect rule hook/v7 #12422

victorjulien · 2025-01-17T21:03:41Z

Some initial support for rule hooks, where you can specify where the rule should be hooked into the engine.

Currently only:

    Generic:
            <app_proto>:request_done and <app_proto>:response_done
    
    Per protocol, it uses the registered progress (state) values. E.g.
    
            tls:client_hello_done
    
    A rule ruleset could be:
    
            pass tls:client_hello_done any any -> any any (tls.sni; content:"www.google.com"; sid:21; alert;)
            drop tls:client_hello_done any any -> any any (sid:22;)
    
    The pass rule is evaluated when the client hello is parsed, and if it
    doesn't match the drop rule will be evaluated.

SV_BRANCH=OISF/suricata-verify#2239

https://redmine.openinfosecfoundation.org/issues/7485

victorjulien · 2025-01-17T21:05:44Z

@njlavigne this may interest you. Some simple examples https://github.com/OISF/suricata-verify/pull/2239/files#diff-005b44165e67b794ef7ec85c2ed081eb9e1a2e8bcc7961f07a6da3a9678c1a2e

victorjulien · 2025-01-17T21:06:19Z

src/app-layer-ssl.c

@@ -43,6 +43,58 @@
 #include "util-enum.h"
 #include "util-validate.h"

+static SCEnumCharMap tls_state_client_table[] = {


each of these can be used in the rules

suricata-qa · 2025-01-18T17:30:03Z

Information: QA ran without warnings.

Pipeline 24264

victorjulien · 2025-01-20T14:43:20Z

Whoops forgot the unittests...

codecov · 2025-01-20T17:00:31Z

Codecov Report

Attention: Patch coverage is 86.90614% with 113 lines in your changes missing coverage. Please review.

Project coverage is 80.70%. Comparing base (cbda276) to head (02c82c2).

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #12422    +/-   ##
========================================
  Coverage   80.70%   80.70%            
========================================
  Files         925      926     +1     
  Lines      258914   259212   +298     
========================================
+ Hits       208949   209203   +254     
- Misses      49965    50009    +44

Flag	Coverage Δ
fuzzcorpus	`56.88% <65.28%> (+0.07%)`	⬆️
livemode	`19.54% <35.93%> (+0.13%)`	⬆️
pcap	`44.22% <43.48%> (+<0.01%)`	⬆️
suricata-verify	`63.45% <83.19%> (+0.06%)`	⬆️
unittests	`58.38% <57.12%> (+<0.01%)`	⬆️

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

suricata-qa · 2025-01-21T06:00:03Z

Information: QA ran without warnings.

Pipeline 24284

jasonish · 2025-01-21T21:41:07Z

src/util-lua-packet.c

+        LUA_ERROR("failed to get userdata");
+    }
+    s->p = p;
+    luaL_getmetatable(luastate, "packet::metatable");


I'm thinking we should prefix these names as wel... suricata:packet::metatable or whatever.

suricata-qa · 2025-01-22T13:20:03Z

Information: QA ran without warnings.

Pipeline 24318

suricata-qa · 2025-01-22T15:55:03Z

Information: QA ran without warnings.

Pipeline 24319

victorjulien · 2025-01-22T18:33:10Z

src/detect-lua.c

    }
+    SCLuaSbLoadLibs(t->luastate);


I need this for the built-in lib to work with security.lua.allow-restricted-functions=true. Does this look OK @jasonish? Or should we only call

/* Setup our custom require. */ lua_pushcfunction(L, SCLuaSbRequire); lua_setglobal(L, "require");

somehow?

I think we have to go through requiref.. Essentially a different module registration. This require doesn't know about our custom modules. But its required for output-scripts to. Just haven't gone down that path for enough yet.

But does it work? Can you load our module and restricted modules?

Tried only our own OISF/suricata-verify@671d95f

My attempt and consolidating registration for use in sandboxed and non-sandboxed environments, to keep the ability to load external modules in non-sandboxed environments.

https://github.com/OISF/suricata/pull/12452/files

victorjulien · 2025-01-22T18:35:02Z

src/util-lua-packet.c

+    { "payload", LuaPacketPayload },
+    { "pcap_cnt", LuaPacketPcapCnt },
+    { "timestring", LuaPacketTimestring },
+    { "timestamp", LuaPacketTimestamp },


I've simply converted the SCPacketTimestamp, SCPacketTimestring and SCPacketTuple here. Is this the way to go or should we leave things like to string conversions of timestamp and addresses to the script?

Would there be use for both types? Raw and then an as_string variant?

suricata-qa · 2025-01-23T05:00:04Z

Information: QA ran without warnings.

Pipeline 24325

njlavigne · 2025-01-24T20:43:08Z

This looks really useful to the point where I think we would consider recommending it to almost all of our users as the recommended way to write application layer rules. It seems like having rules triggered on a well-defined point in the connection state like this would avoid most cases where users need to drop down into packet-level details to debug some of the more challenging issues they face today which is a huge plus.

The tls:client_hello_done event would probably be the single most useful event for us today, and I like that this seems like an idea that would be extensible to other types of events too as other relevant cases are identified.

suricata-qa · 2025-01-27T18:15:03Z

ERROR:

ERROR: QA failed on build_asan.

Pipeline 24375

suricata-qa · 2025-01-27T21:05:03Z

Information: QA ran without warnings.

Pipeline 24377

suricata-qa · 2025-01-29T19:25:03Z

Information: QA ran without warnings.

Pipeline 24428

suricata-qa · 2025-01-31T07:20:03Z

ERROR:

ERROR: QA failed on build_fetch.

Pipeline 24468

Example: ``` local packet = require "suricata.packet" function init (args) local needs = {} return needs end function match (args) p = packet.get() payload = p:payload() ts = p:timestring() for line in payload:gmatch("([^\r\n]*)[\r\n]+") do if line == "GET /index.html HTTP/1.0" then ipver, srcip, dstip, proto, sp, dp = p:tuple() SCLogNotice(string.format("%s %s->%s %d->%d (pcap_cnt:%d) match! %s", ts, srcip, dstip, sp, dp, p:pcap_cnt(), line)); return 1 end end return 0 end ``` Methods: `get` creates the packet object. `payload` returns the packet payload as a buffer `packet` returns the whole packet (includes headers) `pcap_cnt` returns the `pcap_cnt` (pcap file mode only) `tuple` returns various fields: srcip, dstip, proto, sp, dp `timestamp` returns time as 2 numbers: seconds and microseconds `timestring` returns a timestamp as a string Ticket: OISF#7488.

Moving forward the packetlib is to be used. Ticket: OISF#7488.

Register internal libs for the case where loading external modules is allowed.

To support hook based buffer names.

e.g. server hello done has no data

Per direction track progress to be able to have more fine grained control over where the detection engines and logging hooks in.

Generic: <app_proto>:request_done and <app_proto>:response_done Per protocol, it uses the registered progress (state) values. E.g. tls:client_hello_done A rule ruleset could be: pass tls:client_hello_done any any -> any any (tls.sni; content:"www.google.com"; sid:21; alert;) drop tls:client_hello_done any any -> any any (sid:22;) The pass rule is evaluated when the client hello is parsed, and if it doesn't match the drop rule will be evaluated. Registers each generic lists as "<alproto>:<progress state>:generic" (e.g. "tls:client_hello_done:generic"). Ticket: OISF#7485.

For registration of app-layer inspection, no longer use the 'needs' table from the script, but instead use the rule hook setting. Ticket: OISF#4783.

Instead of having a per detection engine list of rule that couldn't be prefiltered, put those into special "prefilter" engines. For packet and frame rules this doesn't change much, it just removes some hard coded logic from the detect engine. For the packet non-prefilter rules in the "non-prefilter" special prefilter engine, add additional filtering for the packet variant. It can prefilter on alproto, dsize and dest port. The frame non-prefilter rules are added to a single engine, that per rule checks the alproto and the type. For app-layer, there is an engine per progress value, per app-layer protocol and per direction. This hooks app-layer non-prefilter rules into the app inspect logic at the correct "progress" hook. e.g. a rule like dns.query; bsize:1; Negated MPM rules will also fall into this category: dns.query; content:!"abc"; Are part of a special "generic list" app engine for dns, at the same progress hook as `dns.query`. This all results in a lot fewer checks: previous: -------------------------------------------------------------------------- Date: 1/29/2025 -- 10:22:25. Sorted by: number of checks. -------------------------------------------------------------------------- Num Rule Gid Rev Ticks % Checks Matches Max Ticks Avg Ticks Avg Match Avg No Match -------- ------------ -------- -------- ------------ ------ -------- -------- ----------- ----------- ----------- -------------- 1 20 1 0 181919672 11.85 588808 221 60454 308.96 2691.46 308.07 2 50 1 0 223455914 14.56 453104 418 61634 493.17 3902.59 490.02 3 60 1 0 185990683 12.12 453104 418 60950 410.48 1795.40 409.20 4 51 1 0 192436011 12.54 427028 6084 61223 450.64 2749.12 417.42 5 61 1 0 180401533 11.75 427028 6084 61093 422.46 2177.04 397.10 6 70 1 0 153899099 10.03 369836 0 61282 416.13 0.00 416.13 7 71 1 0 123389405 8.04 369836 12833 44921 333.63 2430.23 258.27 8 41 1 0 63889876 4.16 155824 12568 39138 410.01 1981.97 272.10 9 40 1 0 64149724 4.18 155818 210 39792 411.70 4349.57 406.38 10 10 1 0 70848850 4.62 65558 0 39544 1080.70 0.00 1080.70 11 11 1 0 94743878 6.17 65558 32214 60547 1445.19 2616.14 313.92 this commit: -------------------------------------------------------------------------- Date: 1/29/2025 -- 10:15:46. Sorted by: number of checks. -------------------------------------------------------------------------- Num Rule Gid Rev Ticks % Checks Matches Max Ticks Avg Ticks Avg Match Avg No Match -------- ------------ -------- -------- ------------ ------ -------- -------- ----------- ----------- ----------- -------------- 1 50 1 0 138776766 19.23 95920 418 167584 1446.80 3953.11 1435.83 2 60 1 0 97988084 13.58 95920 418 182817 1021.56 1953.63 1017.48 3 51 1 0 105318318 14.60 69838 6084 65649 1508.04 2873.38 1377.74 4 61 1 0 89571260 12.41 69838 6084 164632 1282.56 2208.41 1194.20 5 11 1 0 91132809 12.63 32779 32214 373569 2780.22 2785.58 2474.45 6 10 1 0 66095303 9.16 32779 0 56704 2016.39 0.00 2016.39 7 70 1 0 48107573 6.67 12928 0 42832 3721.19 0.00 3721.19 8 71 1 0 32308792 4.48 12928 12833 39565 2499.13 2510.05 1025.09 9 41 1 0 25546837 3.54 12886 12470 41479 1982.53 1980.84 2033.05 10 40 1 0 26069992 3.61 12886 210 38495 2023.13 4330.05 1984.91 11 20 1 0 639025 0.09 221 221 14750 2891.52 2891.52 0.00

suricata-qa · 2025-01-31T10:55:02Z

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.uptime	623	650	104.33%

Pipeline 24469

victorjulien commented Jan 17, 2025

View reviewed changes

victorjulien force-pushed the detect-rule-hook/v7 branch 2 times, most recently from 322afa3 to 6b41bab Compare January 20, 2025 13:59

victorjulien force-pushed the detect-rule-hook/v7 branch 2 times, most recently from c95a068 to 671986d Compare January 20, 2025 16:12

victorjulien force-pushed the detect-rule-hook/v7 branch from 671986d to 03f7f04 Compare January 21, 2025 21:00

jasonish reviewed Jan 21, 2025

View reviewed changes

victorjulien force-pushed the detect-rule-hook/v7 branch from 03f7f04 to 844ea95 Compare January 22, 2025 13:33

victorjulien force-pushed the detect-rule-hook/v7 branch from 844ea95 to 300605e Compare January 22, 2025 18:31

victorjulien commented Jan 22, 2025

View reviewed changes

victorjulien force-pushed the detect-rule-hook/v7 branch from 300605e to 043a1df Compare January 27, 2025 16:54

victorjulien force-pushed the detect-rule-hook/v7 branch 4 times, most recently from 070070d to 4373f10 Compare January 29, 2025 11:23

victorjulien force-pushed the detect-rule-hook/v7 branch from 50b8f55 to 94c81f8 Compare January 30, 2025 21:12

victorjulien added 19 commits January 31, 2025 09:21

detect/lua: remove unused tls flag

b122831

detect/lua: minor code cleanup

1e23255

lua: remove old lua payload/packet calls

f9ac988

Moving forward the packetlib is to be used. Ticket: OISF#7488.

detect/lua: register built-in libs also for open setup

9e7cd24

Register internal libs for the case where loading external modules is allowed.

detect: don't register duplicate app inspect engines

af738e2

detect/nfs: don't double register nfq_request generic list

d69d76b

app-layer: constify AppLayerGetProtoByName

9abaa3e

detect: allow longer buffer names

75abafe

To support hook based buffer names.

tls: fix handshake handling being too strict

3cee30d

e.g. server hello done has no data

tls: introduce per direction progress tracking

6eb8cc1

Per direction track progress to be able to have more fine grained control over where the detection engines and logging hooks in.

app-layer: API for mapping progress name vs id

4a7a0c9

tls: expose progress by name

9ca3129

http1: register progress state names

3992a47

detect/lua: use rule hook instead of init() 'needs' table

3a0fee3

For registration of app-layer inspection, no longer use the 'needs' table from the script, but instead use the rule hook setting. Ticket: OISF#4783.

detect: reuse hook based generic lists

d78f051

WIP start of pkt hook

0c8d131

victorjulien force-pushed the detect-rule-hook/v7 branch from 94c81f8 to 02c82c2 Compare January 31, 2025 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect rule hook/v7 #12422

Detect rule hook/v7 #12422

victorjulien commented Jan 17, 2025

victorjulien commented Jan 17, 2025

victorjulien Jan 17, 2025

suricata-qa commented Jan 18, 2025

victorjulien commented Jan 20, 2025

codecov bot commented Jan 20, 2025 •

edited

Loading

suricata-qa commented Jan 21, 2025

jasonish Jan 21, 2025

suricata-qa commented Jan 22, 2025

suricata-qa commented Jan 22, 2025

victorjulien Jan 22, 2025

jasonish Jan 22, 2025

jasonish Jan 22, 2025

victorjulien Jan 22, 2025

jasonish Jan 22, 2025

victorjulien Jan 22, 2025

jasonish Jan 22, 2025

suricata-qa commented Jan 23, 2025

njlavigne commented Jan 24, 2025

suricata-qa commented Jan 27, 2025

suricata-qa commented Jan 27, 2025

suricata-qa commented Jan 29, 2025

suricata-qa commented Jan 31, 2025

suricata-qa commented Jan 31, 2025

Detect rule hook/v7 #12422

Are you sure you want to change the base?

Detect rule hook/v7 #12422

Conversation

victorjulien commented Jan 17, 2025

victorjulien commented Jan 17, 2025

Choose a reason for hiding this comment

suricata-qa commented Jan 18, 2025

victorjulien commented Jan 20, 2025

codecov bot commented Jan 20, 2025 • edited Loading

Codecov Report

suricata-qa commented Jan 21, 2025

Choose a reason for hiding this comment

suricata-qa commented Jan 22, 2025

suricata-qa commented Jan 22, 2025

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

suricata-qa commented Jan 23, 2025

njlavigne commented Jan 24, 2025

suricata-qa commented Jan 27, 2025

suricata-qa commented Jan 27, 2025

suricata-qa commented Jan 29, 2025

suricata-qa commented Jan 31, 2025

suricata-qa commented Jan 31, 2025

codecov bot commented Jan 20, 2025 •

edited

Loading