Packet alerts/v8 #7000

jufajardini · 2022-02-14T17:28:35Z

Previous PR: #6999

Describe changes:

have a more user friendly approach in case of configuration having PACKET_ALERT_MAX: 0 (concern raised by @inashivb )

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/4207

suricata-verify-pr: 743
OISF/suricata-verify#743

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.

jufajardini · 2022-02-14T17:30:44Z

Assigning it to @catenacyber so he can decide if his concerns have been addressed properly ^^

codecov · 2022-02-14T18:08:51Z

Codecov Report

Merging #7000 (7a420a9) into master (b5166bd) will decrease coverage by 0.07%.
The diff coverage is 98.06%.

@@            Coverage Diff             @@
##           master    #7000      +/-   ##
==========================================
- Coverage   77.70%   77.63%   -0.08%     
==========================================
  Files         628      628              
  Lines      185656   185557      -99     
==========================================
- Hits       144270   144057     -213     
- Misses      41386    41500     +114

Flag	Coverage Δ
fuzzcorpus	`57.94% <73.52%> (-0.41%)`	⬇️
suricata-verify	`54.45% <96.42%> (-0.01%)`	⬇️
unittests	`63.00% <93.95%> (-0.03%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

suricata-qa · 2022-02-15T05:15:05Z

Information:

Passed without warnings.

Pipeline 6248

catenacyber · 2022-02-17T11:21:27Z

Will re look into this...

catenacyber

I think BUG_ON can be triggered.
Otherwise, some nits

catenacyber · 2022-02-22T11:49:14Z

src/tests/detect.c

@@ -4307,10 +4264,9 @@ static int SigTestWithin01 (void)
    }

    /* packet 4 */
-    p4 = SCMalloc(SIZE_OF_PACKET);
+    p4 = PacketGetFromAlloc();


This pattern seems to remain in doc/devguide/codebase/testing.rst cf git grep SIZE_OF_PACKET

hmmm, ok, will check. Thanks!

catenacyber · 2022-02-22T11:50:03Z

src/detect-engine-alert.c

 /**
 * \brief Check if a certain sid alerted, this is used in the test functions
 *
 * \param p   Packet on which we want to check if the signature alerted or not
- * \param sid Signature id of the signature that thas to be checked for a match
+ * \param sid Signature id of the signature that has to be checked for a match


commit should be fix typo unless it is some meta joke ;-)

And I see no

Plus small clang formatting change.

xD lol. the things that our brains do to us.

the formatting change thing is that extra line that was also deleted. Thanks <3

catenacyber · 2022-02-22T11:50:51Z

src/detect-engine-alert.h

@@ -32,7 +32,6 @@ void PacketAlertFinalize(DetectEngineCtx *, DetectEngineThreadCtx *, Packet *);
 int PacketAlertAppend(DetectEngineThreadCtx *, const Signature *,
        Packet *, uint64_t tx_id, uint8_t);
 int PacketAlertCheck(Packet *, uint32_t);
-int PacketAlertRemove(Packet *, uint16_t);


Love this kind of diff

catenacyber · 2022-02-22T11:52:00Z

src/detect-engine-alert.c

-    int match = 0;
-
-    if (pos > p->alerts.cnt) {
+    if (pos >= p->alerts.cnt) {


This change off-by-1 seems important for its own commit, is it not ?

This was done by @victorjulien though. Is it ok if I change commit history to have this patch on its own commit?

catenacyber · 2022-02-22T11:53:24Z

src/detect-engine-alert.c

-        memcpy(&p->alerts.alerts[i], &p->alerts.alerts[i + 1], sizeof(PacketAlert));
+    const uint16_t after = (p->alerts.cnt - 1) - pos;
+    if (after) {
+        const uint16_t after_start = pos + 1;


nit:
I would not name after_start
It confused me when I understood pos + 1

catenacyber · 2022-02-22T11:54:37Z

src/detect-engine-alert.c

-            memcpy(&p->alerts.alerts[i + 1], &p->alerts.alerts[i], sizeof(PacketAlert));
-        }
+        /* find position to insert */
+        for (i = p->alerts.cnt - 1; i >= 0 && p->alerts.alerts[i].num > s->num; i--)


would love to have i as uint16_t as everything else

catenacyber · 2022-02-22T11:59:35Z

src/detect-engine-alert.c

@@ -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);


⚠️ 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 ?

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

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

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.

I'll try to add a unittest, too

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

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...

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

I see. This logic seem to be done elsewhere, at the moment...

catenacyber · 2022-02-22T12:01:31Z

src/decode.c

+        if (max <= 0 || max > UINT8_MAX) {
+            SCLogWarning(SC_ERR_INVALID_VALUE,
+                    "Invalid value for packet-alert-max, default value set instead");
+            packet_alert_max = PACKET_ALERT_MAX;


Is this necessary ?

We thought it would be good to check if we don't have invalid values here, and Victor suggested that I followed this style found elsewhere...

The check is necessary indeed.
The line packet_alert_max = PACKET_ALERT_MAX; does not seem to be.

Ok, got it now, thanks for bearing with me xD

catenacyber · 2022-02-22T12:21:32Z

And doc is missing to describe packet-alert-max in suricata.yaml right ?

jufajardini · 2022-03-10T16:02:37Z

Replaced by #7133

When createing a new PgsqlTransaction, we first increment PgsqlState's tx_id and then pass that onto the new tx tx_id. As it is not created they're first id 0, there's no need to adjust when freeing or searching. Bug OISF#7000

When we create a new transaction, we increment the State's tx_id and pass that to the tx.tx_id. so, when calling get_tx and free_tx, we shouldn't need to adjust the tx_id again. Related to Bug OISF#7000

Expose the raw stream earlier to the detection engine, as Pgsql can have multiple messages per transaction and usually will have a message complete within one TCP packet. Related to Bug OISF#7000

Expose the raw stream earlier to the detection engine, as Pgsql can have multiple messages per transaction and usually will have a message complete within one TCP packet. Bug OISF#7000 Related to Bug OISF#7026

Expose the raw stream earlier to the detection engine, as Pgsql can have multiple messages per transaction and usually will have a message complete within one TCP packet. Bug OISF#7000 Related to Bug OISF#7026 (cherry picked from commit 0946c21)

Task OISF#7000

Once we are tracking tx progress per-direction for PGSQL, we can trigger the raw stream reassembly for detection purposes once the transactions are completed. Task OISF#7000

Once we are tracking tx progress per-direction for PGSQL, we can trigger the raw stream reassembly, for detection purposes, as soonas the transactions are completed in the given direction. Task OISF#7000

Once we are tracking tx progress per-direction for PGSQL, we can trigger the raw stream reassembly, for detection purposes, as soon as the transactions are completed in the given direction. Task OISF#7000

Once we are tracking tx progress per-direction for PGSQL, we can trigger the raw stream reassembly, for detection purposes, as soon as the transactions are completed in the given direction. Task OISF#7000 (cherry picked from commit 2b1ad81)

jufajardini and others added 6 commits February 11, 2022 16:28

decode: make packet_alert_max configurable

00c8cde

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

detect-engine-alert: fix tipo

f173952

Plus small clang formatting change.

detect/alert: optimize alert queue processing

06c6e25

detect/alert: remove redundant check

1827351

When appending a new alert, there is no need to check if alerts.cnt > 0 since alerts.cnt is unsigned.

jufajardini requested a review from a team as a code owner February 14, 2022 17:28

jufajardini mentioned this pull request Feb 14, 2022

Packet alerts/v7 #6999

Closed

jufajardini assigned catenacyber Feb 14, 2022

jufajardini mentioned this pull request Feb 14, 2022

Add tests for packet_alert_max - v3 OISF/suricata-verify#743

Closed

jufajardini mentioned this pull request Feb 15, 2022

test/http2-files: add comparison for fast.log output - v1 OISF/suricata-verify#744

Closed

catenacyber requested changes Feb 22, 2022

View reviewed changes

jufajardini mentioned this pull request Mar 10, 2022

Packet alerts/v9 #7133

Closed

jufajardini closed this Mar 10, 2022

jufajardini deleted the packet-alerts/v8 branch May 2, 2022 13:01

jufajardini added a commit to jufajardini/suricata that referenced this pull request Jul 17, 2024

pgsql: trigger raw stream reassembly at tx completion

de11dfc

Task OISF#7000

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packet alerts/v8 #7000

Packet alerts/v8 #7000

jufajardini commented Feb 14, 2022

jufajardini commented Feb 14, 2022

codecov bot commented Feb 14, 2022

suricata-qa commented Feb 15, 2022 •

edited

Loading

catenacyber commented Feb 17, 2022

catenacyber left a comment

catenacyber Feb 22, 2022

jufajardini Feb 25, 2022

catenacyber Feb 22, 2022

catenacyber Feb 22, 2022

jufajardini Feb 22, 2022

catenacyber Feb 22, 2022

catenacyber Feb 22, 2022

jufajardini Feb 23, 2022

catenacyber Feb 22, 2022

catenacyber Feb 22, 2022

catenacyber Feb 22, 2022

jufajardini Feb 23, 2022

catenacyber Feb 23, 2022

jufajardini Feb 23, 2022

jufajardini Feb 23, 2022

jufajardini Feb 23, 2022

jufajardini Feb 23, 2022

catenacyber Feb 23, 2022

jufajardini Feb 23, 2022

catenacyber Feb 22, 2022

jufajardini Feb 23, 2022

catenacyber Feb 23, 2022

jufajardini Feb 23, 2022

catenacyber commented Feb 22, 2022

jufajardini commented Mar 10, 2022

Packet alerts/v8 #7000

Packet alerts/v8 #7000

Conversation

jufajardini commented Feb 14, 2022

jufajardini commented Feb 14, 2022

codecov bot commented Feb 14, 2022

Codecov Report

suricata-qa commented Feb 15, 2022 • edited Loading

catenacyber commented Feb 17, 2022

catenacyber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catenacyber commented Feb 22, 2022

jufajardini commented Mar 10, 2022

suricata-qa commented Feb 15, 2022 •

edited

Loading