Skip to content

app-layer: improve limits on number of probing parsers#12233

Closed
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:protodetect-probing-mask32-7437-v1
Closed

app-layer: improve limits on number of probing parsers#12233
catenacyber wants to merge 1 commit intoOISF:masterfrom
catenacyber:protodetect-probing-mask32-7437-v1

Conversation

@catenacyber
Copy link
Contributor

@catenacyber catenacyber commented Dec 5, 2024

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

Describe changes:

  • app-layer: improve limits on number of probing parsers (which was limited to 32 in total and worked out of luck with the current 37 protocols)

Part of #12163

There was an implicit limit of 32 app-layer protocols
used by probing parsers through a mask, meaning that
Suricata should not support more than 32 app-layer protocols
in total.

This limit is relaxed to each flow not being able to
run more than 32 probing parsers, meaning that for each source
and destination port combination, the sum of registered
probing parsers should not exceed 32, even if there are more
than 32 in total.

Ticket: 7437
@codecov
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.18%. Comparing base (09ba69c) to head (b8571ac).
Report is 32 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12233      +/-   ##
==========================================
- Coverage   83.19%   83.18%   -0.01%     
==========================================
  Files         912      912              
  Lines      257166   257105      -61     
==========================================
- Hits       213938   213863      -75     
- Misses      43228    43242      +14     
Flag Coverage Δ
fuzzcorpus 61.01% <83.33%> (+<0.01%) ⬆️
livemode 19.40% <0.00%> (-0.01%) ⬇️
pcap 44.36% <83.33%> (-0.04%) ⬇️
suricata-verify 62.76% <83.33%> (-0.03%) ⬇️
unittests 59.17% <93.47%> (-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.error.tls.parser 1152 1203 104.43%
SURI_TLPR1_stats_chk
.uptime 624 654 104.81%
.app_layer.tx.ftp 95383 102154 107.1%
.ftp.memuse 2906 10638 366.07%

Pipeline 23709

@catenacyber
Copy link
Contributor Author

@ct0br0 this QA result looks unrelated/unexpected

Have you seen something similar ? Should I rebase this ? Should we run QA again ? Or do nothing..?

@ct0br0
Copy link

ct0br0 commented Dec 18, 2024

i'll give it another run to see if it's the same and try to get pcaps

@catenacyber
Copy link
Contributor Author

Ok, so this PR seems to uncover and fix another bug

Previous check if (alproto_masks[0] == mask) { to FLOW_SET_PP_DONE(f, dir); was missing some cases

  1. I have a flow with client side detected as TLS
  2. We try to detect server side, only expect to run SMTP (port 465) but run both TLS and SMTP so alproto_masks[0] is bigger than (includes) mask

Right check should have been if ((alproto_masks[0] & mask) == mask)

@catenacyber
Copy link
Contributor Author

Replaced by #12307

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.

3 participants