detect-flowbits: adding details for flowbits v4 #9691

hadiqaalamdar · 2023-10-25T22:11:56Z

I have read the contributing guide lines at https://docs.suricata.io/en/latest/devguide/codebase/contributing/contribution-process.html
I have signed the Open Information Security Foundation contribution agreement at https://suricata.io/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:
Previous PR: #9688

Describe changes:

added the recommended changes from the last PR.
The SV tests id 1, 8 and 9 are failing

Output from console:

===> flowbits: Sub test #1: FAIL : expected 1 matches; got 0 for filter {'filename': 'rules.json', 'count': 1, 'match': {'id': 1, 'lists.packet.matches[0].name': 'flowbits', 'lists.packet.matches[0].flowbits.action': 'noalert'}}
===> flowbits: Sub test #8: FAIL : expected 1 matches; got 0 for filter {'filename': 'rules.json', 'count': 1, 'match': {'id': 8, 'lists.postmatch.matches[0].name': 'flowbits', 'lists.postmatch.matches[0].flowbits.cmd': 'isset', 'lists.postmatch.matches[0].flowbits.ored_flowbits': 2, 'lists.postmatch.matches[0].flowbits.ored_variables[0]': 'fb1', 'lists.postmatch.matches[0].flowbits.ored_variables[1]': 'fb2'}}
===> flowbits: Sub test #9: FAIL : expected 1 matches; got 0 for filter {'filename': 'rules.json', 'count': 1, 'match': {'id': 9, 'lists.packet.matches[0].name': 'flowbits', 'lists.packet.matches[0].flowbits.ored_flowbits': 1, 'lists.packet.matches[0].flowbits.cmd': 'isnotset', 'lists.packet.matches[0].flowbits.ored_variables[0]': 'fb1', 'lists.packet.matches[0].flowbits.ored_variables[1]': 'fb2'}}

SV BRANCH = OISF/suricata-verify#1441

Task OISF#6309

inashivb · 2023-10-26T03:06:17Z

src/detect-engine-analyzer.c

