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 8 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 about 1 year ago Actions
Copy link
 #2

  • Priority changed from Normal to High

Victor, how bad is this assertion ?

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

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

PA Updated by Philippe Antoine about 1 year 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 11 months ago Actions
Copy link
 #5

  • Affected Versions 8.0.0 added

PA Updated by Philippe Antoine 10 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 10 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 10 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 10 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 10 months ago Actions
Copy link
 #10

  • Assignee changed from Victor Julien to Shivani Bhardwaj

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

  • Status changed from New to In Review

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

  • Status changed from In Review to Closed

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

  • Priority changed from High to Normal

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

  • Private changed from Yes to No
Actions
Copy link

Also available in: PDF Atom