Detect rule hook/v11 #12758
Draft
Detect rule hook/v11 #12758
+2,679
−908
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
victorjulien
requested review from
jufajardini,
catenacyber,
jasonish and
a team
as code owners
This was referenced Mar 12, 2025
Closed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12758 +/- ##
==========================================
+ Coverage 80.87% 80.89% +0.02%
==========================================
Files 936 937 +1
Lines 259748 260595 +847
==========================================
+ Hits 210061 210804 +743
- Misses 49687 49791 +104
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Information: QA ran without warnings. Pipeline 25079 |
|
Information: QA ran without warnings. Pipeline 25085 |
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
To support hook based buffer names.
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.
tls.version isn't hooked to a specific state by default. Allow it to register at the rule hook.
WIP packet: track hooks that this packet triggers hooks: set flow_start hook in packet detect: pack prefilter engine struct For future expansion of the fields. detect/prefilter: put pkt mask in struct In preparation for adding more pkt fields. WIP detect: implement pkt:flow_start hook SQUASH detect flow start hook
Firewall rules are like normal rule, with some key differences.
They are loaded separate, and first, from:
```yaml
firewall-rule-path: /etc/suricata/firewall/
firewall-rule-files:
- fw.rules
```
Differences with regular "threat detection" rules:
1. these rules are evaluated before threat detection rules
2. these rules are evaluated in the order as they appear in the rule file
3. currently only rules specifying an explicit hook at supported
a. as a consequence, no rules will be treated as (like) IP-only, PD-only or
DE-only
Mostly for QA purposes.
For testing mixing firewall and threat detection rules.
victorjulien
force-pushed
the
detect-rule-hook/v11
branch
from
fe99dfd to
626361f
Compare
|
Information: QA ran without warnings. Pipeline 25119 |
Add default drops for packet and app, add 'accept' action distinguishing between a firewall `accept` and a IPS `pass`. Firewall has 2 hooks: `packet_filter` and `app_filter`. Allow a `accept:hook`. This immediately stops the processing of the rules in this hook and continues processing in the next hook. Hook order: `packet_filter` -> `packet_td` -> `app_filter` -> `app_td`. Each of the hooks has some unique properties: `packet_filter`: - default policy is `drop` - rules are process in order - action scopes are explicit - `drop` or `accept` is immediate - `accept:hook` continues to `packet_td` `packet_td`: - default policy is `accept:hook` - rules are ordered by IDS/IPS ordering logic - action scopes are implicit - actions are queued - continues to `app_filter` or `alert/action finalize` `app_filter`: - default policy is `drop` - rules are process in order - action scopes are explicit - `drop` is immediate - `accept` is conditional on possible `drop` from `packet_td` - `accept:hook` continues to `app_td`, `accept:packet` or `accept:flow` continues to `alert/action finalize` `app_td`: - default policy is `accept:hook` - rules are ordered by IDS/IPS ordering logic - action scopes are implicit - actions are queued - continues to `alert/action finalize` Implementation: During sigorder, split into packet_filter, app_filter and general td. Spliting td into packet_td and app_td breaks expectations of existings IDS/IPS rules.
Allow fw rules to work when in pass:flow mode. When firewall mode is enabled, `pass:flow` will not skip the detection engine anymore, but instead we'll process the firewall rules and then apply the pass before inspecting threat detect rules.
Allow registering the progress as -1, which means it will be invoked each time the app prefilters are called.
Special notation for allowing an app-layer rule to match on each app update: `alert http1:request_update ...` `accept:hook tls:request_update ...`
victorjulien
force-pushed
the
detect-rule-hook/v11
branch
from
626361f to
2ec9062
Compare
|
Latest update adds:
accept:hook tls:client_hello_done $HOME_NET any -> $EXTERNAL_NET any (tls.sni; content:"www.google.com"; sid:104; alert;)
accept:hook tls:request_update $HOME_NET any -> $EXTERNAL_NET any (app-layer-state:<client_hello_done; alert; sid:105;)
accept:hook tls:request_update $HOME_NET any -> $EXTERNAL_NET any (app-layer-state:>client_hello_done; alert; sid:106;)
# accept all return states
accept:hook tls:response_update $EXTERNAL_NET any -> $HOME_NET any (alert; sid:205;)One observation: |
victorjulien
commented
Mar 14, 2025
| @@ -421,6 +433,11 @@ static inline void PacketAlertFinalizeProcessQueue( | |||
| SCLogDebug("sid:%u: is a pass rule, so break out of loop", s->id); | |||
| break; | |||
| } | |||
|
|
|||
| // TODO we can also drop if alert is suppressed, right? | |||
There was a problem hiding this comment.
need a test for this:
ips sig that drops, alert is suppressed. Overrides a app_filter accept
|
WARNING:
Pipeline 25127 |
|
Information: QA ran without warnings. Pipeline 25137 |
|
Information: QA ran without warnings. Pipeline 25139 |
|
Information: QA ran without warnings. Pipeline 25142 |
|
TODOs after a bit of live testing:
|
|
Another thing: when crafting a ruleset it's getting pretty annoying to assign sids, wonder if we should allow skipping them for fw rules. |
Closed
|
To be closed for #12820 ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Takes #12697 and overhauls some of the concepts. It removes the
appfwidea, in favor of the following:Firewall has 2 hooks:
packet_filterandapp_filter.Allow a
accept:hook. This immediately stops the processing of therules in this hook and continues processing in the next hook.
Hook order:
packet_filter->packet_td->app_filter->app_td.Hooks
Each of the hooks has some unique properties:
packet_filter:drop:packetdroporacceptis immediateaccept:hookcontinues topacket_tdpacket_td:accept:hookapp_filteroralert/action finalizeapp_filter:drop:packetdropis immediateacceptis conditional on possibledropfrompacket_tdaccept:hookcontinues toapp_td,accept:packetoraccept:flowcontinues to
alert/action finalizeapp_td:accept:hookalert/action finalizeThe interplay between firewall and threat detect is still a bit more complex than I'd like, but here we're limited by trying to remain compatible to existing IDS/IPS rulesets.
SV_BRANCH=OISF/suricata-verify#2327