Project

General

Profile

Actions

Bug #4171

closed

Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL

Added by Philippe Antoine over 3 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:
Needs backport, Needs backport to 5.0, Needs backport to 6.0


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

Related issues 3 (0 open3 closed)

Blocks Suricata - Bug #4273: protodetect: SEGV due to NULL ptr derefClosedPhilippe AntoineActions
Copied to Suricata - Bug #4345: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000ULClosedJeff LucovskyActions
Copied to Suricata - Bug #4346: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000ULClosedShivani BhardwajActions
Actions #1

Updated by Philippe Antoine over 3 years ago

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 #2

Updated by Victor Julien over 3 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 #3

Updated by Victor Julien over 3 years ago

  • Target version changed from 6.0.1 to 6.0.2
Actions #4

Updated by Philippe Antoine over 3 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 #5

Updated by Philippe Antoine over 3 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

This pcap was produced with
  1. python script from http-connect S-V test
  2. Mixed packets order with editcap and mergecap (1-6,10,9,7-8)
  3. Manually crafted to increase the TCP option window scale to 7 (128) on both sides
  4. 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 #6

Updated by Philippe Antoine over 3 years ago

  • Assignee set to Philippe Antoine
Actions #7

Updated by Philippe Antoine about 3 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 ?

Actions #8

Updated by Victor Julien about 3 years ago

  • Target version changed from 6.0.2 to 7.0.0-beta1
  • Label Needs backport added
Actions #9

Updated by Philippe Antoine about 3 years ago

  • Label Needs backport to 5.0, Needs backport to 6.0 added
Actions #10

Updated by Jeff Lucovsky about 3 years ago

  • Copied to Bug #4345: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL added
Actions #11

Updated by Jeff Lucovsky about 3 years ago

  • Copied to Bug #4346: Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL added
Actions #12

Updated by Philippe Antoine almost 3 years ago

  • Private changed from Yes to No
Actions #13

Updated by Philippe Antoine almost 3 years ago

  • Blocks Bug #4273: protodetect: SEGV due to NULL ptr deref added
Actions #14

Updated by Philippe Antoine over 2 years ago

  • Status changed from In Review to Closed
Actions

Also available in: Atom PDF