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

applayer: incr flow counter if alproto for other dir was detected #12415

Closed
wants to merge 1 commit into from

Conversation

inashivb
Copy link
Member

Link to ticket: https://redmine.openinfosecfoundation.org/issues/7238

@catenacyber
What do you think about this?
Can this lead to additional flows being miscalculated for any cases?

@inashivb inashivb requested a review from catenacyber January 17, 2025 10:35
@inashivb
Copy link
Member Author

For the pcap at hand, for example, on master, I see:

$ cat output/eve.json | jq -c 'select(.event_type=="flow" and .app_proto == "dcerpc")' | wc -l
9
$ cat output/eve.json | jq 'select(.event_type=="stats") | .stats.app_layer.flow.dcerpc_tcp'
0

Consider a case like the following.
-> Opposing direction data comes in first
-> At the beginning of Protocol Detection, the alproto for other side is
unknown
-> A successful call to TCPProtoDetectTriggerOpposingSide can however
lead to detection of alproto for the other side
-> Following this, a successful call is made to AppLayerParserParse

However, the flow counter is not incremented for this case.
This also does not map the number of flow events logged by Suricata for
such a case.

Increment the flow counter for a new flow for any applayer proto that is
accepted by that specific protocol's parser.

Related to Bug 7238
@inashivb inashivb force-pushed the flow-counter-update/v1 branch from 3c14f81 to 3ba44a3 Compare January 17, 2025 11:06
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.64%. Comparing base (8f6795d) to head (3ba44a3).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12415   +/-   ##
=======================================
  Coverage   80.63%   80.64%           
=======================================
  Files         918      918           
  Lines      258696   258700    +4     
=======================================
+ Hits       208598   208627   +29     
+ Misses      50098    50073   -25     
Flag Coverage Δ
fuzzcorpus 56.81% <100.00%> (+<0.01%) ⬆️
livemode 19.40% <0.00%> (+<0.01%) ⬆️
pcap 44.28% <100.00%> (-0.02%) ⬇️
suricata-verify 63.25% <100.00%> (+0.02%) ⬆️
unittests 58.52% <100.00%> (+<0.01%) ⬆️

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

@suricata-qa
Copy link

WARNING:

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.flow.smtp 7747 10699 138.11%
SURI_TLPR1_stats_chk
.app_layer.flow.ftp 36200 38016 105.02%
.app_layer.flow.smtp 348555 661554 189.8%
.app_layer.flow.smb 41618 54492 130.93%
.app_layer.flow.dcerpc_tcp 43 111 258.14%
IPS_AFP_stats_chk
.app_layer.flow.smtp 37800 74518 197.14%
.app_layer.flow.smb 243000 350989 144.44%
TREX_GENERIC_stats_chk
.app_layer.flow.smtp 14747 29493 199.99%
.app_layer.flow.smb 141752 228671 161.32%

Pipeline 24250

@inashivb inashivb closed this Jan 23, 2025
@inashivb inashivb deleted the flow-counter-update/v1 branch January 23, 2025 06:17
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.

2 participants