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

analysis: report rule state altered by other rule - v3 #12515

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jufajardini
Copy link
Contributor

@jufajardini jufajardini commented Jan 31, 2025

Previous PR: #12311

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7456

Related to https://redmine.openinfosecfoundation.org/issues/7484

Describe changes:

  • only walk over isset sids array if there are signatures that must be flagged as stateful
  • rename fields
  • list all upstream sids and flowbits a rule state depends on
  • create JSON structure to allow for inclusion of other possible dependencies in the same object
  • if no dependency exists, nothing is logged (the field dependencies won't be present

Output sample:
A rule with a flowbit dependency will have the dependencies object. Otherwise, nothing is logged.

{
  "raw": "alert tcp-pkt any any -> any any (msg:\"Flowbit isset ored flowbits - pkt rule\"; flowbits:isset,fb5|fb6; sid:16;)",
  "id": 16,
  "gid": 1,
  "rev": 0,
  "msg": "Flowbit isset ored flowbits - pkt rule",
  "app_proto": "unknown",
  "requirements": [
    "flow",
    "real_pkt"
  ],
  "type": "pkt",
  "dependencies": {
    "flowbits": {
      "upstream": {
        "state_modifying_rules": {
          "sids": [
            2,
            15
          ],
          "names": [
            "fb6",
            "fb5"
          ]
        }
      }
    }
  }
}

Provide values to any of the below to override the defaults.

SV_BRANCH=OISF/suricata-verify#2270

For setting s->init_data for flowbit rules impacted by `set`, we can
first check whether this will be needed, and *then* walk over the
flowbits array.
Flowbits can make a rule such as a packet rule be treated as a stateful
rule, without actually changing the rule type.

Add a flag to allow reporting such cases via engine analysis.

Task OISF#7456
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 90.12346% with 8 lines in your changes missing coverage. Please review.

Project coverage is 80.68%. Comparing base (c00e137) to head (23a840e).

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12515      +/-   ##
==========================================
- Coverage   80.68%   80.68%   -0.01%     
==========================================
  Files         925      925              
  Lines      258916   258993      +77     
==========================================
+ Hits       208914   208976      +62     
- Misses      50002    50017      +15     
Flag Coverage Δ
fuzzcorpus 56.80% <41.97%> (-0.01%) ⬇️
livemode 19.39% <4.93%> (-0.01%) ⬇️
pcap 44.21% <41.97%> (+0.03%) ⬆️
suricata-verify 63.38% <90.12%> (+<0.01%) ⬆️
unittests 58.35% <6.17%> (-0.02%) ⬇️

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

@suricata-qa
Copy link

Information: QA ran without warnings.

Pipeline 24463

@jufajardini
Copy link
Contributor Author

I ran Suricata locally against an ET open ruleset (with and without this branch). Simple results:

  • without this change
    -- rules.json file size: 103,9 MB
    -- execution time: ~42.76 s
  • with this change
    -- rules.json file size: 103,9 MB
    -- execution time: ~43.21 s

@jasonish
Copy link
Member

jasonish commented Feb 4, 2025

      "upstream": {

What does upstream mean here? Are there any other possible values here? Downstream?

@jufajardini
Copy link
Contributor Author

      "upstream": {

What does upstream mean here? Are there any other possible values here? Downstream?

Yes, sorry, I should have linked a related ticket: https://redmine.openinfosecfoundation.org/issues/7484

This is the idea of what it could look like:

dependencies: {
  flowbits: {
    downstream: { (sids that depend on us)},
    upstream: { (we depend on),
      state_modifying_rules: {
         sids: [...] 
         names: [...]
      }
    }
  }
}

@jasonish
Copy link
Member

jasonish commented Feb 4, 2025

      "upstream": {

What does upstream mean here? Are there any other possible values here? Downstream?

Yes, sorry, I should have linked a related ticket: https://redmine.openinfosecfoundation.org/issues/7484

This is the idea of what it could look like:

dependencies: {
  flowbits: {
    downstream: { (sids that depend on us)},
    upstream: { (we depend on),
      state_modifying_rules: {
         sids: [...] 
         names: [...]
      }
    }
  }
}

Makes sense. Is the idea just for direct dependencies? Or transitive as well? By that I mean if a depends on b and b depends on c. Will c show up as a dependency for a?

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.

3 participants