-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
detect-flowbits: adding details for flowbits v2 #9685
Conversation
NOTE: This PR may contain new authors:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the inline comments, this was a difficult review, but I hope that the feedback will be enough to help you move forward. If not, don't hesitate to reach out!
I have left feedback on the SV tests on the SV PR itself.
For the commit message, please use the imperative form add
, to follow our standards :)
jb_set_uint(js, "idx", cd->idx); | ||
jb_set_uint(js, "or_list_size", cd->or_list_size); | ||
jb_set_uint(js, "or_list", cd->or_list); | ||
if (cd->or_list_size > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the list size is grater than 0, this means we have a list, and might need to use a for
or another loop to access the flowbits names.
jb_set_uint(js, "or_list_size", cd->or_list_size); | ||
jb_set_uint(js, "or_list", cd->or_list); | ||
if (cd->or_list_size > 0) | ||
jb_set_string(js, "name", cd->or_list[cd->idx]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm terribly sorry, I misguided you on how to access the variable name. I misunderstood Shivani's reply, and this led to that.
Seeing the CI failures here I went and further investigated the code. For this work, I believe that VarNameStoreSetupLookup
would be our choice, and then we would pass cd->idx
and VAR_TYPE_FLOW_BIT
to it, to get the variable name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following the above, I spent some time to understand the code.
Aspects to consider:
- So, cd->idx starts at 1
- cd->or_list_size will only be greater than 0 for rules where we see the flowint or logic (cf https://github.com/OISF/suricata-verify/blob/master/tests/detect-flowbits/test.rules#L3). But the idx is global for flowbits. This means that we could have a list_size 2, and yet the idx for the variables accounted in the or_list be 2 and 3.
- Flowbits will only save a variable name if it sees a rule with
flowbits:set,
with the said variable name. Otherwise, that variable won't be registered, and therefore can't be retrieved withVarNameStoreSetupLookup
.
So, in the cases when cd->or_list_size > 0
we will have to that those into account, and then we can open an array to log the variable names. I think we can use the key ored_variables
here. See https://github.com/OISF/suricata/blob/master/src/detect-engine-analyzer.c#L797 for an example of how to open an array to log a list of values.
This info will be important to be able to properly proceed with this task, and also when improving the accompanying SV tests for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right to assume that if the cd->or_list_size > 0
and cd->cmd == DETECT_FLOWBITS_CMD_SET
then a for loop storing the variable names in an array should begin. I'm not sure about the second condition, should I keep it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's not how this works. cd->or_list_size
will only be greater than zero for rules like the ones you see in lines 3 and 4 here https://github.com/OISF/suricata-verify/blob/master/tests/detect-flowbits/test.rules#L3-L4
So you should not see size > 0
when DETECT_FLOWBITS_CMD_SET
. The SET
command will have only one variable name associated with it. Does this answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, so should I create an ored_variables
array and store names in it using a for loop if cd->or_list_size > 0
? But I'm not sure on how to access the name if cd->or_list_size == 0
. Should it be stored in a separate name
variable?:
const char *varname = VarNameStoreSetupLookup(cd->idx, VAR_TYPE_FLOW_BIT);
jb_set_string(js, "name", varname);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be simpler.
-
No need to create an array, as you could call
VarNameStoreSetupLookup
from thejb_set_string
orjb_append_string
functions directly. -
When
or_list_size == 0
, then you know the rule only has one flowbits variable, and therefore you can passcd->idx
directly toVarNameStoreSetupLookup
as id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, and these would both go in the name
variable or should I keep them separate in ored_variables
and name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you'll have either one case or the other, the way I see this is as follows:
- if
or_list_size == 0
, we log outname
- if
or_list_size > 0
, we log an array ofored_variables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood. Thank you so much!
jb_set_uint(js, "idx", cd->idx); | ||
jb_set_uint(js, "or_list_size", cd->or_list_size); | ||
jb_set_uint(js, "or_list", cd->or_list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that or_list_size
can have some value for a rule writer, as feedback to indicate that the or flowbit rule that they created works, so we could keep that and maybe change the json key to variables
.
Following the same reasoning, idx
and or_list
can be left out from this output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your detailed response. I'll look into this further and make the suggested changes.
work continued on PR: #9688 |
Task #6309
Link to redmine ticket:
Previous PR: #9683
Describe changes:
SV BRANCH = OISF/suricata-verify#1438