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

tests: add rule type check for flowbits v7 #1529

Closed

Conversation

hadiqaalamdar
Copy link
Contributor

@hadiqaalamdar hadiqaalamdar commented Dec 8, 2023

Task #6309

Redmine ticket: https://redmine.openinfosecfoundation.org/issues/6309
Previous PR: #1526

Suricata PR: OISF/suricata#10018

Output from console:

===> flowbits: Sub test #11: FAIL : expected 1 matches; got 0 for filter {'filename': 'rules.json', 'count': 1, 'match': {'id': 11, 'lists.packet.matches[0].name': 'flowbits', 'lists.packet.matches[0].flowbits.action': 'noalert'}}

Output from stdout file:

Notice: suricata: This is Suricata version 7.0.3-dev (2fe2d8250 2023-10-19) running in USER mode [LogVersion:suricata.c:1149]
Warning: threshold-config: Error opening file: "/usr/local/etc/suricata//threshold.config": Permission denied [SCThresholdConfInitContext:util-threshold-config.c:177]
Warning: detect-flowbits: flowbit 'fb3' is checked but not set. Checked in 4 and 1 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:602]
Warning: detect-flowbits: flowbit 'fb4' is checked but not set. Checked in 5 and 0 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:602]
Warning: detect-flowbits: flowbit 'fb5' is checked but not set. Checked in 13 and 0 other sigs [DetectFlowbitsAnalyze:detect-flowbits.c:602]

Copy link
Contributor

@jufajardini jufajardini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or operand is there now \o\

Thanks for changing the min-version :)

I've noticed that the output example must be updated:

  • it's indicating Suri 7, but this work is on Suri 8.
  • we know that the other checks pass, so would be a good thing (and it is a good practice, too), to include an output sample of the eve rules file, showing how things are looking (although that might be a better case for the Suri PR, now that I think about it)
  • What do you think of including at least one "ored" rule with three flowbits, just to showcase? ;)

Comment on lines +65 to +69
id: 7
lists.packet.matches[0].name: "flowbits"
lists.packet.matches[0].flowbits.cmd: "isnotset"
lists.packet.matches[0].flowbits.names[0]: "fb1"
lists.packet.matches[0].flowbits.names[1]: "fb2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that now the operand: or output is working 🎉 https://github.com/OISF/suricata/actions/runs/7145283554/job/19460682102?pr=10018#step:17:351
So we can add that to the checks for the "ored" rules :P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I missed this comment. I'm not sure I understand how you want me to change the "ored" rules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to change in the rules themselves, just to add a check for the operand, for the cases when we expect to see it. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you mean adding lists.packet.matches[0].flowbits.operand: "or" in the test.yaml file?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

Comment on lines +94 to +100
- filter:
filename: rules.json
count: 1
match:
id: 11
lists.packet.matches[0].name: "flowbits"
lists.packet.matches[0].flowbits.action: "noalert"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm almost sure that the check for the noalert rule will have to be rather different.
One thing we know is that the flags array does contain that info, so this is probably something that we could add, for now, if nothing else.

{
  "raw": "alert ip any any -> any any (msg:\"Noalert\"; flowbits:noalert; sid:11;)",
  "id": 11,
  "gid": 1,
  "rev": 0,
  "msg": "Noalert",
  "app_proto": "unknown",
  "requirements": [],
  "type": "ip_only",
  "flags": [
    "src_any",
    "dst_any",
    "sp_any",
    "dp_any",
    "noalert",
    "toserver",
    "toclient"
  ],
  "pkt_engines": [],
  "frame_engines": [],
  "lists": {}
}

@hadiqaalamdar
Copy link
Contributor Author

New PR: #1532

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.

2 participants