output/eve: add 'verdict' field to 'alert' and 'drop' events - v8 #9162

jufajardini · 2023-07-06T15:40:10Z

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

Previous PR: #8596

Sharing this now to clarify one aspect, and check if this is going on the right direction.

Describe changes from previous PR:

add verdict as a new output event
use flags to set configuration options for drop event (haven't touched the flows all/start one, as this was leading to failures, but can make it a TODO
drop idea of having alert.action as optional
incorporate verdict format suggestions from previous PR

TODOs:

add more examples in the documentation
document the json schema
document verdict configs for suricata.yaml (userguide)

Provide values to any of the below to override the defaults.

SV_BRANCH=pr/1293

OISF/suricata-verify#1293

Outputs examples:

# ids mode, reject, verdict event:
{
  "timestamp": "2004-05-13T10:17:08.993643+0000",
  "flow_id": 1055224944964893,
  "pcap_cnt": 6,
  "event_type": "verdict",
  "src_ip": "65.208.228.223",
  "src_port": 80,
  "dest_ip": "145.254.160.237",
  "dest_port": 3372,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "verdict": {
    "action": "alert",
    "reject-target": "source",
    "reject": "[tcp-reset, icmp-prohib, user defined]"
  }
}

# ids mode, reject, alert event (non-interesting portions removed):
{
  "timestamp": "2004-05-13T10:17:08.993643+0000",
  "flow_id": 1055224944964893,
  "pcap_cnt": 6,
  "event_type": "alert",
  "src_ip": "65.208.228.223",
  "src_port": 80,
  "dest_ip": "145.254.160.237",
  "dest_port": 3372,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "alert": {
    "action": "blocked",
    "gid": 1,
    "signature_id": 1000001,
    "rev": 0,
    "signature": "",
    "category": "",
    "severity": 3
  },
...
  "verdict": {
    "action": "alert",
    "reject-target": "source",
    "reject": "[tcp-reset, icmp-prohib, user defined]"
  }
}
# ids mode, alert:
{
  "timestamp": "2004-05-13T10:17:08.783340+0000",
  "flow_id": 1055224944964893,
  "pcap_cnt": 5,
  "event_type": "verdict",
  "src_ip": "65.208.228.223",
  "src_port": 80,
  "dest_ip": "145.254.160.237",
  "dest_port": 3372,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "verdict": {
    "action": "alert"
  }
}
# ips mode, drop event:
{
  "timestamp": "2016-07-13T22:42:07.388277+0000",
  "flow_id": 2019293786341530,
  "pcap_cnt": 8,
  "event_type": "drop",
  "src_ip": "10.16.1.11",
  "src_port": 54186,
  "dest_ip": "82.165.177.154",
  "dest_port": 80,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "direction": "to_server",
  "drop": {
    "len": 40,
    "tos": 0,
    "ttl": 64,
    "ipid": 36762,
    "tcpseq": 2059970575,
    "tcpack": 2548486954,
    "tcpwin": 237,
    "syn": false,
    "ack": true,
    "psh": false,
    "rst": false,
    "urg": false,
    "fin": true,
    "tcpres": 0,
    "tcpurgp": 0,
    "reason": "flow drop"
  },
  "verdict": {
    "action": "drop"
  }
}
# ips mode, alert event, drop rule:
{
  "timestamp": "2016-07-13T22:42:07.199844+0000",
  "flow_id": 2019293786341530,
  "pcap_cnt": 4,
  "event_type": "alert",
  "src_ip": "10.16.1.11",
  "src_port": 54186,
  "dest_ip": "82.165.177.154",
  "dest_port": 80,
  "proto": "TCP",
  "pkt_src": "wire/pcap",
  "alert": {
    "action": "blocked",
    "gid": 1,
    "signature_id": 2,
    "rev": 1,
    "signature": "",
    "category": "",
    "severity": 3
  },
...
  "verdict": {
    "action": "drop"
  }
}

Besides having the alert's verdict as a part of the alert event, also offer the ability to have it as an independent event type. Related to Task OISF#5464

Related to Bug OISF#5464

The `field action` portion seemed to be comprised of a more generic section that followed it. Also formatted the section for lines to be within the character limit.

jufajardini · 2023-07-06T15:42:27Z

src/output-json-drop.c

-#define LOG_DROP_ALERTS 1
+#define LOG_DROP_ALERTS  BIT_U8(1)
+#define LOG_DROP_VERDICT BIT_U8(2)


Changed this, tried adding the flows start/all option here, but led to failures. Can make this a TODO.

jufajardini · 2023-07-06T15:44:43Z

src/output-json-verdict.c

+        } else if (p->action & ACTION_REJECT_BOTH) {
+            JB_SET_STRING(jb, "reject-target", "both");
+        }
+        JB_SET_STRING(jb, "reject", "[tcp-reset, icmp-prohib, user defined]");


This is the bit I wasn't sure about the approach, @victorjulien . The examples from #8596 (comment) show all options. If that's the case, would this work? Or do we want to check what is actually being done by Suri, and output only that option? If so, I'm done sure how to figure out when will it be user defined...

The other two I figured I could decide by checking the packet proto.

this shouldn't be a single string, but a json array of strings

"user defined" can be ignored, it was a place holder for some future ideas

I see, thanks.

codecov · 2023-07-06T19:06:18Z

Codecov Report

Merging #9162 (24c9bf8) into master (9b09b29) will decrease coverage by 0.01%.
The diff coverage is 81.25%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9162      +/-   ##
==========================================
- Coverage   82.34%   82.33%   -0.01%     
==========================================
  Files         968      969       +1     
  Lines      273546   273656     +110     
