Project

General

Profile

Actions
Copy link

Bug #7636

closed
PA SB

tcp: assertion triggered in StreamTcpReassembleAppLayer

Bug #7636: tcp: assertion triggered in StreamTcpReassembleAppLayer

Added by Philippe Antoine about 1 year ago. Updated 6 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Shivani Bhardwaj
Target version:
8.0.1
Affected Versions:
8.0.0
Effort:
Difficulty:
Label:

Description

Found by oss-fuzz:
https://issues.oss-fuzz.com/u/1/issues/407269999

fuzz_predefpcap_aware: stream-tcp-reassemble.c:1425: int StreamTcpReassembleAppLayer(ThreadVars *, TcpReassemblyThreadCtx *, TcpSession *, TcpStream *, Packet *, enum StreamUpdateDir): Assertion `!(((stream_flags & 2) == 0))' failed.

Regression range is https://github.com/OISF/suricata/compare/cd69955d7fca991d0ffc615f6ee7dce9dd20a3c5...834378ff887b3d6ac1903efb7a3e7164f593abd0

Reproducer is
./src/suricata -c fuzz.yaml -k none -r lol2.pcap --disable-detection -l log --runmode=single

but this does not trigger on my other device...

Files

lol2.pcap (283 Bytes) lol2.pcap Philippe Antoine, 03/31/2025 01:51 PM

VJ Updated by Victor Julien about 1 year ago Actions
Copy link
 #1

  • Target version changed from 8.0.0-beta1 to 8.0.0-rc1

PA Updated by Philippe Antoine 10 months ago Actions
Copy link
 #2

  • Priority changed from Normal to High

Victor, how bad is this assertion ?

VJ Updated by Victor Julien 10 months ago Actions
Copy link
 #3

  • Target version changed from 8.0.0-rc1 to 9.0.0-beta1

PA Updated by Philippe Antoine 10 months ago Actions
Copy link
 #4

I do not think this bug is bad.

I think it only happens in midstream with fin+syn packets after some first midstream packet

Impact would be some structures kept in memory a bit longer than needed...

PA Updated by Philippe Antoine 9 months ago Actions
Copy link
 #5

  • Affected Versions 8.0.0 added

PA Updated by Philippe Antoine 8 months ago Actions
Copy link
 #6

@Shivani Bhardwaj you added the assertion in https://github.com/OISF/suricata/commit/d096b989c813dcc71154a9080cadaee927e45d24

It is not true if ssn->state TCP_CLOSING

What should we do ?
- remove the assertion
- use stream_flags | STREAM_EOF in AppLayerHandleTCPData
- have StreamGetAppLayerFlags consider ssn->state >= TCP_CLOSING. instead of ssn->state TCP_CLOSED

SB Updated by Shivani Bhardwaj 8 months ago Actions
Copy link
 #7

Philippe Antoine wrote in #note-6:

@Shivani Bhardwaj you added the assertion in https://github.com/OISF/suricata/commit/d096b989c813dcc71154a9080cadaee927e45d24

It is not true if ssn->state TCP_CLOSING

Thank you, Philippe! Indeed your analysis is correct.

What should we do ?
- remove the assertion
- use stream_flags | STREAM_EOF in AppLayerHandleTCPData
- have StreamGetAppLayerFlags consider ssn->state >= TCP_CLOSING. instead of ssn->state TCP_CLOSED

TCP_CLOSING seems to not be treated as EOF ref: in StreamTcpPacketSetState, flow's state is updated to "established" if TCP state was "closing" so perhaps removal of assertion is the best course of action. Lmk wdyt?

PA Updated by Philippe Antoine 8 months ago Actions
Copy link
 #8

  • Target version changed from 9.0.0-beta1 to 8.0.1

Ok, should you the PR, or should I ?

SB Updated by Shivani Bhardwaj 8 months ago Actions
Copy link
 #9

Philippe Antoine wrote in #note-8:

Ok, should you the PR, or should I ?

Please check: https://github.com/OISF/suricata/pull/13762

PA Updated by Philippe Antoine 8 months ago Actions
Copy link
 #10

  • Assignee changed from Victor Julien to Shivani Bhardwaj

PA Updated by Philippe Antoine 8 months ago Actions
Copy link
 #11

  • Status changed from New to In Review

PA Updated by Philippe Antoine 7 months ago Actions
Copy link
 #12

  • Status changed from In Review to Closed

VJ Updated by Victor Julien 6 months ago Actions
Copy link
 #13

  • Priority changed from High to Normal

VJ Updated by Victor Julien 6 months ago Actions
Copy link
 #14

  • Private changed from Yes to No
Actions
Copy link

Also available in: PDF Atom