Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/70x/radix/v54 #7425

Closed
wants to merge 28 commits into from

Conversation

victorjulien
Copy link
Member

Detection address handling rewrite. PR for CI/QA purposes.

Includes #7423

Changes:
Fixes a mem leak.
Some cleanups

catenacyber and others added 28 commits May 17, 2022 10:41
Do not mask protocols on both directions with only first packet

For instance :
When the first packet is no valid DNS but on port 53 (a junk request)
second packet (error response from server) does not get checked for DNS
as first packet bit masked away DNS for both directions

Ticket: OISF#2757
As is already done for TCP

Ticket: OISF#2757
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 3.0.0 to 3.1.0.
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@6673cd0...3cea537)

---
updated-dependencies:
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.1.9 to 2.1.11.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@7502d6e...a3a6c12)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <[email protected]>
In the unlikely case of AlertQueueExpand failure, we were incrementing
the discarded alerts stats in AlertQueueAppend via the Packet member in the
DetectEngineThreadCtx, which may not be initialized yet.

Bug OISF#5353
When reloading rules, respect `--set default-rule-path=...` from the
command line if set.

Previously the rule reload would always take the default-rule-path from
the configuration file, even if overrided on the command line.

Issue: OISF#1911
35 -> 36
34 -> 35
33 -> 34
Implement a more compact set of trees specifically for IPv4
and IPv6 addresses. This allows for more compact data structures
and fewer memory allocations.

Based on the existing radix tree implementation.
Until now the rule address handling was based on a list of positive
ranges approach, where the address portions of rules would be parsed
and turned into these positive ranges. This was not very efficient
for larger ranges, and especially in complex environments where the
address variables are complex this wasn't efficient.

Additionally, the IP only engine had its own logic based on the radix
tree matching and essentially re-parsed the same address strings.

This is the first commit in a redesign that unifies handling of address
parsing and matching around radix trees.

Each rule now has 4 separate radix trees. 2 per direction for both
IPv4 and IPv6. The rule parser populates these and records their
state as positive or negative match. The tree is somewhat optimized
in that it tries to avoid redundancy.

Additionally, the `detect address map` logic has been renamed to
`ipcache` and moved into its own file.
IP-only radix trees are now set up with negation support, so that
rules that use negation in their addresses are also supported.

Engine no longer re-parses address ranges as it can now directly
use the regular IP handling.
Additionally minor code cleanups.
This generic implementation is no longer used. It was replaced by
more specific implementations for IPv4 and IPv6.
@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #7425 (922b1e1) into master (b6407c4) will decrease coverage by 0.09%.
The diff coverage is 88.68%.

@@            Coverage Diff             @@
##           master    #7425      +/-   ##
==========================================
- Coverage   75.94%   75.85%   -0.10%     
==========================================
  Files         656      657       +1     
  Lines      189916   184345    -5571     
==========================================
- Hits       144233   139834    -4399     
+ Misses      45683    44511    -1172     
Flag Coverage Δ
fuzzcorpus 60.52% <70.75%> (-0.13%) ⬇️
suricata-verify 52.07% <64.22%> (+0.17%) ⬆️
unittests 60.55% <86.78%> (-0.52%) ⬇️

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

@suricata-qa
Copy link

ERROR:

ERROR: QA failed on tlpw1_files_sha256.

ERROR: QA failed on tlpr1_alerts_cmp.

Pipeline 7514

@victorjulien
Copy link
Member Author

This work isn't usable yet as the new parsing code isn't yet compatible with the old. Will revisit this later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants