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 plugin 5053 v3.15 #12163

Closed
wants to merge 18 commits into from

Conversation

catenacyber
Copy link
Contributor

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

Describe changes:

  • get ready to use dynamic number of app-layer protos (also work with static constant) in all places
  • preventive fix of macro with parenthesis cc @jufajardini
  • fix fuzz for protocol detection by having the right init sequence
  • sip: remove UPDATE method for detection as HTTP uses it (found by the fixed fuzz target)
  • app-layer plugins
  • remove limitation on probing parsers about 32 protocols

Let me know if I should split this PR

#12127 with latest commit adding versioning for plugin suricata compatibility as suggested by Pierre Chillier

instead of a global variable.

For easier initialization with dynamic number of protocols
for expectation_proto

Ticket: 5053
so that we can use safely EXCEPTION_POLICY_MAX*sizeof(x)
Ticket: 5053

delay after initialization so that StringToAppProto works
As too many cases are found when splitting tcp payload
As it is also used for HTTP/1
Remove it only for TCP and keep it for UDP.
Because some alprotos will remain static and defined as a constant,
such as ALPROTO_UNKNOWN=0, or ALPROTO_FAILED.

The regular already used protocols keep for now their static
identifier such as ALPROTO_SNMP, but this could be made more
dynamic in a later commit.

ALPROTO_FAILED was used in comparison and these needed to change to use
either ALPROTO_MAX or use standard function AppProtoIsValid
Ticket: 5053

The names are now dynamically registered at runtime.
The AppProto alproto enum identifiers are still static for now.

This is the final step before app-layer plugins.
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.
Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 63.25758% with 194 lines in your changes missing coverage. Please review.

Project coverage is 47.51%. Comparing base (bd7d38e) to head (087111b).
Report is 42 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12163      +/-   ##
==========================================
- Coverage   49.81%   47.51%   -2.31%     
==========================================
  Files         909      911       +2     
  Lines      257904   258327     +423     
==========================================
- Hits       128467   122734    -5733     
- Misses     129437   135593    +6156     
Flag Coverage Δ
fuzzcorpus 56.65% <68.00%> (-4.30%) ⬇️
livemode 19.40% <50.78%> (-0.04%) ⬇️
pcap 44.46% <67.78%> (+0.04%) ⬆️
suricata-verify 62.74% <73.60%> (-0.01%) ⬇️
unittests 9.00% <31.25%> (+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
.app_layer.tx.ftp 95383 102161 107.11%
.ftp.memuse 2906 10638 366.07%

Pipeline 23589

@catenacyber
Copy link
Contributor Author

Let me know if I should split this PR

So, I did

After these are merged, there will remain to do one PR with plugin support

@catenacyber
Copy link
Contributor Author

Rebased in #12256

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