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

analysis: report rule state altered by other rule - v2 #12311

Closed
wants to merge 1 commit into from
Closed

analysis: report rule state altered by other rule - v2 #12311

Show file tree
Hide file tree
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
10 changes: 10 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 @@ -1047,6 +1047,16 @@ void EngineAnalysisRules2(const DetectEngineCtx *de_ctx, const Signature *s)
break;
}

if (s->init_data->is_rule_state_dependant) {
jb_open_object(ctx.js, "rule_state_dependant");
jb_set_uint(ctx.js, "rule_depends_on_sid", s->init_data->rule_state_dependant_id);
Copy link
Member

Choose a reason for hiding this comment

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

what if there are multiple dependencies?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what if there are multiple dependencies?

I thought of that at first, but from what I understood, only one rule can set a flowbits variable, isn't that so?

Copy link
Member

Choose a reason for hiding this comment

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

I think a rule is only "dependent" on a certain rule or flowbit when you're checking whether a flowbit was set so essentially the isset command. set does not mean the rule is dependent.

  1. Combinations of isset that can show you multiple dependencies are as follows. Assume fb1 was set by sid: 1 and fb2 was set by sid: 2
    and support: flowbits: isset, fb1; flowbits: isset, fb2; -> dependent on sid:1 and sid: 2.
    or support: flowbits: isset, fb1|fb2; -> also dependent on sid:1 and sid: 2.
  2. Suricata does not seem to have a reservation against two different rules setting a flowbit so maybe there are usecases for that too. So, yes, there can be multiple rules setting the same flowbit.

jb_set_string(ctx.js, "rule_depends_on_flowbit",
Copy link
Member

Choose a reason for hiding this comment

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

can this just be flowbit? Seems the key is a bit redundant with the object name.

Also, as Shivani is asking: this is 1-N essentially, can we list them all? I'm changing my mind a bit on this. I think being complete is probably best.

So

"rule_state_dependant": {
    "sids": [ 1901, 124, 666 ],
    "flowbits": [ "fb2", "abc" ],
},

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought only one rule could set a flowbits rule, that's why I used this approach. Given that that's not the case, it's a bit more complicated to be done, but I'll follow that approach. I'll pursue understanding Shivani's comment about flowbits.json and see if that helps, too, somehow.

Copy link
Member

Choose a reason for hiding this comment

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

I'll pursue understanding Shivani's comment about flowbits.json and see if that helps, too, somehow.

From what I got from Victor's comment, I think your approach is what we want so you can skip that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you were actually right, thanks for the explanations and feedback :)

VarNameStoreSetupLookup(s->init_data->rule_state_variable_idx, VAR_TYPE_FLOW_BIT));
jb_close(ctx.js);
} else {
jb_set_bool(ctx.js, "rule_state_dependant", s->init_data->is_rule_state_dependant);
}

jb_open_array(ctx.js, "flags");
if (s->flags & SIG_FLAG_SRC_ANY) {
jb_append_string(ctx.js, "src_any");
Expand Down
5 changes: 5 additions & 0 deletions src/detect-flowbits.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,11 @@ int DetectFlowbitsAnalyze(DetectEngineCtx *de_ctx)

if (to_state) {
s->init_data->init_flags |= SIG_FLAG_INIT_STATE_MATCH;
s->init_data->is_rule_state_dependant = true;
// fetch the signature id that sets the flowbit making the isset rule stateful
s->init_data->rule_state_dependant_id =
de_ctx->sig_array[array[i].set_sids[array[i].set_sids_idx - 1]]->id;
s->init_data->rule_state_variable_idx = i;
SCLogDebug("made SID %u stateful because it depends on "
"stateful rules that set flowbit %s", s->id, varname);
}
Expand Down
3 changes: 3 additions & 0 deletions src/detect-parse.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1537,6 +1537,9 @@ Signature *SigAlloc (void)
* overwritten, we can then assign the default value of 3 */
sig->prio = -1;

/* rule interdepency is false, at start */
sig->init_data->is_rule_state_dependant = false;

sig->init_data->list = DETECT_SM_LIST_NOTSET;
return sig;
}
Expand Down
5 changes: 5 additions & 0 deletions src/detect.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,11 @@ typedef struct SignatureInitData_ {

/* highest list/buffer id which holds a DETECT_CONTENT */
uint32_t max_content_list_id;

/* inter-signature state dependency */
bool is_rule_state_dependant;
uint32_t rule_state_dependant_id;
uint32_t rule_state_variable_idx;
Comment on lines +600 to +604
Copy link
Member

Choose a reason for hiding this comment

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

Can we get this info to analyzer w/o inflating this struct so much?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. This is an init-time only helper struct

} SignatureInitData;

/** \brief Signature container */
Expand Down
Loading