Project

General

Profile

Actions

Security #6299

closed

mqtt pcap with anomalies takes too long to process because of app-layer-event detection

Added by Philippe Antoine 8 months ago. Updated 2 months ago.

Status:
Closed
Priority:
Normal
Target version:
Affected Versions:
Label:
CVE:
Git IDs:

9240ae250cc369306803740279df2ab3eca6b54a
5bb8800588e7b4a09e1770f049cd88be71e2d30b
2fb50598f23b112f14ec15330e11c40b74caa35f
89936b6530690c6d03869b2ad8b82f9f84776f94

Severity:
CRITICAL
Disclosure Date:
12/25/2023

Description

Cf time ./src/suricata -r .lol.pcap -c suricata.yaml -S rules/mqtt-events.rules -l log -k none

Analysis :
- About 1000 MQTT transactions are created and not completed from client to server (with an anomaly unintroduced message)
- Server sends many packets with a 2 bytes of payload
- Client finally ACKS these packets, which create many transactions, with anomalies, and also triggering the MQTT transactions flush mechanism

Some possible solutions ?
- Decreasing yaml mqtt.max-tx to 64 decreases the time from one minute to 6 seconds
- DetectRunTx should not inspect the transactions that have not been modified by the current packet

Victor, thoughts about this ?


Files

lol.pcap (1.43 MB) lol.pcap Philippe Antoine, 09/07/2023 08:10 AM
httpslow.pcap (1.35 MB) httpslow.pcap Philippe Antoine, 09/18/2023 11:26 AM

Subtasks 1 (0 open1 closed)

Security #6539: mqtt pcap with anomalies takes too long to process (7.0.x backport)ClosedPhilippe AntoineActions

Related issues 3 (2 open1 closed)

Related to Suricata - Security #5921: http1: configurable limit for maximum number of live transactions per flowClosedPhilippe AntoineActions
Related to Suricata - Optimization #4749: app-layer: track changed txs for detect and loggingNewActions
Related to Suricata - Optimization #6728: detect: prefilter for app-layer-eventNewPhilippe AntoineActions
Actions #1

Updated by Philippe Antoine 8 months ago

Could keyword app-layer-event support prefilter ?

Actions #2

Updated by Philippe Antoine 8 months ago

  • Status changed from New to In Review
  • Target version changed from 7.0.2 to 7.0.1
Actions #3

Updated by Philippe Antoine 8 months ago

Found by QuadFuzz for reference

Actions #4

Updated by Victor Julien 8 months ago

  • Assignee changed from Victor Julien to Philippe Antoine
Actions #5

Updated by Victor Julien 8 months ago

  • Target version changed from 7.0.1 to 7.0.2
Actions #6

Updated by Philippe Antoine 7 months ago

Looks like https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=62416 is another variant found by oss-fuzz,
No MQTT, but HTTP, still using app-layer-event. keyword in signatures and spending much time qsort in DetectRunTx

Actions #7

Updated by Philippe Antoine 7 months ago

  • Related to Security #5921: http1: configurable limit for maximum number of live transactions per flow added
Actions #8

Updated by Philippe Antoine 7 months ago

So, diagnostic is
- create and make live many transaction for one flow (there is no limit for HTTP1 per #5921)
- use multiple rules with app-layer-event keyword (may work with others) but there is no pre filtering beyond the app-layer protocol

So, for each packet, we run DetectRunTx which runs for each transaction
As there is no prefilter, we merge 'state' rules from the regular prefilter
Then, we merge stored state into results, which is a duplicate of the signatures with the flags set (pointer set integer 0)

Problems :
- no limit or too big for the number of transaction per live flow
- we use qsort instead of merging two sorted lists when we merge 'state' rules from the regular prefilter (when quicksort may be at its quadratic worst-case implementation as the list is about already sorted)
- we store a state with flags set to 0 for these app-layer events, leading to duplicate signatures`

@Victor Julien I wonder what you think about this

Actions #9

Updated by Philippe Antoine 7 months ago

oss-fuzz reproducer with time ./src/suricata -r httpslow.pcap -c suricata.yaml -k none --set runmode=single -S rules/http-events.rules

Actions #10

Updated by Philippe Antoine 7 months ago

Summing up the subtasks :
- merge sorted lists instead of using qsort
- do not store state with flags set to 0
- do not run detection if nothing has been updated (need to investigate the QA diff for SMTP halved transactions)
- MQTT should only raise event UnintroducedMessage once per flow, and maybe not if it was picked up midstream
- MQTT should reuse an exiting transaction for this event instead of creating a new empty one just for this purpose
- limit the number of live HTTP1 transactions tracked in #5921

Actions #11

Updated by Victor Julien 7 months ago

  • Target version changed from 7.0.2 to 7.0.3
Actions #12

Updated by Philippe Antoine 6 months ago

  • Related to Optimization #4749: app-layer: track changed txs for detect and logging added
Actions #13

Updated by Victor Julien 5 months ago

  • Target version changed from 7.0.3 to 8.0.0-beta1
Actions #14

Updated by Victor Julien 5 months ago

  • Label Needs backport to 7.0 added
Actions #15

Updated by OISF Ticketbot 5 months ago

  • Subtask #6539 added
Actions #16

Updated by OISF Ticketbot 5 months ago

  • Label deleted (Needs backport to 7.0)
Actions #17

Updated by Philippe Antoine 4 months ago

  • Disclosure Date set to 12/25/2023
Actions #18

Updated by Philippe Antoine 4 months ago

  • Subject changed from mqtt pcap with anomalies takes too long to process to mqtt pcap with anomalies takes too long to process because of app-layer-event detection
Actions #19

Updated by Victor Julien 4 months ago

  • Severity changed from MODERATE to CRITICAL

Severe slowdown, so CRITICAL.

Actions #21

Updated by Philippe Antoine 3 months ago

After the merge of https://github.com/OISF/suricata/pull/10160, I wonder if there is one more subticket to do :

Is there a way that app-layer-event keyword is evaluated only once per transaction ? And not 50 times if there are 50 rules for 50 different app-layer events...
I wonder especially in the simple case when there is no event. Could we have a prefilter-ish mechanism saying that none of these signatures will match on the transaction in its current state ?

Actions #22

Updated by Philippe Antoine 3 months ago

Actions #23

Updated by Philippe Antoine 3 months ago

  • Status changed from In Review to Resolved
Actions #24

Updated by Philippe Antoine 3 months ago

  • Status changed from Resolved to Closed
  • Git IDs updated (diff)
Actions #25

Updated by Victor Julien 2 months ago

  • Private changed from Yes to No
Actions

Also available in: Atom PDF