Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stream syn queue/v60 #8563

Merged
merged 33 commits into from Mar 1, 2023
Merged

Conversation

victorjulien
Copy link
Member

#8562 with additional fixes

suricata-verify-pr: 1136

Due to gaps/padding the size of the struct won't change.
Support case where there are multiple SYN retransmits, where
each has a new timestamp.

Before this patch, Suricata would only accept a SYN/ACK that
matches the last timestamp. However, observed behavior is that
the server may choose to only respond to the first. In IPS mode
this could lead to a connection timing out as Suricata drops
the SYN/ACK it considers wrong, and the server continues to
retransmit it.

This patch reuses the SYN/ACK queuing logic to keep a list
of SYN packets and their window, timestamp, wscale and sackok
settings. Then when the SYN/ACK arrives, it is first evaluated
against the normal session state. But if it fails due to a
timestamp mismatch, it will look for queued SYN's and see if
any of them match the timestamp. If one does, the ssn is updated
to use that SYN and the SYN/ACK is accepted.

Bug: OISF#5856.
An ACK that ACK'd older data while still being in-window could
lead to FIN_WAIT1 to FIN_WAIT2 state transition. Detect this
case and generally harden the check.

Bug: OISF#5877.
next_seq sometimes depends on last_ack in cases of packet loss
catch up, so first update it.

Bug: OISF#5877.
Update next_seq to SEQ + payload_len + 1, so retransmission checks
work better.

Bug: OISF#5877.
For accepted FIN packets, always update next_seq. This helps track the
FIN sequence.

Bug: OISF#5877.
These flags can then later be used by stream logging.
Default to allowing 10 SYNs to not trigger an event on a connection
attempt that times out.
Debug facility to get a per packet view into the stream engine's state.

Logs after a packet has been processed in the stream engine, so the view
into the state includes the updates based on the current packet.

Marked as experimental so it can be changed w/o notice.

Bug: OISF#5876.
On every accepted packet in established state, update next_seq if
packet seq+len is larger than existing next_seq. This allows it to
catch up after large gaps that are filled again a bit later.

Bug: OISF#5877.
Modeled after Wiresharks Dup-ACK detection.
RFC 2883 specifies a special use of SACKs to indicate a host has
received a segment it considers a spurious retransmission.
Linux is slightly more permissive wrt timestamps than many
other OS'. To avoid many events/issues with linux hosts, add an
option to allow for this slightly more permissive behavior.

Ideally the host-os config would be used, but in practice this
setting is rarely set up correctly, if at all.

This option is enabled by default.
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: OISF#5875.
This is another indicator for packet loss or strange captures.
If a in-tree segment was partly before base_seq the overlap detection
miscalculated the data offset. This lead to memcmp comparing the wrong
data.

Bug: OISF#5881.
@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.flow.end.tcp_state.last_ack 1004 257 25.6%
SURI_TLPR1_stats_chk
.flow.end.tcp_state.fin_wait2 156072 127814 81.89%

Pipeline 12630

@victorjulien victorjulien merged commit 8746fbe into OISF:master Mar 1, 2023
@victorjulien victorjulien deleted the stream-syn-queue/v60 branch March 31, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants