Tls alert/v1

[victorjulien]

Some draft commits to help with FFR packets in IPS mode triggering pkt inspection.

Some non-organized thoughts

Flow Timeout packet matching
============================

Flow packets are created because:

1. flow has ended; and
2. inspect_id[0] == 0; and
3. total_txs == 1; so pseudo packet created

Pseudo packet evaluates simple rule, which matches:

alert tcp any any -> any 443 (flow: to_server; flowbits:isset, tls_error; sid:09901033; rev:1; msg:"Allow TLS error handling (outgoing packet)"; )

Rule has type "pkt", as it matches on flow, flowbits. However, it does not have flag `REAL_PKT` as both flow and flowbits can be mixed with app-layer.


Couple of ideas:

1. workaround could be to skip `s->type:"pkt"` and `p->flags:PKT_PSEUDO_STREAM_END`. Appears to work.
   
   a. this is more fine grained than should be necessary. Ideally, skip `DetectRunPrefilterPkt`
      and `DetectRulePacketRules` -- however, not yet possible as `app-layer-event` matching
      (and possibly more) still depends on it somehow (sigs added to `det_ctx->match_array`).
      i. move app-layer-event rules into specialized engine (ticket)
      ii. review other such cases (`app-layer-protocol`?)
      iii. skip packet detect for these packets

2. can we avoid the pseudo packets from being generated?

   Or better question: why are they being generated?

   No app-layer sigs, but `AppLayerParserSetTransactionInspectId` is still called
   
   `AppLayerParserSetTransactionInspectId` called w/o knowledge of no sigs inspecting the state.
   
   `AppLayerParserSetTransactionInspectId` also not considering flow complete until the pseudo packets.
   `STREAM_EOF` flag has no effect. Only depth/gap. Should EOF also lead to tx complete?

[suricata-qa]

ERROR:

ERROR: QA failed on build_asan.

Pipeline 23140

[codecov]

Codecov Report

Attention: Patch coverage is 77.77778% with 4 lines in your changes missing coverage. Please review.

Project coverage is 83.23%. Comparing base (55b922c) to head (dca9527).
Report is 504 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11999      +/-   ##
==========================================
+ Coverage   82.75%   83.23%   +0.47%     
==========================================
  Files         910      910              
  Lines      249016   258197    +9181     
==========================================
+ Hits       206069   214904    +8835     
- Misses      42947    43293     +346     

Flag	Coverage Δ
fuzzcorpus	61.39% <77.77%> (+0.58%)	⬆️
livemode	19.42% <5.55%> (+0.71%)	⬆️
pcap	44.42% <55.55%> (+0.29%)	⬆️
suricata-verify	62.79% <61.11%> (+0.50%)	⬆️
unittests	59.27% <11.11%> (+0.26%)	⬆️

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

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jasonish]

There is nothing specific to IPS here is there? I think the issue at hand is present in both.

Commit 236d3e9 does appear to address the issue on its own:

• its more narrow focused than the REAL flag/mask
• it prevents the packet rules from running on the pseudo packet (how I see the root of the issue)

So are the other 2 commits out of scope for the immediate fix. The new event could be interesting to have though.

[jasonish]

Or better question: why are they being generated?

In this case its to "poke" the app-layer. For this specific bug its not needed. However, there are many S-V tests that appear to depend on this "poke", even if there is no new data. Trying to be smart and not generated when not needed might be too involved from a backport perspective.

[suricata-qa]

Information: QA ran without warnings.

Pipeline 23143

[inashivb]

Thank you for the detailed explanation and analysis! 🙇🏽‍♀️

1. workaround could be to skip s->type:"pkt" and p->flags:PKT_PSEUDO_STREAM_END. Appears to work.

I like this as it clearly expresses the intent of skipping the signature + pkt.

a. this is more fine grained than should be necessary. Ideally, skip DetectRunPrefilterPkt
and DetectRulePacketRules -- however, not yet possible as app-layer-event matching
(and possibly more) still depends on it somehow (sigs added to det_ctx->match_array).

The match array count seems to be the same in either cases. (Case 1: your solution; Case 2: Skipping over the said Detect* fns leading to existing test failures). But, got more to look in this area. Don't have a conclusive thought on this either..

  i. move app-layer-event rules into specialized engine (ticket)
  ii. review other such cases (`app-layer-protocol`?)
  iii. skip packet detect for these packets

2. can we avoid the pseudo packets from being generated?

   Or better question: why are they being generated?

   No app-layer sigs, but AppLayerParserSetTransactionInspectId is still called

   AppLayerParserSetTransactionInspectId called w/o knowledge of no sigs inspecting the state.

   AppLayerParserSetTransactionInspectId also not considering flow complete until the pseudo packets.
   STREAM_EOF flag has no effect. Only depth/gap. Should EOF also lead to tx complete?

It does make logical sense to ask the applayer protocol for status on EOF.

[catenacyber]

I think this should be closed after the merge of #12258

[victorjulien]

Think the tls "alert record" parsing should still be rebased and resubmitted, so keeping it open for now.

[victorjulien]

Replaced by #12764