detect: don't run pkt sigs on ffr pkts

[victorjulien]

Last packet from the TLS TCP session moves TCP state to CLOSED.

This flags the app-layer with APP_LAYER_PARSER_EOF_TS or APP_LAYER_PARSER_EOF_TC depending on the direction of the final packet. This flag will just have been set in a single direction.

This leads to the last packet updating the inspect id in that packets direction.

At the end of the TLS session a pseudo packet is created, because:

• flow has ended
• inspected tx id == 0, for at least one direction
• total txs is 1

Then a packet rule 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)"; )

The SIG_MASK_REQUIRE_REAL_PKT is not preventing the match, as the flowbits keyword doesn't set it.

To avoid this match. This patch skips signatures of the SIG_TYPE_PKT for flow end packets.

Ticket: #7318.

SV_BRANCH=OISF/suricata-verify#2121

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.25%. Comparing base (dd71ef0) to head (8d0fa24).
Report is 78 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12095      +/-   ##
==========================================
- Coverage   83.25%   83.25%   -0.01%     
==========================================
  Files         910      906       -4     
  Lines      257571   257649      +78     
==========================================
+ Hits       214450   214501      +51     
- Misses      43121    43148      +27     

Flag	Coverage Δ
fuzzcorpus	61.24% <100.00%> (+0.07%)	⬆️
livemode	19.42% <50.00%> (+0.01%)	⬆️
pcap	44.37% <50.00%> (-0.10%)	⬇️
suricata-verify	62.74% <100.00%> (-0.05%)	⬇️
unittests	59.28% <50.00%> (-0.07%)	⬇️

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

[suricata-qa]

Information: QA ran without warnings.

Pipeline 23284

[catenacyber]

@jufajardini can you remind me what SIG_TYPE_PKT are ?
Are we here with the default case in SignatureSetType ? (because signature uses flowbits only)
Are there other cases besides flowbits (and flowints I suppose) where we get s->type == SIG_TYPE_PKT without SIG_MASK_REQUIRE_REAL_PKT ?

[catenacyber]

Should it work the same for SIG_TYPE_APPLAYER ?
That is

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

using tls in rule's alproto instead of tcp

[victorjulien]

Should it work the same for SIG_TYPE_APPLAYER ? That is

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

using tls in rule's alproto instead of tcp

That won't lead to a SIG_TYPE_APPLAYER rule. IIRC currently the only rule that would lead to SIG_TYPE_APPLAYER is something like: alert tls any any -> any any (sid:1;). Any inspect engine or general match condition will make it a different type.

A SIG_TYPE_APPLAYER does act mostly like a SIG_TYPE_PKT rule, with the exception that it applies the rule action to the flow instead of just the packet.

There are several SV tests that fail if I include SIG_TYPE_APPLAYER in the check here, so that would mean another behavior change.

[inashivb]

The patch makes sense to me but,
I still do not understand why is it ok at this point to have TLS parser tell total txs == 1, when there are no txs and the flow has ended.. 🥲

[jasonish]

The patch makes sense to me but, I still do not understand why is it ok at this point to have TLS parser tell total txs == 1, when there are no txs and the flow has ended.. 🥲

With the PCAP from the initial report, the handshake was not complete. So it actually makes sense that the 1, and only 1 transaction on TLS would still be considered active on flow close.. However, I think the check could be made smarter to check the state of the transaction before always returning 1. However, in the specific PCAP the behaviour is correct I think.

[inashivb]

The patch makes sense to me but, I still do not understand why is it ok at this point to have TLS parser tell total txs == 1, when there are no txs and the flow has ended.. 🥲

With the PCAP from the initial report, the handshake was not complete. So it actually makes sense that the 1, and only 1 transaction on TLS would still be considered active on flow close.. However, I think the check could be made smarter to check the state of the transaction before always returning 1. However, in the specific PCAP the behaviour is correct I think.

I had totally missed the handshake failure. Thanks for explaining! 🙇🏽‍♀️
+1 to making the state of transaction reported by TLS parser smarter.

[catenacyber]

Thanks for the work, almost good except SV test

CI : ✅
Code : good
Commits segmentation : ok
Commit messages : good
Git ID set : looks fine for me
CLA : you already contributed
Doc update : @jufajardini should there be a doc update about signature type packet not matching on pseudo packets ?
Redmine ticket : ok
Rustfmt : no rust
Tests : SV PR is failing for 7 🔴
Dependencies added: none

@jufajardini where can we find your presentation about signature types ?

[jufajardini]

Thanks for the work, almost good except SV test

CI : ✅ Code : good Commits segmentation : ok Commit messages : good Git ID set : looks fine for me CLA : you already contributed Doc update : @jufajardini should there be a doc update about signature type packet not matching on pseudo packets ? Redmine ticket : ok Rustfmt : no rust Tests : SV PR is failing for 7 🔴 Dependencies added: none

@jufajardini where can we find your presentation about signature types ?

Working on a new version of the PR atm. I'll take into account your comments here to try to add more examples. Current it this: #12114

I'll share a pdf with the presentation with you, as I think we haven't published those yet.

[jufajardini]

Doc update : @jufajardini should there be a doc update about signature type packet not matching on pseudo packets ?

I'll add something about this to the new docs.

[catenacyber]

Thanks Juliana

Why is a rule with flowbits a packet type ?
Because it is the default type if we have a keyword (not IP only) and it is not app-layer transaction ?

[victorjulien]

Thanks Juliana

Why is a rule with flowbits a packet type ? Because it is the default type if we have a keyword (not IP only) and it is not app-layer transaction ?

Flowbits can change per packet, at least in theory. The flowbits analysis isn't sophisticated enough to see if this is true.

[victorjulien]

#12169 plus OISF/suricata-verify#2148 shows what it could look like with SIG_TYPE_APPLAYER included.

[jufajardini]

Should it work the same for SIG_TYPE_APPLAYER ? That is

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

using tls in rule's alproto instead of tcp

from my checks, a rule like the above is SIG_TYPE_PKT. The flowbits or flowint commands that won't change a rule from SIG_TYPE_APPLAYER to SIG_TYPE_PKT are for setting/ unsettling and toggling, when applicable. I'm adding a section about this to the rule docs. And add what Victor explained here, to said section.

[jufajardini]

@jufajardini can you remind me what SIG_TYPE_PKT are ? Are we here with the default case in SignatureSetType ? (because signature uses flowbits only) Are there other cases besides flowbits (and flowints I suppose) where we get s->type == SIG_TYPE_PKT without SIG_MASK_REQUIRE_REAL_PKT ?

The Packet type signature is one that requires inspection on a packet level info, for instance, the packet header.

Checking the output of --engine-analysis, I see that many pkt rules do require a real packet, but not all of them. And I saw at least one ip_only rule requiring a real_pkt. I'll fish examples from the SV test associated with this PR to see what are the results there.

[catenacyber]

Rebased in #12257 with good SV test