Packet alerts/v7 #6999
Closed
Packet alerts/v7 #6999
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some unittests used SCMalloc for allocating new Packet the unittests. While this is valid, it leads to segmentation faults when we move to dynamic allocation of the maximum alerts allowed to be triggered by a single packet. This massive patch uses PacketGetFromAlloc, which initializes a Packet in such a way that any dynamic allocated structures within will also be initialized. Related to Task OISF#4207
The maximum of possible alerts triggered by a unique packet was hardcoded to 15. With usage of 'noalert' rules, that limit could be reached somewhat easily. Make that configurable via suricata.yaml. Conf Bug#4941 Task OISF#4207
Plus small clang formatting change.
When appending a new alert, there is no need to check if alerts.cnt > 0 since alerts.cnt is unsigned.
with the implementation of configurable packet-alert-max, a bug was found in the way we added alerts to the packet queue. But the new logic exposed a possible bug case when the same signature generated more than one alert for the same packet (from different transactions). This would lead to Suri call memmove with size 0.
Codecov Report
@@ Coverage Diff @@
## master #6999 +/- ##
==========================================
- Coverage 77.70% 77.61% -0.09%
==========================================
Files 628 628
Lines 185656 185557 -99
==========================================
- Hits 144270 144028 -242
- Misses 41386 41529 +143
Flags with carried forward coverage won't be shown. Click here to find out more. |
inashivb
reviewed
Feb 14, 2022
| @@ -189,8 +189,10 @@ 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.alerts[p->alerts.cnt - 1].num < s->num) { | |||
| /* Same signatures can generate more than one alert, if it's a diff tx */ | |||
| if (p->alerts.cnt == 0 || p->alerts.alerts[p->alerts.cnt - 1].num <= s->num) { | |||
There was a problem hiding this comment.
Q: how do we know here if the alerts came from diff txs?
There was a problem hiding this comment.
Since this wasn't checked before, I assumed it was not needed...
inashivb
reviewed
Feb 14, 2022
| { | ||
| intmax_t max = 0; | ||
| if (ConfGetInt("packet-alert-max", &max) == 1) { | ||
| if (max < 0 || max > UINT8_MAX) { |
There was a problem hiding this comment.
Q: What happens in a case when max == 0?
There was a problem hiding this comment.
I think we decided that if the user chose that, that was up to them... 🤔
There was a problem hiding this comment.
change behavior to Warning and default
Closed
|
Followed by: #7000 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#6943 addressing comments done on #6896
Describe changes:
BUG_ONto check if we're not callingmemmovewith size 0Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4207
suricata-verify-pr: 694