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: add details for flowbits v7 #10018

Closed
wants to merge 1 commit into from
Closed

detect-flowbits: add details for flowbits v7 #10018

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
47 changes: 47 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,8 @@
#include "util-time.h"
#include "util-validate.h"
#include "util-conf.h"
#include "detect-flowbits.h"
#include "util-var-name.h"

static int rule_warnings_only = 0;

Expand Down Expand Up @@ -861,6 +863,51 @@ 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");
switch (cd->cmd) {
case DETECT_FLOWBITS_CMD_NOALERT:
jb_set_string(js, "action", "noalert");
break;
Comment on lines +871 to +873
Copy link
Contributor

Choose a reason for hiding this comment

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

Now, we know from our discussions elsewhere that we can't ever have this one here, so we can probably skip the jb call, maybe just add a comment that noalert doesn't lead to calling DumpMatches? 🤔

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;
}
int flag = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

As it's being used as a bool, let's declare it as such ;)
We can also use a more descriptive name, maybe is_or?
Could we move it into the if noalert context?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to everything

if (cd->cmd != DETECT_FLOWBITS_CMD_NOALERT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

And now wondering if we need this noalert if, here.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we do. I think we should also remove this constant though bc it gives me an impression that this is a valid flowbit specific command and while it is used w flowbits, it is never really used to set cmd.

Copy link
Member

Choose a reason for hiding this comment

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

wrt I have made #10021
If it gets accepted, please rebase your code on that PR and remove all usages of DETECT_FLOWBITS_CMD_NOALERT.
If it gets rejected, maybe just add some notes like Juliana mentioned about noalert control never getting to this part.

jb_open_array(js, "names");
if (cd->or_list_size == 0) {
jb_append_string(js, VarNameStoreSetupLookup(cd->idx, VAR_TYPE_FLOW_BIT));
} else if (cd->or_list_size > 0) {
flag = 1;
for (uint8_t i = 0; i < cd->or_list_size; i++) {
const char *varname =
VarNameStoreSetupLookup(cd->or_list[i], VAR_TYPE_FLOW_BIT);
jb_append_string(js, varname);
}
}
jb_close(js); // array
if (flag == 1) {
jb_set_string(js, "operator", "or");
}
}
Comment on lines +904 to +907
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw that this is working! :) Do you think we can move on to thinking about the and case implementation now, @inashivb ?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed. We don't define it as a specific operator anyway like we do for or. It's only fair. 😛

jb_close(js); // object
break;
}
}
jb_close(js);

Expand Down
Loading