Bug #4171: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL - Suricata - Open Information Security Foundation

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

Actions

Copy link

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.

Actions

Copy link

Updated by Victor Julien almost 4 years ago

Target version changed from 6.0.1 to 6.0.2

Actions

Copy link

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...

Actions

Copy link

Updated by Philippe Antoine almost 4 years ago

File out3.pcap out3.pcap added
Status changed from New to In Review

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

This pcap was produced with

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

Actions

Copy link