OISF/suricata
1
0
Fork 0
mirror of https://github.com/OISF/suricata.git synced 2026-10-11 22:09:53 +00:00
Code Issues Packages Projects Releases Wiki Activity

stream/tcp: keep the ACK tracking alive past the reassembly depth

Browse Source 
A direction that hit stream.reassembly.depth stops collecting data, so
StreamTcpPruneSession() stops sliding its reassembly buffer and base_seq
freezes where the data stopped while the sequence numbers keep moving. The
base_seq bound on last_ack updates compares an ACK against that anchor with
SEQ_GT(), which is only meaningful within half the sequence space: 2^31 bytes
past the depth it reads a regular ACK as being behind base_seq and last_ack
stops moving, next_win with it, and every packet from then on is flagged
out-of-window or with an invalid ack. 2^32 bytes later both comparisons line up
again at once, last_ack takes one step of 4 GB and the reassembly engine
reports a gap over data the peer already acked - for TLS a decoder error, and
in IPS mode the exception policy drops the flow without RST or FIN.

Take the bound only while the direction is still reassembling, as that is when
base_seq tracks the stream; nothing is inserted once it stopped, so there is
nothing left for the bound to protect. Where base_seq is ahead of the ACK it
still refuses the update, so the check Redmine #6865 added stays in place for
streams that are still collected.

