Conversation
This solution makes use of the petgraph crate to create a directed
stable graph of signatures. The algorithm is as follows:
- A stable Directed Graph is created
- A Flowbit Analyzer array is created which stores the sids per flowbit
per command
- for each entry in the flowbit analyzer "READ" array of type isset
- set array is walked => a directed edge* from the set sid to isset sid
with the weight of SET command is added.
- for each entry in the flowbit analyzer "READ" array of type isnotset
- unset array is walked => a directed edge from the set sid to isnotset sid
with the weight of SET command is added.
- If there was no error (cycle), this means that the current graph is a
DAG (Directed Acyclic Graph)
- Perform a topological sort of the DAG
1. If a cycle is formed by varying edge weights, it's an invalid cycle.
This would be formed by usage of different "WRITE" commands in
different signatures e.g.
sid: 1 => set, A; isset, B; # a directed edge of weight 0 is created
from 1 to 2 for flowbit A.
sid: 2 => isset, A; unset, B; # a directed edge of weight 2 is
created from 2 to 1 for flowbit B.
2. If the cycle is formed by the same edge weights, it is considered
unsatisfiable at runtime.
Bug 7771
Bug 7638
Bug 1399
before adding them to the Detection Context's signature list. The de_ctx->sig_list serves as a sorted signature list that is later passed on to the grouping fns. If no property of higher value changes the order of the signatures, the order coming from de_ctx->sig_list is final. Add the appropriate calls to resolve flowbit dependencies before adding them to the sig_list. This is especially important for flowbits with complex ordering involved. Bug 7771 Bug 7638 Bug 1399
The final list of signatures must be sorted with all the sorting fns available. However, flowbits SET_READ have their order derived from special handling between the dependent flowbits. Using the usual sorting criteria will mess up the order that was created. Bug 7771 Bug 7638 Bug 1399
Add an output file called "flowbits_dependency_graph.json" that outputs
the graph in the following format:
12 : { # sid of the node
in: [{ # all incoming edges
id: "1234" # internal ID of the node,
weight: 2,
sid: 13
}],
out: [{ # all outgoing edges
id: "124" # internal ID of the node,
weight: 2,
sid: 10
}]
}
in two conditions:
1. Right before any unrecoverable error.
2. At the end of the successful graph creation.
In order to deal with the complex flowbits ordering, Suricata internally makes a graph of the signature dependencies created by the flowbits. In this graph, some dependencies may lead to cycles that are not always valid. Suricata tries to resolve these cycles and come up with a final order of signatures that makes sense. Add a configurable limit detect.flowbits.max-cycle-resolution that can be used to asses when to give up. Bug 7638
Merge sort by default is a stable sorting algorithm which means that the relative order of all elements must be preserved in the final list. However, the implementation in SCSigOrder, instead of breaking each of the arrays into halves and going about the usual divide and conquer, ends up doing two things that make it unstable: 1. each array has a reversed order 2. the two halves at any point are created as by picking one item from the main array and putting it in the first half followed by picking second item from the main array and putting it in the second half. Fix this by first finding the midpoint of the main array in the current recursive call and breaking the array there. The midpoint is found using the Tortoise and Hare algorithm. The heads of each of the divided arrays point to the beginning. This guarantees the relative order among the elements to be stable. This becomes a bug when the final order is not to be messed with as done in the solution for Bug 7638 Bug 7771 Bug 1399
|
AI-generated review posted automatically by Suricata ai-review. Verdict: request changes
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #16176 +/- ##
==========================================
- Coverage 83.14% 83.12% -0.02%
==========================================
Files 1004 1005 +1
Lines 277579 277989 +410
==========================================
+ Hits 230789 231084 +295
- Misses 46790 46905 +115
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
True. The only thing taking precedence over flowbits order is action order. I also plan to add "priority" in that precedence list.
Yes and there's no way to know which of the OR'd flowbit should or should not be counted as a mandatory dependency edge. I am going to add a note about this. Up for discussion.
yeah. Can't fatal error on rule reload. In case someone adds a new rule that causes an unresolvable dependency, the only route is generate an error about it and load it anyway and hope the user will take care of it.
|
|
WARNING:
Pipeline = 33747 |
3 participants
Previous PR: #16172
Changes since v19:
Link to tickets:
SV_BRANCH=OISF/suricata-verify#3270
There are known issues still but this is the latest rev.