+                if (cd->or_list_size == 0) {
+                    jb_set_string(js, "name", VarNameStoreSetupLookup(cd->idx, VAR_TYPE_FLOW_BIT));
+                } else if (cd->or_list_size > 0) {
+                    jb_set_uint(js, "ored_flowbits", cd->or_list_size);


not sure about the use of "ored", IMO, "flowbits" should be fine.

inashivb · 2023-10-26T03:07:20Z

src/detect-engine-analyzer.c

+                jb_open_object(js, "flowbits");
+                switch (cd->cmd) {
+                    case DETECT_FLOWBITS_CMD_NOALERT:
+                        jb_set_string(js, "action", "noalert");


~~Note that this behavior is diff in 6.0.x, so you may have to check the tests~~
nvm we're just adding it so it doesn't matter

inashivb · 2023-10-26T03:22:31Z

src/detect-engine-analyzer.c

+                    jb_set_string(js, "name", VarNameStoreSetupLookup(cd->idx, VAR_TYPE_FLOW_BIT));
+                } else if (cd->or_list_size > 0) {
+                    jb_set_uint(js, "ored_flowbits", cd->or_list_size);
+                    jb_open_array(js, "ored_variables");


I think we should keep the output simple. Like

flowbits: name: [fb1]

or

flowbits: name: [fb1, fb2] # in case of an or operation

Also, one thing to note is that even in an or list, not all flowbits may have matched. From what I gather, we want to log the postmatch stuff so we should only log the names of the flowbit that matched? I'll check today and get back if there's a better way to retrieve it.

Also, need to look if or list may be specific to certain cmds.

I'm not sure I understand what you mean by changing the output. Currently, I have output being stored in name and the ored variables in ored_variables. Should I just have the name output? Also, or list is specific only to isset and isnotset cmds from what I understand.

There's no such thing as ored variables. Those are also flowbit names separated by | which signifies or operator. And, there are flowbit names which are individually present in a rule.
I still have to check the code a bit so please wait on it. I'll get to it very soon now.

inashivb

Hi @hadiqaalamdar !

After looking at the code, a few things to consider:

A rule can have multiple flowbits, what would it's layout look like in rules.json if we were to proceed with the current code?
Flowbit oring is supported only for isset and isnotset cmds. When you think about it, it makes sense why.
~~- Even in case of oring, there will be just one match because of how the or operator works.~~
~~- We currently do not store which flowbit out of the or'd flowbits actually was responsible for a match, we'll have to. (somewhere in DetectFlowbitsData)~~

In conclusion,

We do ~~not~~ need to store a list in case of or operator's usage.
We may need a list to sanitize the output in case multiple flowbits are used in the same rule. [This can be done after we have something working]
We need lots of tests for all the scenarios. Please refer to pre-existing s-v tests about flowbits.

Recommendation:
Questions to ask yourself before proceeding.

What are flowbits?
What all commands are supported by flowbits?
What do flowbits rules look like?
What should the output of engine-analysis be in my view?
Write a suricata-verify test with all the different rules you can come up with about flowbits.

These would help you get a clearer idea about what needs to be done. If needed, feel free to ask your mentors for brief overview of concepts and/or validate your understanding.

Please know that we understand this one is a complicated task so you can take your time. 😉

github-actions · 2023-10-26T12:44:19Z

NOTE: This PR may contain new authors:

Hadiqa Alamdar Bukhari <[email protected]>

codecov · 2023-10-26T13:21:26Z

Codecov Report

Merging #9691 (e373c3f) into master (2fe2d82) will decrease coverage by 0.01%.
The diff coverage is 45.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9691      +/-   ##
==========================================
- Coverage   82.39%   82.39%   -0.01%     
==========================================
  Files         968      968              
  Lines      274337   274370      +33     
==========================================
+ Hits       226047   226055       +8     
- Misses      48290    48315      +25

Flag	Coverage Δ
fuzzcorpus	`64.66% <0.00%> (-0.04%)`	⬇️
suricata-verify	`60.91% <45.45%> (-0.01%)`	⬇️
unittests	`62.86% <0.00%> (-0.01%)`	⬇️

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

hadiqaalamdar · 2023-10-26T13:40:20Z

@inashivb
I've looked through the flowbits code however, I'm not really sure on how to deal with the ored flowbits. This function here is what I found to be most relevant to the type of code that I need to add. My thinking was to create an array and use a for loop to store any flowbits, but since I'm not as well acquainted with the suricata code base, I'm unsure on how to proceed.

Edit: Also, should all flowbit names be stored in the same name array? Is there a pre-existing function that I can use to parse through flowbit names or should I use regex and store the names in an array if cd->or_list_size > 0

inashivb · 2023-10-27T05:56:45Z

@inashivb I've looked through the flowbits code however, I'm not really sure on how to deal with the ored flowbits. This function here is what I found to be most relevant to the type of code that I need to add. My thinking was to create an array and use a for loop to store any flowbits, but since I'm not as well acquainted with the suricata code base, I'm unsure on how to proceed.

Hi @hadiqaalamdar !
Seems like I had a misunderstanding about these engine analysis tasks in general that was cleared up after talking w Juliana. I have edited my last review (striked through the unneeded stuff) in case you'd want to check it later if needed.
Don't worry about the issues, we are here to assist you through them while learning things ourselves. Thank you for being patient with us. 😃

Edit: Also, should all flowbit names be stored in the same name array?

Yes, that was my suggestion.

The thing is for a rule like
Rule 1

flowbits:isset,fb1|fb2|fb3

fb1, fb2 and fb3 are the names of flowbits that are separated by or operand (|).
So, in this case, if fb1 is set, we return immediately and for Suricata, its a match!
But, in case fb1 isn't set, we check fb2, if fb2 is set, we return and for Suricata its a match. and so on.

Now, please note the following rule
Rule 2

flowbits:isset,fb1; flowbits:isset,fb2; flowbits:isset,fb3;

In this case, we check if fb1 is set, if yes, we move on to check if fb2 is set and so on.. Only when fb1, fb2 and fb3 are set would the rule considered to be a match. So, this is and logical operation.

Conclusion:

or operation is not anything complicated, it's just flowbit names ored together.
We must have tests for all such scenarios in s-v and see the corresponding output.
Since Rule 1 mentioned above also makes use of flowbit names only, I think we should put it under name as well.
So, ideally, flowbits name should be an array which can store one or more flowbits as shown in the example here: detect-flowbits: adding details for flowbits v4 #9691 (comment) depending on how many flowbits are in a rule for the same command.
We can optionally have an operand key (we can decide on the name later) where we can store if we want to whether it was an or operation but things can become complicated if we tried to do it for and operation.

Your next steps:

Make more flowbit tests with cmds and combinations as mentioned in the rules above.
Don't have separate arrays for ored stuff, put flowbit names under the flowbit.name array.

Does this help you, Hadiqa?

hadiqaalamdar · 2023-10-27T13:52:41Z

Thank you for your detailed response. I'll keep working on this and let you know if I have any further questions

jufajardini · 2023-10-27T15:38:42Z

The thing is for a rule like Rule 1
flowbits:isset,fb1|fb2|fb3
fb1, fb2 and fb3 are the names of flowbits that are separated by or operand (|). So, in this case, if fb1 is set, we return immediately and for Suricata, its a match! But, in case fb1 isn't set, we check fb2, if fb2 is set, we return and for Suricata its a match. and so on.

Now, please note the following rule Rule 2
flowbits:isset,fb1; flowbits:isset,fb2; flowbits:isset,fb3;
In this case, we check if fb1 is set, if yes, we move on to check if fb2 is set and so on.. Only when fb1, fb2 and fb3 are set would the rule considered to be a match. So, this is and logical operation.

I think that the only bit where things seem still a bit fuzzy is this:
Since the engine-analysis output isn't based on matches - as it only analysing rules, not traffic matches, my understanding is that we should focus on outputting a description of the rule. Therefore, for a rule like Rule 1 you've mentioned, we should log what is the flowbits action, and an array with all three flowbit variables, as that's what the rule contains. Does that make sense?

jufajardini · 2023-10-27T15:41:06Z

@inashivb I've looked through the flowbits code however, I'm not really sure on how to deal with the ored flowbits. This function here is what I found to be most relevant to the type of code that I need to add. My thinking was to create an array and use a for loop to store any flowbits, but since I'm not as well acquainted with the suricata code base, I'm unsure on how to proceed.

Edit: Also, should all flowbit names be stored in the same name array? Is there a pre-existing function that I can use to parse through flowbit names or should I use regex and store the names in an array if cd->or_list_size > 0

Now, to create the json array when we have more than one variable in the same rule, we can still use VarNameStoreSetupLookup, and we don't have to create an array as a data structure, I'd say that if we open an array json object and have a loop using VarNameStoreSetupLookup, we should be able to log all existing flowbit variables for a given rule.

hadiqaalamdar · 2023-10-27T16:12:27Z

The thing is for a rule like Rule 1
flowbits:isset,fb1|fb2|fb3
fb1, fb2 and fb3 are the names of flowbits that are separated by or operand (|). So, in this case, if fb1 is set, we return immediately and for Suricata, its a match! But, in case fb1 isn't set, we check fb2, if fb2 is set, we return and for Suricata its a match. and so on.

Now, please note the following rule Rule 2
flowbits:isset,fb1; flowbits:isset,fb2; flowbits:isset,fb3;
In this case, we check if fb1 is set, if yes, we move on to check if fb2 is set and so on.. Only when fb1, fb2 and fb3 are set would the rule considered to be a match. So, this is and logical operation.

@jufajardini from what I understand, I should create one name array json object that stores all the flowbit names. The array will store flowbits in 2 ways

if cd->or_list_size = 0 the flowbit would be at cd->idx and then stored in name.
if cd->or_list_size > 0 there are multiple flowbits and a for loop should be used alongside VarNameStoreSetupLookup to store all the flowbits in name.

My thinking is that this should cover both Rule 1 and Rule 2. Please let me know if I'm missing something.

jufajardini · 2023-10-27T16:28:22Z

The thing is for a rule like Rule 1
flowbits:isset,fb1|fb2|fb3
fb1, fb2 and fb3 are the names of flowbits that are separated by or operand (|). So, in this case, if fb1 is set, we return immediately and for Suricata, its a match! But, in case fb1 isn't set, we check fb2, if fb2 is set, we return and for Suricata its a match. and so on.
Now, please note the following rule Rule 2
flowbits:isset,fb1; flowbits:isset,fb2; flowbits:isset,fb3;
In this case, we check if fb1 is set, if yes, we move on to check if fb2 is set and so on.. Only when fb1, fb2 and fb3 are set would the rule considered to be a match. So, this is and logical operation.
@jufajardini from what I understand, I should create one name array json object that stores all the flowbit names. The array will store flowbits in 2 ways

if cd->or_list_size = 0 the flowbit would be at cd->idx and then stored in name.

if cd->or_list_size > 0 there are multiple flowbits and a for loop should be used alongside VarNameStoreSetupLookup to store all the flowbits in name.

My thinking is that this should cover both Rule 1 and Rule 2. Please let me know if I'm missing something.

From your explanation, this sounds about right!

jasonish · 2023-10-27T16:42:35Z

Just a comment on the format of the JSON. For the simple case we have:

"name": "flowbits",
          "flowbits": {
            "cmd": "isset",
            "name": "fb2"
          }

This looks OK, I'm not so sure about the OR case.. I'd rather see something like:

         "name": "flowbits",
         "flowbits": {
           "cmd": "isset",
           "names": [
             "fb1",
             "fb2"
           ],
           "operator": "or"
         }

Or the following is often used to express such conditions as well:

         "name": "flowbits",
         "flowbits": {
           "cmd": "isset",
           "or": ["fb1", "fb2"],
         }

hadiqaalamdar · 2023-10-27T16:54:01Z

I'll give this a try, thank you!

inashivb · 2023-10-28T02:42:51Z

This looks OK, I'm not so sure about the OR case.. I'd rather see something like:

         "name": "flowbits",
         "flowbits": {
           "cmd": "isset",
           "names": [
             "fb1",
             "fb2"
           ],
           "operator": "or"
         }

This was exactly my suggestion. Wasn't sure on the name of the operator or operand key though. Thanks for clarifying the entire format properly! :)

jufajardini · 2023-11-14T08:00:57Z

I'll give this a try, thank you!

Hi there, I know there was a lot of back and forth for the discussions here, so please let us know if you are having trouble following up on what we mentioned here, ok?

hadiqaalamdar · 2023-11-14T16:57:58Z

Hi there, I know there was a lot of back and forth for the discussions here, so please let us know if you are having trouble following up on what we mentioned here, ok?

Hi, sorry for the delay. I will continue to work on this and share an update this week.

hadiqaalamdar · 2023-12-05T18:50:24Z

New PR: #9971

detect-flowbits: adding details for flowbits

e373c3f

Task OISF#6309

hadiqaalamdar requested a review from victorjulien as a code owner October 25, 2023 22:11

This was referenced Oct 25, 2023

tests: adding rule type check for flowbits v4 OISF/suricata-verify#1441

Closed

detect-flowbits: adding details for flowbits v3 #9688

Closed

inashivb reviewed Oct 26, 2023

View reviewed changes

inashivb requested changes Oct 26, 2023

View reviewed changes

jufajardini added the outreachy Contributions made by Outreachy applicants label Oct 26, 2023

hadiqaalamdar mentioned this pull request Dec 5, 2023

detect-flowbits: adding details for flowbits v5 #9971

Closed

3 tasks

hadiqaalamdar closed this Dec 5, 2023

jufajardini mentioned this pull request Dec 6, 2023

tests: add rule type check for flowbits v5 OISF/suricata-verify#1512

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

detect-flowbits: adding details for flowbits v4 #9691

detect-flowbits: adding details for flowbits v4 #9691

hadiqaalamdar commented Oct 25, 2023 •

edited

Loading

inashivb Oct 26, 2023

inashivb Oct 26, 2023 •

edited

Loading

inashivb Oct 26, 2023 •

edited

Loading

hadiqaalamdar Oct 26, 2023 •

edited

Loading

inashivb Oct 26, 2023

inashivb left a comment •

edited

Loading

github-actions bot commented Oct 26, 2023

codecov bot commented Oct 26, 2023

hadiqaalamdar commented Oct 26, 2023 •

edited

Loading

inashivb commented Oct 27, 2023 •

edited

Loading

hadiqaalamdar commented Oct 27, 2023

jufajardini commented Oct 27, 2023

jufajardini commented Oct 27, 2023

hadiqaalamdar commented Oct 27, 2023 •

edited

Loading

jufajardini commented Oct 27, 2023

jasonish commented Oct 27, 2023

hadiqaalamdar commented Oct 27, 2023

inashivb commented Oct 28, 2023 •

edited

Loading

jufajardini commented Nov 14, 2023

hadiqaalamdar commented Nov 14, 2023

hadiqaalamdar commented Dec 5, 2023

detect-flowbits: adding details for flowbits v4 #9691

detect-flowbits: adding details for flowbits v4 #9691

Conversation

hadiqaalamdar commented Oct 25, 2023 • edited Loading

inashivb Oct 26, 2023

Choose a reason for hiding this comment

inashivb Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

inashivb Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

hadiqaalamdar Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

inashivb Oct 26, 2023

Choose a reason for hiding this comment

inashivb left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023

codecov bot commented Oct 26, 2023

Codecov Report

hadiqaalamdar commented Oct 26, 2023 • edited Loading

inashivb commented Oct 27, 2023 • edited Loading

hadiqaalamdar commented Oct 27, 2023

jufajardini commented Oct 27, 2023

jufajardini commented Oct 27, 2023

hadiqaalamdar commented Oct 27, 2023 • edited Loading

jufajardini commented Oct 27, 2023

jasonish commented Oct 27, 2023

hadiqaalamdar commented Oct 27, 2023

inashivb commented Oct 28, 2023 • edited Loading

jufajardini commented Nov 14, 2023

hadiqaalamdar commented Nov 14, 2023

hadiqaalamdar commented Dec 5, 2023

hadiqaalamdar commented Oct 25, 2023 •

edited

Loading

inashivb Oct 26, 2023 •

edited

Loading

inashivb Oct 26, 2023 •

edited

Loading

hadiqaalamdar Oct 26, 2023 •

edited

Loading

inashivb left a comment •

edited

Loading

hadiqaalamdar commented Oct 26, 2023 •

edited

Loading

inashivb commented Oct 27, 2023 •

edited

Loading

hadiqaalamdar commented Oct 27, 2023 •

edited

Loading

inashivb commented Oct 28, 2023 •

edited

Loading