Output alert applayer v5 by catenacyber · Pull Request #8922 · OISF/suricata

catenacyber · 2023-05-25T11:32:38Z

Link to redmine ticket:
None, preliminary work for https://redmine.openinfosecfoundation.org/issues/5053 and app-layer plugins
Continuation of #8772
https://redmine.openinfosecfoundation.org/issues/5977

Describe changes:

Fix setup-app-layer script so that it adds app-layer metadata to alerts
Adds ftp metadata to alerts
Adds tftp metadata to alerts
Adds krb5 metadata to alerts
Removes tx_id from ssh and http2 logging

Continues #8893 with rebase and adding ticket number to krb5 commit

SV_BRANCH=pr/1196

OISF/suricata-verify#1196

Still to do :

Create tickets for missing protocols : pgsql, dcerpc, dhcp,
Any ideas about the commit segmentation ?

Especially fix setup-app-layer script to not forget this part

Ticket: OISF#5977

and used by bittorrent

And fix setup app layer script up for it

catenacyber · 2023-05-25T11:35:38Z

Create tickets for missing protocols : pgsql, dcerpc, dhcp,

I created https://redmine.openinfosecfoundation.org/issues/6090 https://redmine.openinfosecfoundation.org/issues/6091 and https://redmine.openinfosecfoundation.org/issues/6092

codecov · 2023-05-25T12:25:53Z

Codecov Report

Merging #8922 (9ccda72) into master (ebe0a7b) will increase coverage by 0.04%.
The diff coverage is 96.96%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8922      +/-   ##
==========================================
+ Coverage   82.30%   82.34%   +0.04%     
==========================================
  Files         969      957      -12     
  Lines      273335   272773     -562     
==========================================
- Hits       224961   224627     -334     
+ Misses      48374    48146     -228

Flag	Coverage Δ
fuzzcorpus	`64.84% <84.24%> (+0.19%)`	⬆️
suricata-verify	`60.38% <96.96%> (-0.08%)`	⬇️
unittests	`63.07% <24.84%> (+0.12%)`	⬆️

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

victorjulien

Main issue is the central table, see inline.

victorjulien · 2023-05-25T17:29:53Z

src/output-json-mqtt.c

-    if (!rs_mqtt_logger_log(state, tx, thread->mqttlog_ctx->flags, js))
+    jb_open_object(js, "mqtt");
+    if (!rs_mqtt_logger_log(tx, thread->mqttlog_ctx->flags, js))
        goto error;


no close on error?

No, the error handler just free's it. Its not used anymore.

Indeed as Jason said.

To be more complete :

When we log a MQTT event, if we fail to log the MQTT data, we do not log anything at all

When we log an alert, if we fail to log the MQTT data, we still log the alert

src/output.c

victorjulien · 2023-05-25T17:33:55Z

src/output.c

+    }
+}
+
+static AppLayerLogger alert_applayer_loggers[ALPROTO_MAX] = {


I don't really like this idea of handling these per protocol things be in a central table. Seems an anti-pattern wrt trying to make it as much a plugin as possible.

The use of this central table is indeed the central point, and it is meant to allow the use of app-layer plugins

What do we want ?
I think we want

to avoid duplicating boilerplate code (and remove C file, to get better rust stats)

that every app-layer protocols log their metadata for alerts, and for their own events

The solution currently used is to have

For alerts, output-json-alert.c has a big switch with boilerplate wrapper code calling hardcoded app-layer logging functions, and this is missing for several protocols

So, the use of a central table, for alerts,

makes it easy to sport the missing protocols (and to add them, as is done in this PR for krb5 for instance)

removes the boilerplate code (taking the mark, trying to log, closing or restoring the mark...)

makes it easy to extend by plugins (that "just" need to extend the array)

For app-layer events, we have registration of boilerplate-wrapper functions that are registered in a central table
static OutputTxLogger *list[ALPROTO_MAX] in output-tx.c

Here, this PR

removes boilerplate code for simple logger (not ones using state, or flags...), like JsonLogThreadInit already exists but is not used by every protocol and JsonGenericLogger is introduced making use of the new table for simple loggers

So @victorjulien what do you think ?
It looks like a design flaw to have different wrapper functions for logging alert and app-layer metadata (causing not to have app-layer metadata for every protocol/alert)
Most of the protocols/cases are simple, but some require more complexity

Should I make a first part of this PR with the uncontroversial changes ? (first commits, use of JsonLogThreadInit)

I think a centralized store doesn't have to use a table of hardcoded entries, but could also be more dynamic, like with many other parts of the engine. The parsers could then all a registration function that adds their entry into the store.

Well, not sure to see the point but this is quite easy to add :-)

