RFC: alert/noalert and config keyword extensions #10157

victorjulien · 2024-01-12T17:07:00Z

Other than the alert addition from #10154, this adds a way to disable stream reassembly completely and app-layer parsing (for TCP only atm).

E.g.

config smb any any -> any any (config:stream disable, type reassembly, scope flow; sid:1;)
config tcp any any -> any 445 (config:stream disable, type reassembly, scope flow; sid:2; alert;)
config tcp any any -> any 80 (flow:to_server; config:app-layer disable, type parser, scope flow; sid:3; alert;)
alert tcp any any -> any 80 (flow:to_server; content:"HTTP/1.0"; sid:4;)

Sid 1 and 2 both disable all stream reassembly for that flow.

Sid 3 only disables app parsers for on port 80. Content inspection still works (sid 4).

Would like some feedback on how to express things, and on what toggles to add.

Replaces default "alert" logic and removed SIG_FLAG_NOALERT. Instead, "noalert" unsets ACTION_ALERT. Same for flowbits:noalert and friends. In signature ordering rules w/o action are sorted as if they have 'alert', which is the same behavior as before, but now implemented explicitly. Ticket: OISF#5466.

This can be used to implement alert then pass logic. Add support for alert-then-pass to alert handling routines. Ticket: OISF#5466.

suricata-qa · 2024-01-13T02:10:05Z

Information:

ERROR: QA failed on SURI_TLPW2_autofp_suri_time.

field	baseline	test	%
	SURI_TLPW2_autofp_stats_chk
.uptime	181	192	106.08%

Pipeline 17500

codecov · 2024-01-15T13:04:16Z

Codecov Report

Attention: 39 lines in your changes are missing coverage. Please review.

Comparison is base (1dcf69b) 82.19% compared to head (cb11aa8) 82.08%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10157      +/-   ##
==========================================
- Coverage   82.19%   82.08%   -0.11%     
==========================================
  Files         974      974              
  Lines      271825   271901      +76     
==========================================
- Hits       223416   223190     -226     
- Misses      48409    48711     +302

Flag	Coverage Δ
fuzzcorpus	`62.65% <59.82%> (-0.37%)`	⬇️
suricata-verify	`61.39% <57.26%> (-0.02%)`	⬇️
unittests	`62.84% <63.90%> (-0.01%)`	⬇️

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

jufajardini

Other than the alert addition from #10154, this adds a way to disable stream reassembly completely and app-layer parsing (for TCP only atm).

Shouldn't we document this, as well as the alert keyword? (in the same section as noalert, the latter should go, I think).

Apart from that, all alert-related work seems to look good, to me. Only two minor nit comments (inline).
I saw that we have a SV test with a drop noalert rule that works as expected (bug-4663), wondering if there are other cases to ensure we cover with tests.

src/detect-engine-alert.c

jufajardini · 2024-01-15T19:58:45Z

src/detect-noalert.c


-void DetectNoalertRegister (void)
+void DetectNoalertRegister(void)


If we're registering alert and noalert here, shouldn't we rename the function?

src/detect-engine-alert.c

inashivb

LGTM mostly. Some questions, mostly nit, inline.

inashivb · 2024-01-16T07:18:23Z

src/detect-engine-analyzer.c

+    if ((s->action & ACTION_ALERT) == 0) {
        jb_append_string(ctx.js, "noalert");


So, now, during engine analysis, we log noalert flag even if it was not explicitly mentioned but just an alert was not mentioned? e.g. pass tcp any any -> any 22 (sid:2; gid:10000003; msg:\"PASS SSH\";) logs noalert in flags. Is this the intention?

I think it was a quick hack but we should really add the actions, which we don't do yet.

On further thought, I do think adding noalert for a pass makes sense. We don't alert if the rule matches.

src/detect-config.c

inashivb · 2024-01-16T08:18:35Z

src/util-config.h

    CONFIG_TYPE_FILE,       /* file logging */
    CONFIG_TYPE_PCAP,       /* pcap logging */
    CONFIG_TYPE_DROP,       /* drop logging */
+    CONFIG_TYPE_REASSEMBLY, /**< stream reassembly */
+    CONFIG_TYPE_APP_PARSER, /**< app parsing */
+    CONFIG_TYPE_APP_FILES,  /**< app files */


Lots of TBD types here. Should they be marked?

Should the scope be made a part of type like it is for APP types?

inashivb · 2024-01-16T08:32:42Z

feedback on how to express things

not sure about the hyphen in app-layer in rule lang.
For stream, other type that comes to mind: memcap [allow to disable memcap limits when say one is interested in getting full stream matching a certain criteria]
logging makes more sense as a type to me than a subsystem so we can allow more control about what part of the system we want to disable logging on. Like, say, we want to disable logging but only for stream engine or an applayer parser or a tx (this means that tx seems more suited as a subsystem)

What do you think?

victorjulien · 2024-01-16T09:09:54Z

feedback on how to express things
1. not sure about the hyphen in `app-layer` in rule lang.

What would you suggest as an alternative? app_layer?

2. For stream, other type that comes to mind: `memcap` [allow to disable memcap limits when say one is interested in getting full stream matching a certain criteria]

While this would be nice it will also be quite some work. Currently memcap is not tracked/handled with per flow/session limits, so changing that would require quite a bit of work.

3. `logging` makes more sense as a type to me than a subsystem so we can allow more control about what part of the system we want to disable logging on. Like, say, we want to disable logging but only for stream engine or an applayer parser or a tx (this means that tx seems more suited as a subsystem)

I conceptualize subsys as "capture", "decode", "stream", "app", "detect" and "output". (where logging == output here, I guess I thought it to be clearer at the time)

In any case, logging is already in 7 (and 6 I think), as a "subsys". Can rework that of course.

inashivb · 2024-01-16T09:24:44Z

What would you suggest as an alternative? app_layer?

Yes. Or even applayer. Both are consistent with our general rule keywords.

While this would be nice it will also be quite some work. Currently memcap is not tracked/handled with per flow/session limits, so changing that would require quite a bit of work.

I see. Thank you!

I conceptualize subsys as "capture", "decode", "stream", "app", "detect" and "output". (where logging == output here, I guess I thought it to be clearer at the time)

ok. I didn't see it that way indeed.

In any case, logging is already in 7 (and 6 I think), as a "subsys". Can rework that of course.

suricata-qa · 2024-01-16T09:25:03Z

Information: QA ran without warnings.

Pipeline 17531

catenacyber · 2024-01-18T10:16:17Z

Is there a redmine ticket for this ?

jufajardini · 2024-01-18T22:51:47Z

Is there a redmine ticket for this ?

For the pass-alert thing: https://redmine.openinfosecfoundation.org/issues/5466

catenacyber · 2024-04-30T09:08:16Z

Any update on this ?

suricata-qa · 2024-05-08T05:40:03Z

Information: QA ran without warnings.

Pipeline 17531

victorjulien · 2024-06-06T10:58:48Z

Replaced by #11260.

victorjulien added 4 commits January 12, 2024 14:29

detect/noalert: minor cleanup

a106c13

detect/alert: minor loop cleanup

d5de9e0

detect: implement 'alert' keyword as a companion to 'noalert'

9b3724d

This can be used to implement alert then pass logic. Add support for alert-then-pass to alert handling routines. Ticket: OISF#5466.

detect/config: minor code cleanups

57a451e

victorjulien force-pushed the detect-pass-alert/v3 branch from bd7f928 to 441efc0 Compare January 15, 2024 12:30

detect/config: implement stream/app

cb11aa8

victorjulien force-pushed the detect-pass-alert/v3 branch from 441efc0 to cb11aa8 Compare January 15, 2024 12:45

victorjulien requested review from jufajardini and inashivb January 15, 2024 15:33

jufajardini reviewed Jan 15, 2024

View reviewed changes

inashivb reviewed Jan 16, 2024

View reviewed changes

victorjulien mentioned this pull request Jun 6, 2024

Detect pass alert/v6 #11260

Closed

victorjulien closed this Jun 6, 2024

victorjulien mentioned this pull request Jun 7, 2024

Detect pass alert/v2 #10154

Closed

victorjulien deleted the detect-pass-alert/v3 branch June 23, 2024 07:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: alert/noalert and config keyword extensions #10157

RFC: alert/noalert and config keyword extensions #10157

victorjulien commented Jan 12, 2024

suricata-qa commented Jan 13, 2024

codecov bot commented Jan 15, 2024

jufajardini left a comment

jufajardini Jan 15, 2024

inashivb left a comment

inashivb Jan 16, 2024

victorjulien Jan 16, 2024

victorjulien May 16, 2024

inashivb Jan 16, 2024

inashivb commented Jan 16, 2024

victorjulien commented Jan 16, 2024 •

edited

Loading

inashivb commented Jan 16, 2024

suricata-qa commented Jan 16, 2024

catenacyber commented Jan 18, 2024

jufajardini commented Jan 18, 2024

catenacyber commented Apr 30, 2024

suricata-qa commented May 8, 2024

victorjulien commented Jun 6, 2024


		void DetectNoalertRegister (void)
		void DetectNoalertRegister(void)

		if ((s->action & ACTION_ALERT) == 0) {
		jb_append_string(ctx.js, "noalert");

RFC: alert/noalert and config keyword extensions #10157

RFC: alert/noalert and config keyword extensions #10157

Conversation

victorjulien commented Jan 12, 2024

suricata-qa commented Jan 13, 2024

codecov bot commented Jan 15, 2024

Codecov Report

jufajardini left a comment

Choose a reason for hiding this comment

jufajardini Jan 15, 2024

Choose a reason for hiding this comment

inashivb left a comment

Choose a reason for hiding this comment

inashivb Jan 16, 2024

Choose a reason for hiding this comment

victorjulien Jan 16, 2024

Choose a reason for hiding this comment

victorjulien May 16, 2024

Choose a reason for hiding this comment

inashivb Jan 16, 2024

Choose a reason for hiding this comment

inashivb commented Jan 16, 2024

victorjulien commented Jan 16, 2024 • edited Loading

inashivb commented Jan 16, 2024

suricata-qa commented Jan 16, 2024

catenacyber commented Jan 18, 2024

jufajardini commented Jan 18, 2024

catenacyber commented Apr 30, 2024

suricata-qa commented May 8, 2024

victorjulien commented Jun 6, 2024

victorjulien commented Jan 16, 2024 •

edited

Loading