Project

General

Profile

Actions

Bug #5271

closed

app-layer: timeout when removing many transactions from the beginning

Added by Philippe Antoine over 2 years ago. Updated about 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

Found by oss-fuzz
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=44307

Attached pcap from fuzzing input + a bit of crafting gets 36k transactions and spends 36 seconds removing them, especially moving memory around the rust vector.


Files

dns.pcap (660 KB) dns.pcap Philippe Antoine, 04/16/2022 07:44 AM

Subtasks 7 (0 open7 closed)

Bug #5277: dns: More efficient transaction handlingClosedJason IshActions
Bug #5294: mqtt: convert to vecdequeClosedJason IshActions
Bug #5295: rdp: convert transaction list to vecdequeClosedJason IshActions
Bug #5296: http2: convert transaction list to vecdequeClosedJason IshActions
Bug #5297: pgsql: convert transaction list to vecdequeClosedJason IshActions
Bug #5298: template (rust): convert transaction list to vecdequeClosedJason IshActions
Bug #5321: dcerpc: More efficient transaction handlingClosedJason IshActions

Related issues 3 (0 open3 closed)

Related to Suricata - Bug #5278: app-layer: Allow for non slice based transaction containers in generate get iterator (rust)ClosedJason IshActions
Related to Suricata - Bug #5314: ftp: quadratic complexity for tx iterator with linked listClosedPhilippe AntoineActions
Related to Suricata - Bug #5637: quic: convert to vecdequeClosedPhilippe AntoineActions
Actions #1

Updated by Philippe Antoine over 2 years ago

Happens on many other protocols than DNS, DNS is just an example

Actions #2

Updated by Jason Ish over 2 years ago

The main issue here appears to be the use of a vector for transactions. Add new transactions is relatively cheap due to how vectors grow, but removing from anywhere other than the end results in all the memory being moved up, this is particularly bad when removing from the front. Take DNS, if you can get thousands of transactions added with one segment of input, due to Suricata transaction handling, they will be removed in order from the front. So thats multiple removals and multiple memmmoves.

The fix for parsers where transactions are normally removed in the same order as they are added, such as DNS, is a VecDeque which has efficient removals from the front. A VecDeque should also help with parsers that need to remove from the middle as its likely less memory will need to be moved due to a VecDeque internally being 2 vectors.

Actions #3

Updated by Jason Ish over 2 years ago

  • Related to Bug #5278: app-layer: Allow for non slice based transaction containers in generate get iterator (rust) added
Actions #4

Updated by Philippe Antoine over 2 years ago

  • Assignee changed from Philippe Antoine to Jason Ish

Jason, you seem to have already begun to work on this

List of protocols found by oss-fuzz :
mqtt, http2, dcerpc, rdp, pgsql...

Actions #5

Updated by Jason Ish over 2 years ago

Philippe Antoine wrote in #note-4:

Jason, you seem to have already begun to work on this

List of protocols found by oss-fuzz :
mqtt, http2, dcerpc, rdp, pgsql...

Started here: https://github.com/OISF/suricata/pull/7313

Its starts with a change to the trait to allow for VecDeque as a transaction store. Should be trivial to convert affected protocols, DNS was only a few lines change with the update to the trait.

Actions #6

Updated by Jason Ish over 2 years ago

List of protocols found by oss-fuzz :
mqtt, http2, dcerpc, rdp, pgsql...

Any others? I'm actually thinking now that a VecDeque is a better collection that a vec for all parsers and should just become the default.

Actions #7

Updated by Philippe Antoine over 2 years ago

That is the exhaustive list as of today...
But I am ok with VecDeque being the default :-)

Actions #8

Updated by Philippe Antoine over 2 years ago

  • Related to Bug #5314: ftp: quadratic complexity for tx iterator with linked list added
Actions #9

Updated by Philippe Antoine over 2 years ago

  • Status changed from New to Closed

All is done here

Actions #10

Updated by Philippe Antoine almost 2 years ago

  • Related to Bug #5637: quic: convert to vecdeque added
Actions #11

Updated by Victor Julien about 1 year ago

  • Private changed from Yes to No
Actions

Also available in: Atom PDF