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

next/586/20240924/v1 #11829

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Conversation

victorjulien
Copy link
Member

catenacyber and others added 2 commits September 24, 2024 16:01
For internal function that is not part of C FFI
If the ACK packet completing the 3whs is received, the stream engine will
transition to "established". However, the packet itself will not be tagged
as "established". This will only happen for the next packet after the 3whs,
so that `flow:established` only matches after the 3whs.

It is possible that the ACK packet completing the 3whs was lost. Since the
ACK packets themselves are not acknowledged, there will be no retransmission
of them. Instead, the next packet can have the expected ACK flag as well as
data.

This case was mishandled in a subtle way. The stream engine state transition
was done correctly, as well as the data handling and app-layer updates.
However, the packet itself was not tagged as "established", which meant
that `flow:established` would not yet match.

This patch detects this case and tags the packet as established if ACK
with data is received that completes the 3whs.

Bug: OISF#7264.
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.60%. Comparing base (c55c7d6) to head (45eb7e4).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11829      +/-   ##
==========================================
+ Coverage   82.57%   82.60%   +0.02%     
==========================================
  Files         912      912              
  Lines      249357   249351       -6     
==========================================
+ Hits       205918   205965      +47     
+ Misses      43439    43386      -53     
Flag Coverage Δ
fuzzcorpus 60.59% <3.22%> (+0.03%) ⬆️
livemode 18.72% <0.00%> (-0.01%) ⬇️
pcap 44.06% <3.22%> (-0.06%) ⬇️
suricata-verify 62.03% <100.00%> (+0.03%) ⬆️
unittests 58.93% <1.61%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@jasonish jasonish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Staging looks clean.

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPR1_stats_chk
.uptime 649 624 96.15%

Pipeline 22799

@victorjulien victorjulien merged commit 45eb7e4 into OISF:master Sep 25, 2024
59 checks passed
@victorjulien victorjulien deleted the next/586/20240924/v1 branch September 25, 2024 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants