detect-flowbits: adding details for flowbits v5 #9971
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #9971 +/- ##
==========================================
+ Coverage 82.39% 82.43% +0.04%
==========================================
Files 968 970 +2
Lines 274337 271392 -2945
==========================================
- Hits 226047 223734 -2313
+ Misses 48290 47658 -632
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
Updated PR description to make redmine ticket link visible and use |
| switch (cd->cmd) { | ||
| case DETECT_FLOWBITS_CMD_NOALERT: | ||
| jb_set_string(js, "action", "noalert"); | ||
| // jb_set_string(js,"name", NULL); |
There was a problem hiding this comment.
Please remove the commented our line
There was a problem hiding this comment.
yep, don't know how I missed that. I'll remove it right away.
There was a problem hiding this comment.
We're getting there, I see that most SV checks are passing, now, kudos!
I've left some comments for the work to continue :)
Also please note that the commit message is still not following our guidelines.
Something like detect/analyzer: add details to flowbits keyword for the commit title would work. The ticket indication is good!
| switch (cd->cmd) { | ||
| case DETECT_FLOWBITS_CMD_NOALERT: | ||
| jb_set_string(js, "action", "noalert"); | ||
| // jb_set_string(js,"name", NULL); |
There was a problem hiding this comment.
yeah, I must have missed the commented out code. Sorry!
| 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) { | ||
| 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 |
There was a problem hiding this comment.
Considering that for noalert there won't be an associated variable name, I don't think we want to always open this array, only when it's not noalert, I guess. Not sure what's the best approach in terms of implementation, for this, right now.
There was a problem hiding this comment.
(btw, my guess is that that's what's causing the last check in the SV test to fail. this double close when nothing was added to the json object is leading to suricata not having the json object with the flowbits info, in the noalert case.)
| } | ||
| } | ||
| jb_close(js); // array | ||
| jb_close(js); // object |
There was a problem hiding this comment.
Considering the discussion here - #9691 (comment) -, we are missing the operator to be added to the details.
There was a problem hiding this comment.
For this, I was thinking about creating an operator array that stores "or" if it encounters '|' in the the list, but I'm not sure on how to store "and" yet.
There was a problem hiding this comment.
I'd say skip and for now, multiple flowbit objs in the output for a rule for the same cmd and w/o or would indicate and. It'll quickly get too complicated otherwise.
|
New PR: #10008 |
Task #6309
Link to redmine ticket: https://redmine.openinfosecfoundation.org/issues/6309
Previous PR: #9691
Describe changes:
SV_BRANCH=OISF/suricata-verify#1512