src/output.h

victorjulien · 2023-05-25T17:35:02Z

src/output-json-dns.c

    }
 }

+bool AlertJsonDns(void *txptr, JsonBuilder *js)


should this return false if both calls return NULL?

Likely so, this is a behavioral change

Likely so, this is a behavioral change

Not a big one right? It would allow the caller to reset to mark to prevent an empty dns object from being logger in alert.

Does not look a big one

#9005 is the part without behavior changes

Then I intend to do the behavioral changes about alerts : addition of protocols missing alert metadata (like krb5) + behavioral change for dns alert metadata

victorjulien · 2023-05-25T17:37:08Z

rust/src/modbus/log.rs

@@ -27,7 +27,6 @@ pub extern "C" fn rs_modbus_to_json(tx: &mut ModbusTransaction, js: &mut JsonBui



this commit does a lot more than the commit message indicates

Indeed, so, any ideas about the commit segmentation ?
One big commit ? One commit per protocol ? (but if so, the first commit introducing the generic changes should also be applied on some protocol, otherwise, commit-check will fail because of unused functions)

Should I split this PR ?

src/output.c

suricata-qa · 2023-05-25T18:05:03Z

Information: QA ran without warnings.

Pipeline 14069

catenacyber · 2023-06-05T06:31:01Z

Main issue is the central table, see inline.

Thanks for the review, sorry for taking long to respond.
I answer inline

catenacyber · 2023-06-05T07:19:24Z

Continues in #8961

catenacyber added 28 commits May 25, 2023 13:29

output/smtp: remove unused function parameters

9d7a421

output/dns: remove unused function parameters

a00fc00

output/ftp: remove unused function parameters

09830fd

output/alert: rewrite code for app-layer properties

18bb970

Especially fix setup-app-layer script to not forget this part

output/ftp: have ftp properties in alerts

19688ba

output/tftp: have tftp properties in alerts

30c8fb6

output/mqtt: reuse standard code

c0eeac3

output/rfb: reuse standard code

f01e8d6

output/snmp: reuse standard code

f941be0

output/krb5: have krb5 properties in alerts

ac58f4d

Ticket: OISF#5977

output/ftp-data: reuse standard code

ee833ff

output/tls: reuse standard code

00d8267

output: comments for non-generic app-layers

4248f4d

output: generic tx json logger

3be26a4

and used by bittorrent

output/http2: generic tx json logger

9c5f0f4

output/rdp: generic tx json logger

89483ce

output: code reuse for generic tx json logger

d203d3a

output/rfb: generic tx json logger

81f8576

output/sip: generic tx json logger

4430125

output/snmp: generic tx json logger

5c7b7c1

output/quic: generic tx json logger

51e94c4

output/krb5: generic tx json logger

1fc1088

output/tftp: generic tx json logger

31608fc

output/dnp3: restrict function scope to one file

f8bb8ef

output/modbus: generic tx json logger

1ff1ad8

output/ssh: generic tx json logger

d5a2864

output/http2: reuse code for file events

41b36b2

output/template: reuse code for file events

9ccda72

And fix setup app layer script up for it

catenacyber requested review from jasonish and victorjulien as code owners May 25, 2023 11:32

catenacyber requested a review from a team as a code owner May 25, 2023 11:32

catenacyber mentioned this pull request May 25, 2023

Output alert applayer v4 #8893

Closed

victorjulien requested changes May 25, 2023

View reviewed changes

victorjulien reviewed May 25, 2023

View reviewed changes

src/output.c Show resolved Hide resolved

catenacyber mentioned this pull request Jun 5, 2023

Output alert applayer v6 #8961

Closed

catenacyber closed this Jun 5, 2023

catenacyber mentioned this pull request Jun 9, 2023

Output alert applayer v7.1 #9005

Closed

		@@ -27,7 +27,6 @@ pub extern "C" fn rs_modbus_to_json(tx: &mut ModbusTransaction, js: &mut JsonBui

Conversation

catenacyber commented May 25, 2023

Uh oh!

catenacyber commented May 25, 2023

Uh oh!

codecov bot commented May 25, 2023

Codecov Report

Uh oh!

victorjulien left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

suricata-qa commented May 25, 2023

Uh oh!

catenacyber commented Jun 5, 2023

Uh oh!

catenacyber commented Jun 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

Comments