Bug #4171
closedFailed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL
Added by Philippe Antoine almost 4 years ago. Updated over 3 years ago.
Files
| debug2.pcap (1.63 KB) debug2.pcap | Philippe Antoine, 11/21/2020 02:55 PM | ||
| out3.pcap (1.06 KB) out3.pcap | Philippe Antoine, 12/04/2020 08:48 AM |
Updated by Philippe Antoine almost 4 years ago
- File debug2.pcap debug2.pcap added
If I get it right, the scenario is :
- normal tcp handshake
- client sends one byte of data
- server sends an ack, TCPProtoDetect runs on the byte and does not conclude
- client sends some TCP payload with sequence way after the first one segs_right_edge gets updated
- server sends another ack, TCPProtoDetect triggers the assert failure
Not sure what the way to go is :
- remove these DEBUG_VALIDATE_BUG_ON ?
- better computation of size_ts to get the size without the gaps
- something else ?
Reproducer with --set stream.reassembly.depth=0 is attached
Updated by Victor Julien almost 4 years ago
The goal of this check has been to catch cases where we'd queue too much data on one side waiting for data on the other side. While proto detect is inconclusive we won't clear data from the stream engine. The most extreme case in the beginning was the ftp-data channel, which has no data to the server so toserver proto detect wouldn't run and all toclient data was queued in the stream engine. This lead the 'bail conditions checks'.
I suppose that as long as the data is within the TCP window, but incomplete because of non-final gaps, we shouldn't trigger this check. We might still receive data to fill the gaps. If the gaps are final, meaning that ACK has moved beyond it then the logic should probably be updated.
Updated by Victor Julien almost 4 years ago
- Target version changed from 6.0.1 to 6.0.2
Updated by Philippe Antoine almost 4 years ago
So, would this mean using last_ack instead of segs_right_edge to compute size_ts ?
And take care of the case where we only have one side of the traffic, right ?
I guess I will ned to craft some pcap...
Updated by Philippe Antoine almost 4 years ago
Here is a crafted pcap to reproduce the issues
I run./src/suricata -r out3.pcap --set stream.reassembly.depth=0 -c suricata.yaml -k none
- python script from http-connect S-V test
- Mixed packets order with editcap and mergecap (1-6,10,9,7-8)
- Manually crafted to increase the TCP option window scale to 7 (128) on both sides
- Manually crafted to increase the sequence number of now packet 7 (second packet with tcp payload) adding 0x100000 (as much needed to trigger
DEBUG_VALIDATE_BUG_ON(size_ts > 1000000UL);)
Discussion about the 2 issues and the fixes are in the (draft) Gitlab MR
But there may be another problem :
What happens if I reorder packets this way ?
Let's say I normally have 4 packets : req1, resp1, req2, resp2
What happens if they are in this order resp2, req2, resp1, req1 ?
We can test this with SMB open file (first request), and write (second request) to the file identifier returned by first response
Updated by Philippe Antoine almost 4 years ago
By the way, fuzz_sigpcap does not make reproducible results, because we do not reset flows at every input.
Is there an easy way to do that ?
Updated by Victor Julien over 3 years ago
- Target version changed from 6.0.2 to 7.0.0-beta1
- Label Needs backport added
Updated by Philippe Antoine over 3 years ago
- Label Needs backport to 5.0, Needs backport to 6.0 added
Updated by Jeff Lucovsky over 3 years ago
- Copied to Bug #4345: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL added
Updated by Jeff Lucovsky over 3 years ago
- Copied to Bug #4346: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL added
Updated by Philippe Antoine over 3 years ago
- Private changed from Yes to No
Updated by Philippe Antoine over 3 years ago
- Blocks Bug #4273: protodetect: SEGV due to NULL ptr deref added
Updated by Philippe Antoine over 3 years ago
- Status changed from In Review to Closed