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

Packet alerts/v8 #7000

Closed
wants to merge 6 commits into from
Closed

Packet alerts/v8 #7000

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
822cf1b
unittests: alloc Packet with PacketGetFromAlloc
jufajardini Jan 28, 2022
00c8cde
decode: make packet_alert_max configurable
jufajardini Jan 19, 2022
f173952
detect-engine-alert: fix tipo
jufajardini Jan 27, 2022
06c6e25
detect/alert: optimize alert queue processing
victorjulien Jan 29, 2022
1827351
detect/alert: remove redundant check
jufajardini Feb 11, 2022
7a420a9
detect/alert: fix same sid alert w same packet bug
jufajardini Feb 14, 2022
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
4 changes: 2 additions & 2 deletions src/detect-engine-alert.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,8 +189,7 @@ int PacketAlertAppend(DetectEngineThreadCtx *det_ctx, const Signature *s,
SCLogDebug("sid %"PRIu32"", s->id);

/* It should be usually the last, so check it before iterating */
if (p->alerts.cnt == 0 || (p->alerts.cnt > 0 &&
p->alerts.alerts[p->alerts.cnt - 1].num < s->num)) {
if (p->alerts.cnt == 0 || p->alerts.alerts[p->alerts.cnt - 1].num < s->num) {
/* We just add it */
p->alerts.alerts[p->alerts.cnt].num = s->num;
p->alerts.alerts[p->alerts.cnt].action = s->action;
Expand All @@ -209,6 +208,7 @@ int PacketAlertAppend(DetectEngineThreadCtx *det_ctx, const Signature *s,
const uint16_t target_pos = i + 1;
const uint16_t space_post_target = packet_alert_max - 1 - i;
const uint16_t to_move = MIN(space_post_target, (p->alerts.cnt - i));
BUG_ON(to_move == 0);
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 triggered :

It can be 0 : if space_post_target is 0 which can be the case if you have like packet_alert_max= 3, you have 3 alerts with num 3,5,7 and you want to add one with num 6

Than no memmove, just replace the last one

cf #6896 (comment)

Is it not the case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was indeed triggering before, but after the line 193 patch, it has stopped, to me.

It can be 0 : if space_post_target is 0 which can be the case if you have like packet_alert_max= 3, you have 3 alerts with num 3,5,7 and you want to add one with num 6

Would a test like this be a good addition, to check for this type of situation? It passes, for me.
jufajardini/suricata-verify@1a5688d

Copy link
Contributor

Choose a reason for hiding this comment

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

but after the line 193 patch, it has stopped, to me.

You mean the patch at line 186, right ?
I think this check is wrong

if (p->alerts.cnt == packet_alert_max)
        return 0;

in the sense, that if it prevents the BUG_ON which is good, we do not get the expected outcome.

That is if packet_alert_max == 3 and signatures 3,5,7,6 are added as alerts, in the end, we get 3,5,7 instead of 3,5,6

Would a test like this be a good addition, to check for this type of situation? It passes, for me.

I do not know enough about the context to tell if the S-V test is good or not : I do not know how Suricata will add the alerts, like 3,5,7,6 or 3,5,6,7...
So, there should be more a unit test...
Od do you know more about the context ? ie the order of the calls to PacketAlertAppend

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 was talking this -> if (p->alerts.cnt == 0 || p->alerts.alerts[p->alerts.cnt - 1].num <= s->num)
It used to be just p->alerts.alerts[p->alerts.cnt - 1].num < s->num and that could trigger the BUG_ON, but once I changed that, I didn't see it be triggered again.

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'll try to add a unittest, too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but after the line 193 patch, it has stopped, to me.

You mean the patch at line 186, right ? I think this check is wrong

if (p->alerts.cnt == packet_alert_max)
        return 0;

in the sense, that if it prevents the BUG_ON which is good, we do not get the expected outcome.

That is if packet_alert_max == 3 and signatures 3,5,7,6 are added as alerts, in the end, we get 3,5,7 instead of 3,5,6

So the expected behavior if we have more sig matches than packet_alert_ max is to drop some older alert for the new? Is that so because once we have added an alert to the queue, it is already logged/ taken care of, so we can let go of it?

Would a test like this be a good addition, to check for this type of situation? It passes, for me.

I do not know enough about the context to tell if the S-V test is good or not : I do not know how Suricata will add the alerts, like 3,5,7,6 or 3,5,6,7... So, there should be more a unit test... Od do you know more about the context ? ie the order of the calls to PacketAlertAppend

When I run that s-v test with gdb, the result is that signatures are appended 3,5,6,7, which means we do not append sig 7

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'm thinking that with https://redmine.openinfosecfoundation.org/issues/4943 the behavior for PacketAlertAppend is to just append the signatures that Suri has decided should stay...

Copy link
Contributor

Choose a reason for hiding this comment

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

So the expected behavior if we have more sig matches than packet_alert_ max is to drop some older alert for the new? Is that so because once we have added an alert to the queue, it is already logged/ taken care of, so we can let go of it?

There is a priority order on the signatures (which is not exactly their signature id I think)
So, we should keep the highest priority ones

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 see. This logic seem to be done elsewhere, at the moment...

memmove(p->alerts.alerts + target_pos, p->alerts.alerts + i,
(to_move * sizeof(PacketAlert)));

Expand Down