==========================================
+ Hits       225247   225317      +70     
- Misses      48299    48339      +40

Flag	Coverage Δ
fuzzcorpus	`64.54% <8.03%> (-0.06%)`	⬇️
suricata-verify	`60.74% <81.25%> (-0.01%)`	⬇️
unittests	`62.88% <5.35%> (-0.03%)`	⬇️

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

suricata-qa · 2023-07-06T20:10:04Z

WARNING:

field	baseline	test	%
	SURI_TLPR1_stats_chk
.flow.memuse	592373632	511756848	86.39%

Pipeline 15018

victorjulien · 2023-07-10T15:34:42Z

doc/userguide/output/eve/eve-json-format.rst

+that this does not necessarily indicate the final verdict for a given packet or
+flow, since one packet may match on several rules.
+
+Verdict Field


this is not a field, but an object containing other fields, right?

Right, I was seeing it as a field because it's part of 'alert', but... that's indeed wrong.

also: update description, there are more fields in verdict now.

victorjulien · 2023-07-10T15:35:14Z

doc/userguide/output/eve/eve-json-format.rst

+   "verdict": {
+      "action": "alert"
+      "reject-target": "both"
+      "reject": [ "tcp-reset", "icmp-prohib", "user defined" ]


the value here should be a json array of strings

victorjulien · 2023-07-10T15:39:24Z

src/output-json-verdict.c

+    JsonVerdictLogThread *vlt = thread_data;
+
+    int r = VerdictJson(vlt, p);
+


remove newline

victorjulien · 2023-07-10T15:41:49Z

src/output-json-verdict.c

+
+static int JsonVerdictLogCondition(ThreadVars *tv, void *thread_data, const Packet *p)
+{
+    if (p->alerts.cnt > 0) {


so verdict records are only written for alerts... this excludes drops from outside the detect engine (e.g. exception policy), also "pass" would be excluded, right?

Right on cue for both aspects, I hadn't considered them >__<'

victorjulien · 2023-07-10T15:42:18Z

suricata.yaml.in

@@ -168,6 +168,10 @@ outputs:
            # Enable the logging of tagged packets for rules using the
            # "tag" keyword.
            tagged-packets: yes
+            # Enable logging the final verdict for packets with the alert
+            verdict: yes
+        - verdict:


add a comment to explain on a high level what this is

victorjulien

see inline comments

jufajardini · 2023-07-12T02:13:30Z

Replaced by: #9216

jufajardini added 6 commits July 6, 2023 09:33

userguide/eve: add section about drop event type

412c08c

misc: fix typos & update copyright years

a0509c0

output: add verdict as an event type

1fa4c22

Besides having the alert's verdict as a part of the alert event, also offer the ability to have it as an independent event type. Related to Task OISF#5464

output/alert: add verdict field

6615f84

Related to Bug OISF#5464

outputs/drop: add verdict field

aa205da

Related to Bug OISF#5464

userguide/eve: format and reorganize alert section

24c9bf8

The `field action` portion seemed to be comprised of a more generic section that followed it. Also formatted the section for lines to be within the character limit.

This was referenced Jul 6, 2023

tests: showcase verdict field/events in tests - v2 OISF/suricata-verify#1293

Closed

output/eve: add 'verdict' field to 'alert' and 'drop' events - v7 #8596

Closed

jufajardini commented Jul 6, 2023

View reviewed changes

victorjulien reviewed Jul 10, 2023

View reviewed changes

victorjulien requested changes Jul 10, 2023

View reviewed changes

jufajardini mentioned this pull request Jul 12, 2023

output/eve: add 'verdict' field to 'alert' and 'drop' events - v9 #9216

Closed

jufajardini closed this Jul 12, 2023

jufajardini mentioned this pull request Jul 12, 2023

output/eve: add 'verdict' field to 'alert' and 'drop' events - v10 #9220

Closed

jufajardini deleted the alert-action/v8.1 branch July 14, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

output/eve: add 'verdict' field to 'alert' and 'drop' events - v8 #9162

output/eve: add 'verdict' field to 'alert' and 'drop' events - v8 #9162

jufajardini commented Jul 6, 2023 •

edited

Loading

jufajardini Jul 6, 2023

jufajardini Jul 6, 2023

victorjulien Jul 10, 2023

victorjulien Jul 10, 2023

jufajardini Jul 11, 2023

codecov bot commented Jul 6, 2023

suricata-qa commented Jul 6, 2023

victorjulien Jul 10, 2023

jufajardini Jul 11, 2023

jufajardini Jul 11, 2023

victorjulien Jul 10, 2023

victorjulien Jul 10, 2023

victorjulien Jul 10, 2023

jufajardini Jul 11, 2023

victorjulien Jul 10, 2023

victorjulien left a comment

jufajardini commented Jul 12, 2023

		JsonVerdictLogThread *vlt = thread_data;

		int r = VerdictJson(vlt, p);

output/eve: add 'verdict' field to 'alert' and 'drop' events - v8 #9162

output/eve: add 'verdict' field to 'alert' and 'drop' events - v8 #9162

Conversation

jufajardini commented Jul 6, 2023 • edited Loading

Provide values to any of the below to override the defaults.

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

codecov bot commented Jul 6, 2023

Codecov Report

suricata-qa commented Jul 6, 2023

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

victorjulien left a comment

Choose a reason for hiding this comment

jufajardini commented Jul 12, 2023

jufajardini commented Jul 6, 2023 •

edited

Loading