Pcap conditional v2.2.6 #6389

scottfgjordan · 2021-09-17T20:46:14Z

Make sure these boxes are signed before submitting your Pull Request -- thank you.

I have read the contributing guide lines at https://redmine.openinfosecfoundation.org/projects/suricata/wiki/Contributing
I have signed the Open Information Security Foundation contribution agreement at https://suricata-ids.org/about/contribution-agreement/
I have updated the user guide (in doc/userguide/) to reflect the changes made (if applicable)

Link to redmine ticket:

Describe changes:

Rebase of Pcap conditional v2.2.5 #6154

suricata-verify-pr: 542

Add an option to only write to pcap packets with alerts and flow that have alerted.

It will be used later when multiple writing operations will be necessary.

We add a flag to packet to be able to know if this packet was the first one to get alerts on the flow.

This patch optionally adds packet header to the TCP segment and update the for each segment function by changing the callback. This patch is based on the work by Scott Jordan <scottfgjordan@gmail.com>

This patch update the alert mode of pcap logging. It uses the packet header data added to the TCP segments to build packets corresponding to the acked data that did trigger the alert. It then write it to the pcap file before starting to dump all packet for the flow that did alert.

In alert mode, we need to write the root packet to the pcap file instead of the packet that did trigger the alert.

This patch adds the tag as logging condition. If this option is used all tagged packets are written to the pcap.

TCP memory cap was not taking into account the memory that can be used by realloc of Packet headers in TCP segments.

This patch adds a function to get the current pcap file name that will be used to current packet.

This patch updates EVE alerts to add pcap output filename when pcap capture is done in multi mode. In other mode, the pcap file can be determined by checking the timestamps.

This patch updates tcp segment dumping to dump segments from both sides of the session in order when capturing alerts and tags.

We may need to know that a packet has been tagged but is the first one (and thus is not tagged).

We were missing the first packet when using condition pcap logging in tag mode as it was not tagged. As a result we were not getting the stream data triggering the alert in the pcap file.

As Suricata is not supporting pcap-ng we have to stick with one single datalink type for the capture if ever we want to do pcap logging. Assuming this, this patch introduces a function to set the link type globally. This will be used with pcap conditional logging to get the logging of TCP segments with the correct link type.

Set pseudo packet datalink to the global one. This fixes the case where the pcap handle is open with information coming from a pseudo packet. Without this, we did end up in most cases with an Ethernet packet being written in a Raw pcap.

codecov · 2021-09-18T06:17:10Z

Codecov Report

Merging #6389 (44b7040) into master (b3f447a) will decrease coverage by 0.03%.
The diff coverage is 62.00%.

@@            Coverage Diff             @@
##           master    #6389      +/-   ##
==========================================
- Coverage   76.96%   76.92%   -0.04%     
==========================================
  Files         611      612       +1     
  Lines      186077   186338     +261     
==========================================
+ Hits       143214   143344     +130     
- Misses      42863    42994     +131

Flag	Coverage Δ
fuzzcorpus	`52.58% <10.54%> (-0.13%)`	⬇️
suricata-verify	`51.74% <69.08%> (+0.09%)`	⬆️
unittests	`63.03% <6.20%> (-0.08%)`	⬇️

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

jlucovsky · 2021-09-20T19:39:08Z

src/log-pcap.c

