stream: fix spurious retransmission handling

Fix spurious retransmissions getting dropped, stalling connections in IPS
mode.

There are several reasons why benign spurious retransmissions can happen,
with the most obvious one that an ACK is lost so the sender retransmits
while the receiver has ACK'd it. If Suricata sees the ACK but afterwards
it gets lost, we can get in this condition. Packet loss can have a wide
range of causes here, including packets reaching a host but getting
dropped in the NIC queue or kernel queues due to resource constraints.

So these packets are no longer an "error" in this patch.

Next to this, the accuracy of the spurious retransmission has been
improved. Use SEQ macros to compare sequence numbers. Only use base_seq
if reassembly is still enabled for a stream.

A special case is added for cases where a segment is before last_ack
but after base_seq, which can happen when protocol detection isn't
finished yet. In this case the segment is tagged as spurious, but still
processed. This way we can check for overlaps.

Bug: #5875.

diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index d0e49425136566ef724e8402dba542490e6ebc3e..f6789ae23f1454b05841270ecca65fd1a75d6967 100644 (file)
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -2909,8 +2909,12 @@ static bool StreamTcpPacketIsOutdatedAck(TcpSession *ssn, Packet *p)
 /** \internal
  *  \brief check if packet is before ack'd windows
  *  If packet is before last ack, we will not accept it
+ *
+ *  \retval 0 not spurious retransmission
+ *  \retval 1 before last_ack, after base_seq
+ *  \retval 2 before last_ack and base_seq
  */
-static bool StreamTcpPacketIsSpuriousRetransmission(const TcpSession *ssn, Packet *p)
+static int StreamTcpPacketIsSpuriousRetransmission(const TcpSession *ssn, Packet *p)
 {
     const TcpStream *stream;
     if (PKT_IS_TOCLIENT(p)) {
@@ -2918,20 +2922,36 @@ static bool StreamTcpPacketIsSpuriousRetransmission(const TcpSession *ssn, Packe
     } else {
         stream = &ssn->client;
     }
+    if (p->payload_len == 0)
+        return 0;
+
     /* take base_seq into account to avoid edge cases where last_ack might be
      * too far ahead during heavy packet loss */
-    const uint32_t le = MIN(stream->last_ack, stream->base_seq);
-    if (p->payload_len > 0 && (SEQ_LEQ(TCP_GET_SEQ(p) + p->payload_len, le))) {
+    if (!(stream->flags & STREAMTCP_STREAM_FLAG_NOREASSEMBLY)) {
+        if ((SEQ_LEQ(TCP_GET_SEQ(p) + p->payload_len, stream->base_seq))) {
+            SCLogDebug(
+                    "ssn %p: spurious retransmission; packet entirely before base_seq: SEQ %u(%u) "
+                    "last_ack %u base_seq %u",
+                    ssn, TCP_GET_SEQ(p), TCP_GET_SEQ(p) + p->payload_len, stream->last_ack,
+                    stream->base_seq);
+            STREAM_PKT_FLAG_SET(p, STREAM_PKT_FLAG_SPURIOUS_RETRANSMISSION);
+            return 2;
+        }
+    }
+
+    if ((SEQ_LEQ(TCP_GET_SEQ(p) + p->payload_len, stream->last_ack))) {
         SCLogDebug("ssn %p: spurious retransmission; packet entirely before last_ack: SEQ %u(%u) "
                    "last_ack %u",
                 ssn, TCP_GET_SEQ(p), TCP_GET_SEQ(p) + p->payload_len, stream->last_ack);
         STREAM_PKT_FLAG_SET(p, STREAM_PKT_FLAG_SPURIOUS_RETRANSMISSION);
-        return true;
+        return 1;
     }
+
     SCLogDebug("ssn %p: NOT spurious retransmission; packet NOT entirely before last_ack: SEQ "
-               "%u(%u) last_ack %u, le %u",
-            ssn, TCP_GET_SEQ(p), TCP_GET_SEQ(p) + p->payload_len, stream->last_ack, le);
-    return false;
+               "%u(%u) last_ack %u, base_seq %u",
+            ssn, TCP_GET_SEQ(p), TCP_GET_SEQ(p) + p->payload_len, stream->last_ack,
+            stream->base_seq);
+    return 0;
 }
 
 /**
@@ -5317,9 +5337,12 @@ int StreamTcpPacket (ThreadVars *tv, Packet *p, StreamTcpThread *stt,
             }
         }
 
-        if (StreamTcpPacketIsSpuriousRetransmission(ssn, p)) {
+        int ret = StreamTcpPacketIsSpuriousRetransmission(ssn, p);
+        if (ret > 0) {
             StreamTcpSetEvent(p, STREAM_PKT_SPURIOUS_RETRANSMISSION);
-            goto error;
+            /* skip packet if fully before base_seq */
+            if (ret == 2)
+                goto skip;
         }
 
         /* handle the per 'state' logic */

diff --git a/src/tests/stream-tcp.c b/src/tests/stream-tcp.c
index c9ab17d71bd391bb92a44b0be1a3fb1b52f4d279..8885423dec74dc9d4a0a43dca193b5792e2c81e7 100644 (file)
--- a/src/tests/stream-tcp.c
+++ b/src/tests/stream-tcp.c
@@ -701,7 +701,8 @@ static int StreamTcpTest10(void)
     p->payload = payload;
     p->payload_len = 3;
 
-    FAIL_IF_NOT(StreamTcpPacket(&tv, p, &stt, &pq) == -1);
+    /* spurious retransmission */
+    FAIL_IF_NOT(StreamTcpPacket(&tv, p, &stt, &pq) == 0);
 
     FAIL_IF(((TcpSession *)(p->flow->protoctx))->state != TCP_ESTABLISHED);