Skip to content

Comments

Smtp server detection 1125 v17#8892

Closed
catenacyber wants to merge 6 commits intoOISF:masterfrom
catenacyber:smtp-server-detection-1125-v17
Closed

Smtp server detection 1125 v17#8892
catenacyber wants to merge 6 commits intoOISF:masterfrom
catenacyber:smtp-server-detection-1125-v17

Conversation

@catenacyber
Copy link
Contributor

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/1125 preliminary work

Describe changes:

  • protocoldetect: use stricter max depth
  • pop3: add detection

Modifies #8804 by needed rebase

Summing up :

  • Protocol detection has many bugs (see first commits)
  • Especially, eve.json stats field about flows are not matching the count of flow with app_proto because of so many corner cases
  • QA baseline is wrong because of counting pop3 flows as FTP ones (because of USER pattern matched)
  • QA baseline is wrong because of counting IRC flows on port 5432 as pgsql because pgsql probing parser to client accepts anything

@victorjulien how should this be handled ? Create many tickets and find the right priority order to deal with them ?
Should it begin with just commit f27a6f1 but using port 110 instead of POP3 ? That would solve QA wrongly tagging POP3 flows as FTP

@suricata-qa
Copy link

WARNING:

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.tx.ftp 646 187 28.95%
.app_layer.error.ftp.gap 2 0 -
.app_layer.error.ftp.parser 2 0 -
.ftp.memuse 348 3 0.86%
SURI_TLPR1_stats_chk
.app_layer.flow.pgsql 0 55 -
IPS_AFP_stats_chk
.flow.end.state.established 583199 550799 94.44%
.flow.end.tcp_state.established 201960 169560 83.96%
.app_layer.flow.ftp 33480 1080 3.23%
.app_layer.tx.ftp 131760 2160 1.64%
.app_layer.error.ftp.parser 32400 0 -
TREX_GENERIC_stats_chk
.app_layer.flow.ftp 18444 0 -
.app_layer.tx.ftp 73776 0 -
.app_layer.error.ftp.parser 18444 0 -

Pipeline 13821

memset(pm_results_bf, 0, sizeof(pm_results_bf));

// Do not take pm_ctx->pp_max_len of all probing parsers,
// but only the the probing parsers which matched a pattern.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: remove extra the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done (still waiting for my big questions to be answered)

For probing parsers used with a pattern, restrict the max depth
to these probing parsers and not all probing parsers.

Finishing protocol detection earlier allows to parse data earlier
in the case we recognize only one side.
When an app-layer is recognized for one side, but the other
is still unknown, and the parsing of the one side errors,
it disables app-layer, and thus the ending pseudo-packets do
not get a chance to conclude for app-layer detection on the
unknown side.

So count in stats the app-layer protocol that was recognized
the first time.
When probing parser is only tried from one recognized side
of the protocol, even if this parser failed, it never set
probing parser done as no mask were used.
As these have no StateAlloc, AppLayerParserParse disables app-layer
parsing, which prevents other side protocol detection...
@catenacyber catenacyber force-pushed the smtp-server-detection-1125-v17 branch from af49503 to fdc7c9b Compare May 25, 2023 11:37
@suricata-qa
Copy link

WARNING:

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.tx.ftp 646 187 28.95%
.app_layer.error.ftp.gap 2 0 -
.app_layer.error.ftp.parser 2 0 -
.ftp.memuse 348 3 0.86%
SURI_TLPR1_stats_chk
.app_layer.flow.pgsql 0 55 -
IPS_AFP_stats_chk
.flow.end.state.established 583199 550799 94.44%
.flow.end.tcp_state.established 201960 169560 83.96%
.app_layer.flow.ftp 33480 1080 3.23%
.app_layer.tx.ftp 131760 2160 1.64%
.app_layer.error.ftp.parser 32400 0 -
TREX_GENERIC_stats_chk
.app_layer.flow.ftp 18444 0 -
.app_layer.tx.ftp 73776 0 -
.app_layer.error.ftp.parser 18444 0 -

Pipeline 14077

@victorjulien victorjulien added this to the 8.0 milestone Jul 11, 2023
@suricata-qa
Copy link

WARNING:

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
SURI_TLPW1_stats_chk
.app_layer.tx.ftp 646 187 28.95%
.app_layer.error.ftp.gap 2 0 -
.app_layer.error.ftp.parser 2 0 -
.ftp.memuse 348 3 0.86%
SURI_TLPR1_stats_chk
.app_layer.flow.pgsql 0 55 -
IPS_AFP_stats_chk
.flow.end.state.established 583199 550799 94.44%
.flow.end.tcp_state.established 201960 169560 83.96%
.app_layer.flow.ftp 33480 1080 3.23%
.app_layer.tx.ftp 131760 2160 1.64%
.app_layer.error.ftp.parser 32400 0 -
TREX_GENERIC_stats_chk
.app_layer.flow.ftp 18444 0 -
.app_layer.tx.ftp 73776 0 -
.app_layer.error.ftp.parser 18444 0 -

Pipeline 14077

@catenacyber
Copy link
Contributor Author

Continues in #9500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

4 participants