Project

General

Profile

Actions

Bug #4376

closed

TCP flow that retransmits the SYN with a newer TSval not properly tracked

Added by Nate Shimp about 3 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Target version:
Affected Versions:
Effort:
Difficulty:
Label:

Description

We've run into a situation where Suricata 5.0.3 ignores a TCP flow because the 3-way handshake starts with:

SYN (TSval=X, TSecr=0)                -->
SYN (retransmitted, TSval=Y, TSecr=0) -->
                                      <-- SYN, ACK (TSval=Z, TSecr=Y)

The TCP state machine only stores the original SYN's TSval in the session, so StateSynSentValidateTimestamp() says the timestamps are invalid when it sees the SYN-ACK.
We have a fix for this that updates the session's timestamp if Suricata sees another SYN with a newer timestamp:

diff --git a/src/stream-tcp.c b/src/stream-tcp.c
index a99ce68b6..8e218f460 100644
--- a/src/stream-tcp.c
+++ b/src/stream-tcp.c
@@ -1641,6 +1641,21 @@ static int StreamTcpPacketStateSynSent(ThreadVars *tv, Packet *p,
                     "ssn->client.last_ack %"PRIu32"", ssn,
                     ssn->client.isn, ssn->client.next_seq,
                     ssn->client.last_ack);
+        } else if (PKT_IS_TOSERVER(p)) {
+            /* This covers a subtle edge case where a SYN is retransmitted with a newer
+             * TSval. If the the session's last_ts value isn't updated, the SYN-ACK
+             * will fail timestamp validation and the flow will be lost.
+             */
+            if ((ssn->client.flags & STREAMTCP_STREAM_FLAG_TIMESTAMP) != 0) {
+                if (TCP_HAS_TS(p)) {
+                    SCLogDebug("SYN retransmitted with timestamp options on packet %"PRIu64", ssn %p.", p->pcap_cnt, ssn);
+                    uint32_t ts_val = TCP_GET_TSVAL(p);
+                    if (ssn->client.last_ts < ts_val) {
+                        ssn->client.last_ts = ts_val;
+                        ssn->client.last_pkt_ts = p->ts.tv_sec;
+                    }
+                }
+            }
         }

         /** \todo check if it's correct or set event */

We're wondering if this is a reasonable fix, or if upheaving some security precaution.


Files

syn_retransmit_with_ts.pcap (228 KB) syn_retransmit_with_ts.pcap Example PCAP Nate Shimp, 03/02/2021 04:37 PM

Subtasks 2 (0 open2 closed)

Bug #5421: TCP flow that retransmits the SYN with a newer TSval not properly tracked (6.0.x backport)ClosedVictor JulienActions
Bug #5429: TCP flow that retransmits the SYN with a newer TSval not properly tracked (5.0.x backport)ClosedVictor JulienActions
Actions #1

Updated by Victor Julien about 3 years ago

  • Status changed from New to Assigned
  • Assignee set to Victor Julien
  • Target version set to 7.0.0-beta1

Thanks Nate, I will have a look.

Actions #2

Updated by Victor Julien almost 2 years ago

  • Subject changed from Suricata ignores TCP flow that retransmits the SYN with a newer TSval to TCP flow that retransmits the SYN with a newer TSval not properly tracked
  • Status changed from Assigned to In Review
Actions #3

Updated by Victor Julien almost 2 years ago

  • Assignee changed from Victor Julien to Michael Tremer
Actions #4

Updated by Victor Julien almost 2 years ago

  • Label Needs backport to 5.0, Needs backport to 6.0 added
Actions #6

Updated by Victor Julien over 1 year ago

  • Label deleted (Needs backport to 6.0)
Actions #7

Updated by Victor Julien over 1 year ago

  • Status changed from Resolved to Closed
  • Label deleted (Needs backport to 5.0)
Actions

Also available in: Atom PDF