Flowbit ordering cyclic/v4#13756
Closed
inashivb wants to merge 4 commits into
Closed
Conversation
This should make it possible to catch invalid combinations in the same signature early. This patch covers checking the following invalid cmd combinations: - set + isset - unset + isnotset - set + toggle - set + unset - isset + isnotset the same flowbit in the same signature which is basically an unnecessary operation. This helps bring down the difficulty of handling of actual complex flowbit chains. Bug 7772 Bug 7773 Bug 7774 Bug 7817 Bug 7818
This solution makes use of the petgraph crate to create a graph of flowbits and signatures. Following steps are involved. 1. Nodes are created for flowbits as well as signatures 2. A directed edge is added: from flowbit to signature iff the flowbit use TODO cmd in that sig from signature to flowbit iff the flowbit used TODO cmd in that sig 3. The graph is then normalized i.e. the flowbit nodes are removed and a clean directed edge is created connecting a dependency graph among the signatures. This is done because flowbits are inconsequential in the final required solution. 4. The graph is checked for any cycles. In a case like that, the ruleset must be rejected as it cannot be satisfied during runtime anyway. 5. Finally, a BFS (Breadth First Search) is performed on the graph giving the exact order in which signatures should be such that no dependee comes before a dependant. Bug 7771 Bug 7638
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 high 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
Member
Author
|
This is very close to completion from my end and passes the basic tests so I'd like to see if it holds up in the QA lab.. |
|
WARNING:
Pipeline = 27251 |
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: #13747
Changes since v3:
Known TODOs:
firewall-01-tcp-pkt-state-flowbits⭐For a given READ command (isset, isnotset) and a given WRITE command (set, unset, toggle)
current algorithm marks a combination of following signatures as cyclic -- which shall be rejected in the final rev of this work
However, there may exist such flowbit combinations that are not cyclic. From the test
firewall-01-tcp-pkt-state-flowbits,Link to tickets:
SV_BRANCH=OISF/suricata-verify#2627
Question: What shall be done with signatures which contain a cyclic dependency? Current PR's behavior is to restore the original signature ordering in such a case.