Skip to content

Smtp server detection 1125 v13#8512

Closed
catenacyber wants to merge 8 commits intoOISF:masterfrom
catenacyber:smtp-server-detection-1125-v13
Closed

Smtp server detection 1125 v13#8512
catenacyber wants to merge 8 commits intoOISF:masterfrom
catenacyber:smtp-server-detection-1125-v13

Conversation

@catenacyber
Copy link
Contributor

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

Describe changes:

  • smtp : adds server side detection
  • ftp : adds server side detection
  • protocoldetect: use stricter max depth
  • pop3: add detection

The most special trick is that the (server) probing parser waits for the client side to have seen some data to take a definitive positive decision.
So that If it looks like a SMTP server (it could be a FTP server), let's see if the client looks like SMTP or FTP or something unknown...

Modifies #8448 by

  • adding a commit for better stats about app-layer protocols for flow
  • adding pop3 detection

QA results analysis :

  • There is one ftp flow which is not counted in the stats, hence the QA difference for TREX_GENERIC_stats_chk
  • The flow probing parser is never stopped because of a wrong mask setting even if everything is done
  • But it is a POP flow on port 110, so it should not be counter even if it is by master

Draft :
I think this PR should be split

  • add pop3 detection support (which should change QA results) cf Pop3 parser 3243 v1 #8163 cc @exlasavage
  • fix generic protocol detection bugs
  • add smtp and ftp probing parser

But the generic protocol detection bugs only make a difference (as far as I know) when adding the legit SMTP/FTP probing parsers

What are your thoughts on this ?

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

WARNING:

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%
IPS_AFP_stats_chk
.tcp.ssn_from_cache 327942 310867 94.79%
.app_layer.flow.ftp 33480 1080 3.23%
.app_layer.tx.ftp 131760 2160 1.64%
.app_layer.error.ftp.parser 32400 0 -

Pipeline 12216

@catenacyber catenacyber added this to the 8.0 milestone Feb 2, 2023
so as to allow ' in github discussion and get CI green
@codecov
Copy link

codecov bot commented Feb 2, 2023

Codecov Report

Merging #8512 (1584bec) into master (d9e6301) will increase coverage by 0.17%.
The diff coverage is 77.70%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #8512      +/-   ##
==========================================
+ Coverage   81.84%   82.01%   +0.17%     
==========================================
  Files         967      967              
  Lines      278343   278451     +108     
==========================================
+ Hits       227799   228369     +570     
+ Misses      50544    50082     -462     
Flag Coverage Δ
fuzzcorpus 64.22% <80.53%> (+0.29%) ⬆️
suricata-verify 59.90% <65.48%> (+0.24%) ⬆️
unittests 63.34% <45.94%> (-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.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%
IPS_AFP_stats_chk
.tcp.ssn_from_cache 327942 311072 94.86%
.app_layer.flow.ftp 33480 1080 3.23%
.app_layer.tx.ftp 131760 2160 1.64%
.app_layer.error.ftp.parser 32400 0 -

Pipeline 12239

@catenacyber
Copy link
Contributor Author

TODO : investigate QA ftp diff

@catenacyber
Copy link
Contributor Author

Replaced by #8696

@catenacyber catenacyber closed this Apr 7, 2023
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