Backports/708/v2 by catenacyber · Pull Request #12265 · OISF/suricata

catenacyber · 2024-12-11T14:00:03Z

Link to ticket: https://redmine.openinfosecfoundation.org/issues/
https://redmine.openinfosecfoundation.org/issues/7407
https://redmine.openinfosecfoundation.org/issues/7368

Describe changes:

Backport of detect: don't run pkt sigs on ffr pkts #12258 aka detect: don't run pkt sigs on ffr pkts #12264 (but needed for SV)
Backport of Detect log alert tx one 7199 v8 #12255 as in Backports/708/v1 #12263 but with a different additional commit for the DNS case

SV_BRANCH=OISF/suricata-verify#2179

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: OISF#7318. (cherry picked from commit 0e4faba)

Ticket: 7199 Uses a config parameter detect.guess-applayer-tx to enable this behavior (off by default) This feature is requested for use cases with signatures not using app-layer keywords but still targetting application layer transactions, such as pass/drop rule combination, or lua usage. This overrides the previous behavior of checking if the signature has a content match, by checking if there is only one live transaction, in addition to the config parameter being set. (cherry picked from commit f2c3776)

So we get: 1. request arrives - buffered due to not ackd 2. response arrives, acks request - request is now parsed, response isn't 3. ack for response, response parsed. Then detect runs for request, generates alert. We now have 2 txs. txid will be 0 from AppLayerParserGetTransactionInspectId But txid 1 is unidirectional in the other way, so we can use txid 0 metadata for logging Ticket: 7199

codecov · 2024-12-11T14:19:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.19%. Comparing base (9be2eca) to head (59d6881).

Additional details and impacted files

@@              Coverage Diff               @@
##           main-7.0.x   #12265      +/-   ##
==============================================
- Coverage       83.19%   83.19%   -0.01%     
==============================================
  Files             922      922              
  Lines          260888   260910      +22     
==============================================
+ Hits           217048   217061      +13     
- Misses          43840    43849       +9

Flag	Coverage Δ
fuzzcorpus	`64.17% <42.85%> (-0.02%)`	⬇️
suricata-verify	`63.40% <100.00%> (+0.01%)`	⬆️
unittests	`62.38% <28.57%> (+<0.01%)`	⬆️

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

jufajardini · 2024-12-11T14:45:14Z

src/detect.c

+            if (txd &&
+                    (AppLayerParserGetTxDetectFlags(txd, dir) & APP_LAYER_TX_SKIP_INSPECT_FLAG)) {
+                return true;
+            }


if we don't get a tx here, shouldn't we at least add a debug message (as it seems quite weird that we'd have the tx_cnt, but no tx returned)?

Is it weird? Or would it be the same if we have more than 2 or more transactions open in flight for a non-unidirectional protocol, in which case we don't log any of them either, but we'd still have a tx_cnt.

@jufajardini did you mean tx or txd ?

For txd, this should be impossible indeed, but it is the way to write code to make code analysis tools happy...

For tx, I think it can happen with HTTP2 : you create 2 transactions, but the second one finishes first.
tx_id = 0, tx_cnt = 2, but tx_id = 1 has been freed and is now NULL

Yes, it was for tx. Ok, forgive my noise.

jufajardini

From what APP_LAYER_TX_SKIP_INSPECT_FLAG is for and the case this is tackling, and the tests ran, this seems to make sense to me.

I feel that, as with the firewall scenario, we need way more tests for things like stream rules...

jufajardini · 2024-12-11T15:12:41Z

Considering the purpose of the ticket and discussions around this, I think that this patch is more in line with what we are trying to do than adding the check for a stream match.

jasonish · 2024-12-11T15:17:44Z

src/detect.c

 #endif
 }

+static bool isOnlyTxInDirection(Flow *f, uint64_t txid, uint8_t dir)


nit: IsOnly...

jasonish · 2024-12-11T15:47:47Z

Logic does seem OK? I wonder if it could be wrong in some cases? Does APP_LAYER_TX_SKIP_INSPECT_FLAG have any other uses? But its still a guess, but I do think this makes it a slightly better guess for DNS.

jasonish · 2024-12-11T18:19:35Z

How critical is it that we address this now for these unidirectional protocols? This whole approach is best effort, and perhaps doesn't have to hit some edge cases this yet.

victorjulien · 2024-12-11T18:36:58Z

How critical is it that we address this now for these unidirectional protocols? This whole approach is best effort, and perhaps doesn't have to hit some edge cases this yet.

I would like us to take a bit more time to address this in master first, and then backport something. Meanwhile we can use #12263 to keep the old behavior for the stream match edge case.

jasonish · 2024-12-11T18:45:35Z

How critical is it that we address this now for these unidirectional protocols? This whole approach is best effort, and perhaps doesn't have to hit some edge cases this yet.

I would like us to take a bit more time to address this in master first, and then backport something. Meanwhile we can use #12263 to keep the old behavior for the stream match edge case.

Sounds reasonable to me.

catenacyber · 2024-12-11T20:18:16Z

So, closing this PR waiting for #12266 in master, and in favor of #12263 + #12264

victorjulien and others added 3 commits December 11, 2024 14:57

catenacyber requested review from a team, jufajardini and victorjulien as code owners December 11, 2024 14:00

jufajardini reviewed Dec 11, 2024

View reviewed changes

jasonish reviewed Dec 11, 2024

View reviewed changes

src/detect.c

#endif

}

static bool isOnlyTxInDirection(Flow *f, uint64_t txid, uint8_t dir)

Copy link

Member

jasonish Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IsOnly...

catenacyber closed this Dec 11, 2024

Conversation

catenacyber commented Dec 11, 2024

Uh oh!

codecov bot commented Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jufajardini Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

jasonish Dec 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

catenacyber Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

catenacyber Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

jufajardini Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

jufajardini left a comment

Choose a reason for hiding this comment

Uh oh!

jufajardini commented Dec 11, 2024

Uh oh!

jasonish Dec 11, 2024

Choose a reason for hiding this comment

Uh oh!

jasonish commented Dec 11, 2024

Uh oh!

jasonish commented Dec 11, 2024

Uh oh!

victorjulien commented Dec 11, 2024

Uh oh!

jasonish commented Dec 11, 2024

Uh oh!

catenacyber commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

codecov bot commented Dec 11, 2024 •

edited

Loading

jasonish Dec 11, 2024 •

edited

Loading