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.

Sign up for GitHub

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

Jump to bottom

detect-flowbits: adding details for flowbits v2 #9685

Closed
wants to merge 1 commit into from
Closed

detect-flowbits: adding details for flowbits v2 #9685

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions src/detect-engine-analyzer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
#include "util-time.h"
#include "util-validate.h"
#include "util-conf.h"
#include "detect-flowbits.h"

static int rule_warnings_only = 0;

Expand Down Expand Up @@ -861,6 +862,38 @@ static void DumpMatches(RuleAnalyzer *ctx, JsonBuilder *js, const SigMatchData *
jb_close(js);
break;
}
case DETECT_FLOWBITS: {
const DetectFlowbitsData *cd = (const DetectFlowbitsData *)smd->ctx;

jb_open_object(js, "flowbits");
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);
Comment on lines +869 to +871
Copy link
Contributor

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.

Copy link
Contributor Author

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.

if (cd->or_list_size > 0)
Copy link
Contributor

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_string(js, "name", cd->or_list[cd->idx]);
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 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.

Copy link
Contributor

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 with VarNameStoreSetupLookup.

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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?

Copy link
Contributor Author

@hadiqaalamdar hadiqaalamdar Oct 25, 2023 •
edited
Loading

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);

Copy link
Contributor

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 the jb_set_string or jb_append_string functions directly.

  • When or_list_size == 0, then you know the rule only has one flowbits variable, and therefore you can pass cd->idx directly to VarNameStoreSetupLookup as id.

Copy link
Contributor Author

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?

Copy link
Contributor

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 out name
  • if or_list_size > 0, we log an array of ored_variables

Copy link
Contributor Author

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!

switch (cd->cmd) {
case DETECT_FLOWBITS_CMD_NOALERT:
jb_set_string(js, "cmd", "noalert");
break;
case DETECT_FLOWBITS_CMD_ISSET:
jb_set_string(js, "cmd", "isset");
break;
case DETECT_FLOWBITS_CMD_ISNOTSET:
jb_set_string(js, "cmd", "isnotset");
break;
case DETECT_FLOWBITS_CMD_SET:
jb_set_string(js, "cmd", "set");
break;
case DETECT_FLOWBITS_CMD_UNSET:
jb_set_string(js, "cmd", "unset");
break;
case DETECT_FLOWBITS_CMD_TOGGLE:
jb_set_string(js, "cmd", "toggle");
break;
}
jb_close(js);
break;
}
}
jb_close(js);

Expand Down
Loading