+                    "Pcap logging with multiple link type is not supported.");
+        } else {
+            /* In multi mode, only pcap conditional is not supported as a flow timeout
+             * will trigger packet logging with potentially invalid with. In regular


The first sensor is missing something after with or should be reworded.

jlucovsky · 2021-09-20T19:42:08Z

src/log-pcap.c

+    /* Log alerted flow or tagged flow */
+    switch (ptd->pcap_log->conditional) {
+        case LOGMODE_COND_ALERTS:
+            if (p->alerts.cnt || (p->flow && FlowHasAlerts(p->flow))) {


Could (should?) PKT_FIRST_ALERTS be used here?

It could be, but isn't necessary. The PKT_FIRST_ALERTS flag is used to determine when the data held in the tcp segments should be dumped (i.e. before the first alerted packet is dumped, otherwise will be out of order). The FlowSetHasAlertsFlag is set in the same block as p->flags |= PKT_FIRST_ALERTS.

jlucovsky · 2021-09-20T19:44:42Z

src/log-pcap.c

+            SCLogError(SC_ERR_FSEEK, "fseek failed: %s", strerror(errno));
+            return TM_ECODE_FAILED;
+        }
+        if (fwrite(comp->buffer, 1, out_size, comp->file) < out_size) {


Can out_size be zero? See conditional at L528.

Yes it can be zero. out_size is equal to the return of the LZ4F_compressUpdate which is the number of bytes written in to the compression buffer. From the function documentation "@return: number of bytes written into 'dstBuffer' (it can be zero, meaning input data was just buffered) or an error code if it fails".

I don't believe any functionality in this block has actually changed from the original log-pcap.c file, it has just been moved to be within the PcapWrite function rather than the PcapLog function.

jlucovsky · 2021-09-20T19:47:36Z

src/log-pcap.c

+        MemBufferWriteRaw(pctx->buf, buf, buflen);
+
+        PcapWrite(pctx->pl, pctx->connp, (uint8_t *)pctx->buf->buffer,
+                seg->pcap_hdr_storage->pktlen + buflen);


Suggst using pctx->pl->h->len instead

jlucovsky · 2021-09-20T19:52:38Z

src/log-pcap.c

+#endif
+    }
+    if (ret != TM_ECODE_OK)
+        return ret;


Leaving the pcap log lock held ... need PcapLogUnlock(pl)

Also ... missing call (i think?) to PCAPLOG_PROFILE_END() when returning

Good catch, thanks!

jlucovsky · 2021-09-20T19:54:45Z

src/log-pcap.c

@@ -950,6 +1111,12 @@ static TmEcode PcapLogDataInit(ThreadVars *t, const void *initdata, void **data)

    *data = (void *)td;

+    if (IsTcpSessionDumpingEnabled()) {
+        td->buf = MemBufferCreateNew(65537);


Suggest a comment for why this is the proper value? Should it be configurable? should it be a #define?

Moved to a define in log-pcap.h, as for the particular value, would need to ask @regit as I am not sure why that was selected versus 65535 which appears to be the common OUTPUT_BUFFER_SIZE defined value in other log- files that call MemBufferCreateNew. Will leave as 65537 for now.

jlucovsky · 2021-09-20T19:56:14Z

src/log-pcap.c

+            } else if (strcasecmp(s_conditional, "all") != 0) {
+                SCLogError(SC_ERR_INVALID_ARGUMENT,
+                        "log-pcap: invalid conditional \"%s\". Valid options: \"all\", "
+                        "or \"alerts\" mode ",


tag is valid too

Good catch.

jlucovsky · 2021-09-20T19:56:27Z

src/log-pcap.c

+                pl->conditional = LOGMODE_COND_TAG;
+                EnableTcpSessionDumping();
+            } else if (strcasecmp(s_conditional, "all") != 0) {
+                SCLogError(SC_ERR_INVALID_ARGUMENT,


Suggest FatalError here.

jlucovsky · 2021-09-20T19:59:23Z

src/stream-tcp-list.c

+         * need to check that this is sufficient and allocate more memory if
+         * not.
+         */
+        if (GET_PKT_LEN(rp) - p->payload_len > seg->pcap_hdr_storage->alloclen) {


Suggest using seg->pcap_hdr_storage->pktlen here instead.

and also on L592

scottfgjordan · 2021-09-21T21:03:29Z

Working on addressing feedback ASAP, do you know if there were any QA issues similar to the test ran from the previous PR #6154 (comment) @jlucovsky? I will need to setup a similar test locally to see If I can get to the source of the issue.

scottfgjordan · 2021-09-24T12:58:55Z

Replaced by: #6408

The ConsolidatedDataRow struct had a length field that wasn't truly used. Related to Bug OISF#6389

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389 (cherry picked from commit 1afb485)

Found by oss-fuzz with quadfuzz. Cf https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=63113 According to PostgreSQL documentation the maximum number of rows can be the maximum of tuples that can fit onto max u32 pages - 4,294,967,295 (cf https://www.postgresql.org/docs/current/limits.html). Some rough calculations for that indicate that this could go over max u32, so updating the data_row data type to u64. Bug OISF#6389

Found by oss-fuzz with quadfuzz. Cf https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=63113 According to PostgreSQL documentation the maximum number of rows can be the maximum of tuples that can fit onto max u32 pages - 4,294,967,295 (cf https://www.postgresql.org/docs/current/limits.html). Some rough calculations for that indicate that this could go over max u32, so updating the data_row data type to u64. Bug OISF#6389 (cherry picked from commit 8d3de85)

regit and others added 23 commits September 17, 2021 13:43

log/pcap: conditional logging

be12250

Add an option to only write to pcap packets with alerts and flow that have alerted.

log/pcap: add PcapWrite function

c621145

It will be used later when multiple writing operations will be necessary.

detect: add flag when packet is first with alert

155dd02

We add a flag to packet to be able to know if this packet was the first one to get alerts on the flow.

stream: conditionally add packet header to segment

16ec6aa

This patch optionally adds packet header to the TCP segment and update the for each segment function by changing the callback. This patch is based on the work by Scott Jordan <scottfgjordan@gmail.com>

log/pcap: add support for tunnel logging

41631df

In alert mode, we need to write the root packet to the pcap file instead of the packet that did trigger the alert.

log/pcap: fix some indentation and white spaces

8077777

log/pcap: introduce tag as logging condition

9a37607

This patch adds the tag as logging condition. If this option is used all tagged packets are written to the pcap.

doc: add conditional pcap logging info

09b1812

log/pcap: update copyright date

3aa92e3

stream: count realloc in memcap

af394d5

TCP memory cap was not taking into account the memory that can be used by realloc of Packet headers in TCP segments.

log/pcap: fix typo in error message

dc63c19

log/pcap: add function to get thread pcap file

8ab18f9

This patch adds a function to get the current pcap file name that will be used to current packet.

eve/alert: log pcap output filename

1e84d39

This patch updates EVE alerts to add pcap output filename when pcap capture is done in multi mode. In other mode, the pcap file can be determined by checking the timestamps.

doc: add info about capture_file key

a522e9e

log/pcap: dump segments of both sides of tcp session.

ef205ee

This patch updates tcp segment dumping to dump segments from both sides of the session in order when capturing alerts and tags.

detect/tag: add a tag for first packet

42c6174

We may need to know that a packet has been tagged but is the first one (and thus is not tagged).

log/pcap: fix conditional pcap in tag mode

466505e

We were missing the first packet when using condition pcap logging in tag mode as it was not tagged. As a result we were not getting the stream data triggering the alert in the pcap file.

log/pcap: log segments for pseudo packets

d8e4980

flow: set datalink for pseudo packet

17a931b

Set pseudo packet datalink to the global one. This fixes the case where the pcap handle is open with information coming from a pseudo packet. Without this, we did end up in most cases with an Ethernet packet being written in a Raw pcap.

log/pcap: handle case of multiple link types

bd36ed1

decode: fix typo from rebase

44b7040

scottfgjordan requested review from norg and a team as code owners September 17, 2021 20:46

victorjulien mentioned this pull request Sep 18, 2021

Pcap conditional v2.2.5 #6154

Closed

3 tasks

jlucovsky reviewed Sep 20, 2021

View reviewed changes

scottfgjordan mentioned this pull request Sep 24, 2021

Pcap conditional v2.2.7 #6408

Closed

3 tasks

scottfgjordan closed this Sep 24, 2021

jufajardini added a commit to jufajardini/suricata that referenced this pull request Dec 15, 2023

pgsql: remove unused data field

2caa8d6

The ConsolidatedDataRow struct had a length field that wasn't truly used. Related to Bug OISF#6389

jufajardini added a commit to jufajardini/suricata that referenced this pull request Dec 15, 2023

pgsql: remove unused msg field

845f752

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389

jufajardini mentioned this pull request Dec 15, 2023

pgsql: remove unused msg field - v1 #10062

Closed

victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Dec 19, 2023

pgsql: remove unused msg field

1afb485

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389

jufajardini added a commit to jufajardini/suricata that referenced this pull request Dec 19, 2023

pgsql: remove unused msg field

067ca94

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389 (cherry picked from commit 1afb485)

jufajardini mentioned this pull request Dec 19, 2023

pgsql: remove unused msg field (7.0.x backport) - v1 #10079

Closed

victorjulien pushed a commit to victorjulien/suricata that referenced this pull request Dec 21, 2023

pgsql: remove unused msg field

046a18b

The `ConsolidatedDataRow` struct had a `length` field that wasn't truly used. Related to Bug OISF#6389 (cherry picked from commit 1afb485)

jufajardini mentioned this pull request Jan 4, 2024

pgsql: fix u16 overflow in query data_row - v3 #10116

Closed

jufajardini mentioned this pull request Jan 4, 2024

pgsql: fix u16 overflow in query data_row - v4 #10117

Closed

jufajardini mentioned this pull request Jan 9, 2024

pgsql: fix u16 overflow in query data_row (7.0.x backport) - v1 #10141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pcap conditional v2.2.6 #6389

Pcap conditional v2.2.6 #6389

scottfgjordan commented Sep 17, 2021

codecov bot commented Sep 18, 2021

jlucovsky Sep 20, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

scottfgjordan Sep 23, 2021

jlucovsky Sep 20, 2021

jlucovsky Sep 20, 2021

jlucovsky Sep 20, 2021

scottfgjordan commented Sep 21, 2021

scottfgjordan commented Sep 24, 2021

Pcap conditional v2.2.6 #6389

Pcap conditional v2.2.6 #6389

Conversation

scottfgjordan commented Sep 17, 2021

codecov bot commented Sep 18, 2021

Codecov Report

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

scottfgjordan commented Sep 21, 2021

scottfgjordan commented Sep 24, 2021