[draft] flow/detect: skip packet rules if only tx needs to be inspected

[jasonish]

For discussion only. It appears a pseudo packet that exists only to flush out transaction handling on flow finish is also subject to packet rules, which lead to an alert that doesn't really make sense, matching the details of the first packet in the flow.

Instead, if a flush packet only exists to give the app-layer some love, flag it as such, and skip packet rules.

Only one S-V test fails, but on a quick look, it appears the missing alert after this change is probably correct.

Flags: needs confirmation that this alert shouldn't exist.

Ticket: https://redmine.openinfosecfoundation.org/issues/7318

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 79.05%. Comparing base (501f79c) to head (2995551). Report is 84 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11862      +/-   ##
==========================================
- Coverage   82.56%   79.05%   -3.51%     
==========================================
  Files         912      912              
  Lines      249354   249186     -168     
==========================================
- Hits       205880   197005    -8875     
- Misses      43474    52181    +8707     

Flag	Coverage Δ
fuzzcorpus	60.46% <100.00%> (+<0.01%)	:arrow_up:
livemode	18.88% <58.82%> (+0.15%)	:arrow_up:
pcap	44.15% <100.00%> (+0.06%)	:arrow_up:
suricata-verify	?
unittests	58.94% <58.82%> (+<0.01%)	:arrow_up:

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

[suricata-qa]

Information:

ERROR: QA failed on SURI_TLPW2_single_alerts_cmp.

ERROR: QA failed on SURI_TLPW2_autofp_alerts_cmp.

ERROR: QA failed on SURI_TLPR1_alerts_cmp.

field	baseline	test	%
	SURI_TLPR1_stats_chk
.uptime	649	628	96.76%

Pipeline 22960

[victorjulien]

I think d03660a646071a69ab6c377c3be202f9b2d292d8 is trying to solve something very similar. Is that not sufficient?

[jasonish]

I think d03660a is trying to solve something very similar. Is that not sufficient?

I'm not sure I follow. Not sufficient if we're here I guess. Or is this a non-issue and these alerts should be logged as they are?

[victorjulien]

I think d03660a is trying to solve something very similar. Is that not sufficient?

I'm not sure I follow. Not sufficient if we're here I guess. Or is this a non-issue and these alerts should be logged as they are?

I'm not yet sure I understand the scope of the issue. But in general, if certain alerts shouldn't be generated for pseudo packets, the signature should require "real packets". That is what that commit introduced as a concept. Thats why I thought it might be related.

[jasonish]

I think d03660a is trying to solve something very similar. Is that not sufficient?

I'm not sure I follow. Not sufficient if we're here I guess. Or is this a non-issue and these alerts should be logged as they are?

I'm not yet sure I understand the scope of the issue. But in general, if certain alerts shouldn't be generated for pseudo packets, the signature should require "real packets". That is what that commit introduced as a concept. Thats why I thought it might be related.

I think this is more about transactions, than signatures so I'm not sure if real packet or not matters here, we're just trying to see if any transactions will complete and run analysis on that only.

However, I think the real issue here might be that SSL is already reporting one active transaction, even if none, or complete.

[inashivb]

However, I think the real issue here might be that SSL is already reporting one active transaction, even if none, or complete.

That's exactly what I was suggesting too. :)

[victorjulien]

Is there a public test case yet?

IIRC the rules aren't about tx inspection, so I'm curious about how they are related.

[inashivb]

Is there a public test case yet?

Here: https://github.com/OISF/suricata-verify/pull/2080

[jasonish]

IIRC the rules aren't about tx inspection, so I'm curious about how they are related.

I guess I'm not considering the rules. I'm just considering that the psuedo-packet is only pushed through to let transactions cleanup - that's been my analysis anyways. The real fix could have to do with the rules, if a fix is even needed, or if this is expected and desired behavior, which is still to be resolved.

[jasonish]

Other fix in progress at https://github.com/OISF/suricata/pull/11999.