From: Victor Julien <vjulien@oisf.net>
Date: Wed, 15 Feb 2023 10:56:47 +0000 (+0100)
Subject: stream: improve FIN next_seq handling
X-Git-Tag: suricata-7.0.0-rc2~563
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=80a012a7877cad8b9f8d12ba4bab63971ac7bb78;p=thirdparty%2Fsuricata.git

stream: improve FIN next_seq handling

Update next_seq to SEQ + payload_len + 1, so retransmission checks
work better.

Bug: #5877.
---

diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index de6c41903d..401c84f4e7 100644
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -983,7 +983,7 @@ static int StreamTcpPacketStateNone(
         /* set the sequence numbers and window */
         ssn->client.isn = TCP_GET_SEQ(p) - 1;
         STREAMTCP_SET_RA_BASE_SEQ(&ssn->client, ssn->client.isn);
-        ssn->client.next_seq = TCP_GET_SEQ(p) + p->payload_len;
+        ssn->client.next_seq = TCP_GET_SEQ(p) + p->payload_len + 1;
         ssn->client.window = TCP_GET_WINDOW(p) << ssn->client.wscale;
         ssn->client.last_ack = TCP_GET_SEQ(p);
         ssn->client.next_win = ssn->client.last_ack + ssn->client.window;
@@ -3216,7 +3216,7 @@ static int StreamTcpHandleFin(ThreadVars *tv, StreamTcpThread *stt, TcpSession *
         SCLogDebug("ssn %p: state changed to TCP_FIN_WAIT1", ssn);
 
         if (SEQ_EQ(TCP_GET_SEQ(p), ssn->server.next_seq))
-            ssn->server.next_seq = TCP_GET_SEQ(p) + p->payload_len;
+            ssn->server.next_seq = TCP_GET_SEQ(p) + p->payload_len + 1;
 
         SCLogDebug("ssn %p: ssn->server.next_seq %" PRIu32 "", ssn,
                     ssn->server.next_seq);
@@ -3311,9 +3311,8 @@ static int StreamTcpPacketStateFinWait1(
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
 
-            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->client.next_seq) ||
-                    SEQ_GT(TCP_GET_SEQ(p), (ssn->client.last_ack + ssn->client.window)))
-            {
+            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->client.next_seq - 1) ||
+                       SEQ_GT(TCP_GET_SEQ(p), (ssn->client.last_ack + ssn->client.window))) {
                 SCLogDebug("ssn %p: -> SEQ mismatch, packet SEQ %" PRIu32 ""
                         " != %" PRIu32 " from stream", ssn,
                         TCP_GET_SEQ(p), ssn->client.next_seq);
@@ -3340,11 +3339,11 @@ static int StreamTcpPacketStateFinWait1(
 
             /* Update the next_seq, in case if we have missed the client
                packet and server has already received and acked it */
-            if (SEQ_LT(ssn->server.next_seq, TCP_GET_ACK(p)))
+            if (SEQ_LT(ssn->server.next_seq - 1, TCP_GET_ACK(p)))
                 ssn->server.next_seq = TCP_GET_ACK(p);
 
             if (SEQ_EQ(ssn->client.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->client, (ssn->client.next_seq + p->payload_len));
+                StreamTcpUpdateNextSeq(ssn, &ssn->client, (TCP_GET_SEQ(p) + p->payload_len));
             }
 
             StreamTcpUpdateLastAck(ssn, &ssn->server, TCP_GET_ACK(p));
@@ -3363,12 +3362,12 @@ static int StreamTcpPacketStateFinWait1(
             if (StreamTcpPacketIsRetransmission(&ssn->server, p)) {
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
-            } else if (SEQ_EQ(ssn->server.next_seq, TCP_GET_SEQ(p)) &&
+            } else if (SEQ_EQ(ssn->server.next_seq - 1, TCP_GET_SEQ(p)) &&
                        SEQ_EQ(ssn->client.last_ack, TCP_GET_ACK(p))) {
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
 
-            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->server.next_seq) ||
+            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->server.next_seq - 1) ||
                        SEQ_GT(TCP_GET_SEQ(p), (ssn->server.last_ack + ssn->server.window))) {
                 SCLogDebug("ssn %p: -> SEQ mismatch, packet SEQ %" PRIu32 ""
                         " != %" PRIu32 " from stream", ssn,
@@ -3383,24 +3382,26 @@ static int StreamTcpPacketStateFinWait1(
                 return -1;
             }
 
+            if (ssn->flags & STREAMTCP_FLAG_TIMESTAMP) {
+                StreamTcpHandleTimestamp(ssn, p);
+            }
+
             if (!retransmission) {
                 StreamTcpPacketSetState(p, ssn, TCP_TIME_WAIT);
                 SCLogDebug("ssn %p: state changed to TCP_TIME_WAIT", ssn);
 
                 ssn->client.window = TCP_GET_WINDOW(p) << ssn->client.wscale;
-            }
 
-            if (ssn->flags & STREAMTCP_FLAG_TIMESTAMP) {
-                StreamTcpHandleTimestamp(ssn, p);
-            }
+                /* Update the next_seq, in case if we have missed the client
+                   packet and server has already received and acked it */
+                if (SEQ_LT(ssn->client.next_seq - 1, TCP_GET_ACK(p)))
+                    ssn->client.next_seq = TCP_GET_ACK(p);
 
-            /* Update the next_seq, in case if we have missed the client
-               packet and server has already received and acked it */
-            if (SEQ_LT(ssn->client.next_seq, TCP_GET_ACK(p)))
-                ssn->client.next_seq = TCP_GET_ACK(p);
+                if (SEQ_EQ(ssn->server.next_seq - 1, TCP_GET_SEQ(p))) {
+                    StreamTcpUpdateNextSeq(ssn, &ssn->server, (TCP_GET_SEQ(p) + p->payload_len));
+                }
 
-            if (SEQ_EQ(ssn->server.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->server, (ssn->server.next_seq + p->payload_len));
+                StreamTcpUpdateLastAck(ssn, &ssn->client, TCP_GET_ACK(p));
             }
 
             StreamTcpReassembleHandleSegment(tv, stt->ra_ctx, ssn, &ssn->server, p);
@@ -3426,9 +3427,8 @@ static int StreamTcpPacketStateFinWait1(
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
 
-            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->client.next_seq) ||
-                    SEQ_GT(TCP_GET_SEQ(p), (ssn->client.last_ack + ssn->client.window)))
-            {
+            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->client.next_seq - 1) ||
+                       SEQ_GT(TCP_GET_SEQ(p), (ssn->client.last_ack + ssn->client.window))) {
                 SCLogDebug("ssn %p: -> SEQ mismatch, packet SEQ %" PRIu32 ""
                         " != %" PRIu32 " from stream", ssn,
                         TCP_GET_SEQ(p), ssn->client.next_seq);
@@ -3455,11 +3455,11 @@ static int StreamTcpPacketStateFinWait1(
 
             /* Update the next_seq, in case if we have missed the client
                packet and server has already received and acked it */
-            if (SEQ_LT(ssn->server.next_seq, TCP_GET_ACK(p)))
+            if (SEQ_LT(ssn->server.next_seq - 1, TCP_GET_ACK(p)))
                 ssn->server.next_seq = TCP_GET_ACK(p);
 
-            if (SEQ_EQ(ssn->client.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->client, (ssn->client.next_seq + p->payload_len));
+            if (SEQ_EQ(ssn->client.next_seq - 1, TCP_GET_SEQ(p))) {
+                StreamTcpUpdateNextSeq(ssn, &ssn->client, (TCP_GET_SEQ(p) + p->payload_len));
             }
 
             if (p->tcph->th_flags & TH_ACK)
@@ -3481,9 +3481,8 @@ static int StreamTcpPacketStateFinWait1(
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
 
-            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->server.next_seq) ||
-                    SEQ_GT(TCP_GET_SEQ(p), (ssn->server.last_ack + ssn->server.window)))
-            {
+            } else if (SEQ_LT(TCP_GET_SEQ(p), ssn->server.next_seq - 1) ||
+                       SEQ_GT(TCP_GET_SEQ(p), (ssn->server.last_ack + ssn->server.window))) {
                 SCLogDebug("ssn %p: -> SEQ mismatch, packet SEQ %" PRIu32 ""
                         " != %" PRIu32 " from stream", ssn,
                         TCP_GET_SEQ(p), ssn->server.next_seq);
@@ -3510,11 +3509,11 @@ static int StreamTcpPacketStateFinWait1(
 
             /* Update the next_seq, in case if we have missed the client
                packet and server has already received and acked it */
-            if (SEQ_LT(ssn->client.next_seq, TCP_GET_ACK(p)))
+            if (SEQ_LT(ssn->client.next_seq - 1, TCP_GET_ACK(p)))
                 ssn->client.next_seq = TCP_GET_ACK(p);
 
-            if (SEQ_EQ(ssn->server.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->server, (ssn->server.next_seq + p->payload_len));
+            if (SEQ_EQ(ssn->server.next_seq - 1, TCP_GET_SEQ(p))) {
+                StreamTcpUpdateNextSeq(ssn, &ssn->server, (TCP_GET_SEQ(p) + p->payload_len));
             }
 
             if (p->tcph->th_flags & TH_ACK)
@@ -3557,11 +3556,8 @@ static int StreamTcpPacketStateFinWait1(
             if (SEQ_LT(TCP_GET_ACK(p), ssn->server.next_seq)) {
                 SCLogDebug("ssn %p: ACK's older segment as %u < %u", ssn, TCP_GET_ACK(p),
                         ssn->server.next_seq);
-                retransmission = 1;
-            }
-
-            if (!retransmission) {
-                if (SEQ_EQ(TCP_GET_ACK(p), ssn->server.next_seq + 1)) {
+            } else if (!retransmission) {
+                if (SEQ_EQ(TCP_GET_ACK(p), ssn->server.next_seq)) {
                     if (SEQ_LEQ(TCP_GET_SEQ(p) + p->payload_len, ssn->client.next_win) ||
                             (ssn->flags & (STREAMTCP_FLAG_MIDSTREAM | STREAMTCP_FLAG_ASYNC))) {
                         SCLogDebug("ssn %p: seq %" PRIu32 " in window, ssn->client.next_win "
@@ -3592,11 +3588,11 @@ static int StreamTcpPacketStateFinWait1(
 
             /* Update the next_seq, in case if we have missed the client
                packet and server has already received and acked it */
-            if (SEQ_LT(ssn->server.next_seq, TCP_GET_ACK(p)))
+            if (SEQ_LT(ssn->server.next_seq - 1, TCP_GET_ACK(p)))
                 ssn->server.next_seq = TCP_GET_ACK(p);
 
             if (SEQ_EQ(ssn->client.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->client, (ssn->client.next_seq + p->payload_len));
+                StreamTcpUpdateNextSeq(ssn, &ssn->client, (TCP_GET_SEQ(p) + p->payload_len));
             }
 
             StreamTcpUpdateLastAck(ssn, &ssn->server, TCP_GET_ACK(p));
@@ -3638,7 +3634,7 @@ static int StreamTcpPacketStateFinWait1(
                     SCLogDebug("ssn %p: seq %"PRIu32" in window, ssn->server.next_win "
                             "%" PRIu32 "", ssn, TCP_GET_SEQ(p), ssn->server.next_win);
 
-                    if (TCP_GET_SEQ(p) == ssn->server.next_seq) {
+                    if (TCP_GET_SEQ(p) == ssn->server.next_seq - 1) {
                         StreamTcpPacketSetState(p, ssn, TCP_FIN_WAIT2);
                         SCLogDebug("ssn %p: state changed to TCP_FIN_WAIT2", ssn);
                     }
@@ -3659,11 +3655,11 @@ static int StreamTcpPacketStateFinWait1(
 
             /* Update the next_seq, in case if we have missed the client
                packet and server has already received and acked it */
-            if (SEQ_LT(ssn->client.next_seq, TCP_GET_ACK(p)))
+            if (SEQ_LT(ssn->client.next_seq - 1, TCP_GET_ACK(p)))
                 ssn->client.next_seq = TCP_GET_ACK(p);
 
-            if (SEQ_EQ(ssn->server.next_seq, TCP_GET_SEQ(p))) {
-                StreamTcpUpdateNextSeq(ssn, &ssn->server, (ssn->server.next_seq + p->payload_len));
+            if (SEQ_EQ(ssn->server.next_seq - 1, TCP_GET_SEQ(p))) {
+                StreamTcpUpdateNextSeq(ssn, &ssn->server, (TCP_GET_SEQ(p) + p->payload_len));
             }
 
             StreamTcpUpdateLastAck(ssn, &ssn->client, TCP_GET_ACK(p));
@@ -4673,7 +4669,8 @@ static int StreamTcpPacketStateTimeWait(
             if (StreamTcpPacketIsRetransmission(&ssn->server, p)) {
                 SCLogDebug("ssn %p: packet is retransmission", ssn);
                 retransmission = 1;
-            } else if (TCP_GET_SEQ(p) != ssn->server.next_seq && TCP_GET_SEQ(p) != ssn->server.next_seq+1) {
+            } else if (TCP_GET_SEQ(p) != ssn->server.next_seq - 1 &&
+                       TCP_GET_SEQ(p) != ssn->server.next_seq) {
                 if (p->payload_len > 0 && TCP_GET_SEQ(p) == ssn->server.last_ack) {
                     SCLogDebug("ssn %p: -> retransmission", ssn);
                     SCReturnInt(0);