Project

General

Profile

Actions

Bug #4171

closed

Failed assert in TCPProtoDetectCheckBailConditions size_ts > 1000000UL

Added by Philippe Antoine 10 months ago. Updated about 2 months 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

Blocks Bug #4273: protodetect: SEGV due to NULL ptr derefIn ReviewPhilippe AntoineActions
Actions #1

Updated by Philippe Antoine 10 months 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 10 months 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 10 months ago

  • Target version changed from 6.0.1 to 6.0.2
Actions #4

Updated by Philippe Antoine 10 months 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 10 months 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 9 months ago

  • Assignee set to Philippe Antoine
Actions #7

Updated by Philippe Antoine 8 months 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 7 months ago

  • Target version changed from 6.0.2 to 7.0rc1
  • Label Needs backport added
Actions #9

Updated by Philippe Antoine 7 months ago

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

Updated by Philippe Antoine 5 months ago

  • Private changed from Yes to No
Actions #13

Updated by Philippe Antoine 5 months ago

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

Updated by Philippe Antoine about 2 months ago

  • Status changed from In Review to Closed
Actions

Also available in: Atom PDF