Skip to content

Smtp server detection 1125 v16#8804

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

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

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 #8696 by incrementing pop3 counters

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)

@ct0br0 @pevma should QA count the number of flows with app_proto instead of resorting to stats ?
cf
jq 'select(.event_type=="flow" and .app_proto=="ftp") | .app_proto' log/eve.json | wc -l versus jq 'select(.event_type=="stats") | .stats."app_layer".flow.ftp' log/eve.json as pointed out #8327 (comment)

@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

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...
@ct0br0
Copy link

ct0br0 commented May 4, 2023

As it stands now, any field with a number is compared against our baseline. The first jq looks like it will always equal the number of lines in stats.json? Unless I'm missing something (it is returning nothing for me as is).

@catenacyber
Copy link
Contributor Author

The first jq looks like it will always equal the number of lines in stats.json

jq 'select(.event_type=="flow" and .app_proto=="ftp") | .app_proto' log/eve.json | wc -l returns the number of lines being the number of flows whose app_proto is ftp

jq 'select(.event_type=="stats") | .stats."app_layer".flow.ftp' log/eve.json should return the same it does not in every case :-/

Is it clearer ?

@suricata-qa
Copy link

WARNING:

ERROR: QA failed on IPS_AFP_drop_chk.

field baseline test %
SURI_TLPW1_stats_chk
.tcp.rst 113274 144920 127.94%
.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 13570

@ct0br0
Copy link

ct0br0 commented May 4, 2023

Ah, yes it is. I don't think we capture flow events. There are limits on the size of files we can collect. I can a run with 'flow' logged to compare but I may have to do it manually.

@ct0br0
Copy link

ct0br0 commented May 4, 2023

As noted in other PRs, ignore the IPS section of this. Just updated baseline for that test.

@catenacyber catenacyber added needs review needs rebase Needs rebase to main labels May 12, 2023
@catenacyber
Copy link
Contributor Author

So @jufajardini pgsql probing parser to client accepts anything that has at least 5 bytes : you should uncomment the error return in pgsql_parse_response

@catenacyber
Copy link
Contributor Author

Rebased in #8892

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

Labels

needs rebase Needs rebase to main needs review

Development

Successfully merging this pull request may close these issues.

3 participants