Flowbit ordering cyclic/v16#15738
Open
inashivb wants to merge 10 commits into
Open
Conversation
This brings down the complexity of dependency resolution where such invalid combinations may end up adding more constraint. Task 8656
Analyzer arrays store internal IDs of the signature for an easy lookup later from the DetectEngineCtx. However, the storage arrays were inappropriately named as sid arrays. Make it coherent. Bring Analyzer structs and fns to the header for later use in a wider context.
sid is much more useful for logging and debugging.
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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #15738 +/- ##
==========================================
- Coverage 82.96% 82.96% -0.01%
==========================================
Files 1003 1004 +1
Lines 275031 275424 +393
==========================================
+ Hits 228192 228492 +300
- Misses 46839 46932 +93
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
WARNING:
Pipeline = 32270 |
inashivb
requested review from
a team,
jasonish,
jufajardini and
victorjulien
as code owners
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Previous PR: #15480
Changes since v15:
prioritykeyword combination handledLink to tickets:
SV_BRANCH=OISF/suricata-verify#3196