Bug #5271
closedapp-layer: timeout when removing many transactions from the beginning
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
Updated by Philippe Antoine over 2 years ago
Happens on many other protocols than DNS, DNS is just an example
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.
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
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...
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.
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.
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 :-)
Updated by Philippe Antoine over 2 years ago
- Related to Bug #5314: ftp: quadratic complexity for tx iterator with linked list added
Updated by Philippe Antoine about 2 years ago
- Related to Bug #5637: quic: convert to vecdeque added