Ticket: #9141.
This commit is contained in:
Victor Julien
2026-10-08 13:08:15 +00:00
parent 215961d672
commit e465600c9f
2 changed files with 125 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/stream-tcp.c
+26 -1
View File
@@ -1058,6 +1058,30 @@ void StreamTcpSetOSPolicy(TcpStream *stream, Packet *p)
SCLogDebug("Policy is %" PRIu8 "", stream->os_policy);
}
/**
* \brief check if the reassembly base of a stream can still bound an ACK
*
* The base_seq of a direction is the sequence number of the first byte the
* reassembly buffer holds, and it only moves when StreamTcpPruneSession()
* slides that buffer. A direction that is done reassembling (reassembly depth
* reached, or app-layer and raw both finished) never slides again, so its
* base_seq stays behind while the sequence numbers of the stream keep
* advancing. Since SEQ_GT() only compares within half the sequence space, such
* a stale anchor reads as "ahead of the ACK" once 2^31 bytes went by, which
* would freeze last_ack (and with it next_win) until the sequence space came
* all the way round again 2^32 bytes later. Nothing is inserted in that
* direction anymore, so the bound has nothing to protect there.
*
* \param stream stream to check
* \retval true base_seq still tracks the stream, so it can bound an ACK
* \retval false base_seq went stale, do not bound the ACK with it
*/
static inline bool StreamTcpBaseSeqBoundsAck(const TcpStream *stream)
{
return (stream->flags &
(STREAMTCP_STREAM_FLAG_NOREASSEMBLY | STREAMTCP_STREAM_FLAG_DEPTH_REACHED)) == 0;
}
/**
* \brief macro to update last_ack only if the new value is higher
*
@@ -1067,7 +1091,8 @@ void StreamTcpSetOSPolicy(TcpStream *stream, Packet *p)
*/
#define StreamTcpUpdateLastAck(ssn, stream, ack) \
{ \
if (SEQ_GT((ack), (stream)->last_ack) && SEQ_GT(ack, (stream)->base_seq)) { \
if (SEQ_GT((ack), (stream)->last_ack) && \
(!StreamTcpBaseSeqBoundsAck(stream) || SEQ_GT(ack, (stream)->base_seq))) { \
SCLogDebug("ssn %p: last_ack set to %" PRIu32 ", moved %u forward", (ssn), (ack), \
(ack) - (stream)->last_ack); \
if ((SEQ_LEQ((stream)->last_ack, (stream)->next_seq) && \
src/tests/stream-tcp.c
+99
View File
@@ -3402,6 +3402,101 @@ end:
return ret;
}
/**
* \brief A direction that stopped reassembling must keep tracking ACK's.
*
* Emulates the stream state of a flow that carried 2^31 bytes past the point
* where reassembly depth was reached: base_seq froze there, last_ack is at the
* far edge of the sequence space that base_seq can still compare against, and
* the next ACK of the peer is a regular in-window one. Taking 2^31 bytes of
* traffic to get there is not the point of the test: the arithmetic is what
* matters, so the numbers are set directly.
*/
static int StreamTcpTest46(void)
{
TcpSession ssn;
memset(&ssn, 0, sizeof(ssn));
TcpStream *stream = &ssn.client;
stream->isn = 1000;
stream->base_seq = stream->isn + 1 + 4096;
stream->last_ack = stream->base_seq + (uint32_t)INT32_MAX - 1;
stream->next_seq = stream->last_ack + 1;
stream->flags = STREAMTCP_STREAM_FLAG_NOREASSEMBLY | STREAMTCP_STREAM_FLAG_DEPTH_REACHED;
/* what a peer sends: a step forward of one packet, well inside the window */
const uint32_t ack = stream->next_seq + 8192;
StreamTcpUpdateLastAck((&ssn), stream, ack);
FAIL_IF(stream->last_ack != ack);
PASS;
}
/**
* \brief The same ACK must still be refused while the direction is
* reassembling, for base_seq tracks the stream there and an ACK that
* far ahead of it is not a real one (see Redmine #6865).
*/
static int StreamTcpTest47(void)
{
TcpSession ssn;
memset(&ssn, 0, sizeof(ssn));
TcpStream *stream = &ssn.client;
stream->isn = 1000;
stream->base_seq = stream->isn + 1 + 4096;
stream->last_ack = stream->base_seq + (uint32_t)INT32_MAX - 1;
stream->next_seq = stream->last_ack + 1;
const uint32_t prev_last_ack = stream->last_ack;
const uint32_t ack = stream->next_seq + 8192;
StreamTcpUpdateLastAck((&ssn), stream, ack);
FAIL_IF(stream->last_ack != prev_last_ack);
PASS;
}
/**
* \brief the Redmine #6865 shape on a direction that stopped reassembling
*
* The bound is dropped there, so this is the state to check: an ACK that is
* ahead of last_ack yet behind base_seq. StreamTcpUpdateLastAck() only ever
* moves last_ack forward, as the first half of its condition is
* SEQ_GT(ack, last_ack), so what is at stake is not a backwards step but
* whether a last_ack behind base_seq is still readable. It is:
* STREAM_LASTACK_GT_BASESEQ() is false for it, so the reassembly side takes
* the base offset as the absolute last_ack and reports no progress rather
* than a negative one. Follow up with a regular ACK to show tracking resumes.
*/
static int StreamTcpTest48(void)
{
TcpSession ssn;
memset(&ssn, 0, sizeof(ssn));
TcpStream *stream = &ssn.client;
stream->isn = 1000;
stream->base_seq = stream->isn + 1 + 4096;
stream->last_ack = stream->base_seq - 8192;
stream->next_seq = stream->base_seq;
stream->next_win = stream->last_ack + 65535;
stream->tcp_flags = TH_ACK;
stream->flags = STREAMTCP_STREAM_FLAG_NOREASSEMBLY | STREAMTCP_STREAM_FLAG_DEPTH_REACHED;
/* ahead of last_ack, behind base_seq: what #6865 refuses on a stream that
* is still being reassembled (see StreamTcpTest47) */
const uint32_t prev = stream->last_ack;
const uint32_t ack = stream->base_seq - 4096;
FAIL_IF(!SEQ_GT(ack, prev));
FAIL_IF(SEQ_GT(ack, stream->base_seq));
StreamTcpUpdateLastAck((&ssn), stream, ack);
FAIL_IF(stream->last_ack != ack);
FAIL_IF(SEQ_LT(stream->last_ack, prev));
FAIL_IF(STREAM_LASTACK_GT_BASESEQ(stream));
FAIL_IF(stream->next_win != prev + 65535);
/* and the next regular ACK, now past base_seq, tracks as usual */
const uint32_t next = stream->base_seq + 4096;
StreamTcpUpdateLastAck((&ssn), stream, next);
FAIL_IF(stream->last_ack != next);
FAIL_IF(!STREAM_LASTACK_GT_BASESEQ(stream));
PASS;
}
void StreamTcpRegisterTests(void)
{
UtRegisterTest("StreamTcpTest01 -- TCP session allocation", StreamTcpTest01);
@@ -3460,6 +3555,10 @@ void StreamTcpRegisterTests(void)
UtRegisterTest("StreamTcpTest44 -- SYN/ACK queue", StreamTcpTest44);
UtRegisterTest("StreamTcpTest45 -- SYN/ACK queue", StreamTcpTest45);
UtRegisterTest("StreamTcpTest46 -- ACK tracking past a stale reassembly base", StreamTcpTest46);
UtRegisterTest("StreamTcpTest47 -- ACK bound by base_seq while reassembling", StreamTcpTest47);
UtRegisterTest("StreamTcpTest48 -- #6865 ack shape on a stopped direction", StreamTcpTest48);
/* set up the reassembly tests as well */
StreamTcpReassembleRegisterTests();