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

Protodetect probing mask32 7437 v2 #12307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

catenacyber
Copy link
Contributor

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

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)
  • smtp: improve probing parser in case of TLS first from client

SV_BRANCH=OISF/suricata-verify#2200

Closing #12256 to get this part first

#12233 with

  • new commit for SMTP
  • new SV test
  • a bit more in the first commit message to explain the other accidental bugfix

This will require a QA baseline update as highlighted by the SV test

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.

Also sets probing parsers done sooner in the case the other
side of the connection was detected first.

Ticket: 7437
@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 95972 102934 107.25%
.ftp.memuse 3102 10661 343.68%

Pipeline 24024

@catenacyber
Copy link
Contributor Author

A minimal change to get the same QA change should be

diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c
index a65a98c88a..b40c0adc44 100644
--- a/src/app-layer-detect-proto.c
+++ b/src/app-layer-detect-proto.c
@@ -619,7 +619,7 @@ again_midstream:
         else if (pp_port_sp)
             mask = pp_port_sp->alproto_mask;
 
-        if (alproto_masks[0] == mask) {
+        if ((alproto_masks[0] & mask) == mask) {
             FLOW_SET_PP_DONE(f, dir);
             SCLogDebug("%s, mask is now %08x, needed %08x, so done",
                     (dir == STREAM_TOSERVER) ? "toserver":"toclient",

meaning we have tried all expected probing parsers and maybe more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs baseline update QA will need a new base line
Development

Successfully merging this pull request may close these issues.

2 participants