From f0c697784b4bebbae7b5db758243e4203b2b59d5 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 1 Nov 2022 15:01:52 +0100 Subject: [PATCH 1/5] protodetect: use stricter max probing parser length 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. --- src/app-layer-detect-proto.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c index 863fb920afd6..e7793ec50d25 100644 --- a/src/app-layer-detect-proto.c +++ b/src/app-layer-detect-proto.c @@ -287,6 +287,10 @@ static inline int PMGetProtoInspect(AppLayerProtoDetectThreadCtx *tctx, uint8_t pm_results_bf[(ALPROTO_MAX / 8) + 1]; 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. + uint32_t pp_max_len = pm_ctx->mpm_ctx.maxdepth; + /* loop through unique pattern id's. Can't use search_cnt here, * as that contains all matches, tctx->pmq.pattern_id_array_cnt * contains only *unique* matches. */ @@ -296,6 +300,9 @@ static inline int PMGetProtoInspect(AppLayerProtoDetectThreadCtx *tctx, AppProto proto = AppLayerProtoDetectPMMatchSignature( s, tctx, f, flags, buf, buflen, searchlen, rflow); + if (s->pp_max_depth > pp_max_len) { + pp_max_len = s->pp_max_depth; + } /* store each unique proto once */ if (AppProtoIsValid(proto) && !(pm_results_bf[proto / 8] & (1 << (proto % 8))) ) @@ -306,7 +313,7 @@ static inline int PMGetProtoInspect(AppLayerProtoDetectThreadCtx *tctx, s = s->next; } } - if (pm_matches == 0 && buflen >= pm_ctx->pp_max_len) { + if (pm_matches == 0 && buflen >= pp_max_len) { pm_matches = -2; } PmqReset(&tctx->pmq); From 9e1a4164f53308b64d50e86fc491d3375e906e10 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 10:51:54 +0100 Subject: [PATCH 2/5] flow/stats: count app-layer when error occurs 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. --- src/app-layer.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/app-layer.c b/src/app-layer.c index 0e244caf2039..9748938d022a 100644 --- a/src/app-layer.c +++ b/src/app-layer.c @@ -779,6 +779,9 @@ int AppLayerHandleTCPData(ThreadVars *tv, TcpReassemblyThreadCtx *ra_ctx, if (r != 1) { StreamTcpUpdateAppLayerProgress(ssn, direction, data_len); if (r < 0) { + if (f->alproto_tc == ALPROTO_UNKNOWN || f->alproto_ts == ALPROTO_UNKNOWN) { + AppLayerIncFlowCounter(tv, f); + } ExceptionPolicyApply( p, g_applayerparser_error_policy, PKT_DROP_REASON_APPLAYER_ERROR); SCReturnInt(-1); From e0ae203ac280d3bf96a403d894df5943cfdf50ac Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:48:20 +0100 Subject: [PATCH 3/5] protodetect: finish probing parser asap 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. --- src/app-layer-detect-proto.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c index e7793ec50d25..21b934ef21e1 100644 --- a/src/app-layer-detect-proto.c +++ b/src/app-layer-detect-proto.c @@ -620,6 +620,10 @@ static AppProto AppLayerProtoDetectPPGetProto(Flow *f, const uint8_t *buf, uint3 mask = pp_port_dp->alproto_mask; else if (pp_port_sp) mask = pp_port_sp->alproto_mask; + else { + // only pe0 + mask = pe0->alproto_mask; + } if (alproto_masks[0] == mask) { FLOW_SET_PP_DONE(f, dir); From aaf843b7142373ebb6296a0ba72b1629a798c942 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:12:08 +0100 Subject: [PATCH 4/5] pop3: protocol detection --- doc/userguide/rules/differences-from-snort.rst | 1 + doc/userguide/rules/intro.rst | 1 + etc/schema.json | 9 +++++++++ src/app-layer-detect-proto.c | 4 ++++ src/app-layer-parser.c | 12 ++++++++++++ src/app-layer-protos.c | 5 +++++ src/app-layer-protos.h | 1 + suricata.yaml.in | 2 ++ 8 files changed, 35 insertions(+) diff --git a/doc/userguide/rules/differences-from-snort.rst b/doc/userguide/rules/differences-from-snort.rst index 8226e3a7e899..8387cf77a014 100644 --- a/doc/userguide/rules/differences-from-snort.rst +++ b/doc/userguide/rules/differences-from-snort.rst @@ -19,6 +19,7 @@ Automatic Protocol Detection - dns - http - imap (detection only by default; no parsing) + - pop3 (detection only by default; no parsing) - ftp - modbus (disabled by default; minimalist probe parser; can lead to false positives) - smb diff --git a/doc/userguide/rules/intro.rst b/doc/userguide/rules/intro.rst index 6b0ac46961d1..ab46a362e409 100644 --- a/doc/userguide/rules/intro.rst +++ b/doc/userguide/rules/intro.rst @@ -94,6 +94,7 @@ you can pick from. These are: * ssh * smtp * imap +* pop3 * modbus (disabled by default) * dnp3 (disabled by default) * enip (disabled by default) diff --git a/etc/schema.json b/etc/schema.json index 017b709c1490..a87a27eec90a 100644 --- a/etc/schema.json +++ b/etc/schema.json @@ -3818,6 +3818,9 @@ "pgsql": { "$ref": "#/$defs/stats_applayer_error" }, + "pop3": { + "$ref": "#/$defs/stats_applayer_error" + }, "quic": { "$ref": "#/$defs/stats_applayer_error" }, @@ -3935,6 +3938,9 @@ "pgsql": { "type": "integer" }, + "pop3": { + "type": "integer" + }, "quic": { "type": "integer" }, @@ -4046,6 +4052,9 @@ "pgsql": { "type": "integer" }, + "pop3": { + "type": "integer" + }, "quic": { "type": "integer" }, diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c index 21b934ef21e1..60ca2bc01c5c 100644 --- a/src/app-layer-detect-proto.c +++ b/src/app-layer-detect-proto.c @@ -885,6 +885,8 @@ static void AppLayerProtoDetectPrintProbingParsers(AppLayerProtoDetectProbingPar printf(" alproto: ALPROTO_SSH\n"); else if (pp_pe->alproto == ALPROTO_IMAP) printf(" alproto: ALPROTO_IMAP\n"); + else if (pp_pe->alproto == ALPROTO_POP3) + printf(" alproto: ALPROTO_POP3\n"); else if (pp_pe->alproto == ALPROTO_JABBER) printf(" alproto: ALPROTO_JABBER\n"); else if (pp_pe->alproto == ALPROTO_SMB) @@ -968,6 +970,8 @@ static void AppLayerProtoDetectPrintProbingParsers(AppLayerProtoDetectProbingPar printf(" alproto: ALPROTO_SSH\n"); else if (pp_pe->alproto == ALPROTO_IMAP) printf(" alproto: ALPROTO_IMAP\n"); + else if (pp_pe->alproto == ALPROTO_POP3) + printf(" alproto: ALPROTO_POP3\n"); else if (pp_pe->alproto == ALPROTO_JABBER) printf(" alproto: ALPROTO_JABBER\n"); else if (pp_pe->alproto == ALPROTO_SMB) diff --git a/src/app-layer-parser.c b/src/app-layer-parser.c index 6b93a0faf0ba..b17049dd5128 100644 --- a/src/app-layer-parser.c +++ b/src/app-layer-parser.c @@ -1806,6 +1806,18 @@ void AppLayerParserRegisterProtocolParsers(void) "imap"); } + /** POP3 */ + AppLayerProtoDetectRegisterProtocol(ALPROTO_POP3, "pop3"); + if (AppLayerProtoDetectConfProtoDetectionEnabled("tcp", "pop3")) { + if (AppLayerProtoDetectPMRegisterPatternCS( + IPPROTO_TCP, ALPROTO_POP3, "+OK ", 4, 0, STREAM_TOCLIENT) < 0) { + SCLogInfo("pop3 proto registration failure"); + exit(EXIT_FAILURE); + } + } else { + SCLogInfo("Protocol detection and parser disabled for pop3 protocol."); + } + ValidateParsers(); return; } diff --git a/src/app-layer-protos.c b/src/app-layer-protos.c index f944af7c438e..4ed51819c85c 100644 --- a/src/app-layer-protos.c +++ b/src/app-layer-protos.c @@ -51,6 +51,9 @@ const char *AppProtoToString(AppProto alproto) case ALPROTO_IMAP: proto_name = "imap"; break; + case ALPROTO_POP3: + proto_name = "pop3"; + break; case ALPROTO_JABBER: proto_name = "jabber"; break; @@ -169,6 +172,8 @@ AppProto StringToAppProto(const char *proto_name) return ALPROTO_SSH; if (strcmp(proto_name, "imap") == 0) return ALPROTO_IMAP; + if (strcmp(proto_name, "pop3") == 0) + return ALPROTO_POP3; if (strcmp(proto_name, "jabber") == 0) return ALPROTO_JABBER; if (strcmp(proto_name, "smb") == 0) diff --git a/src/app-layer-protos.h b/src/app-layer-protos.h index b0a5db1c8a01..ffc086bbcea5 100644 --- a/src/app-layer-protos.h +++ b/src/app-layer-protos.h @@ -60,6 +60,7 @@ enum AppProtoEnum { ALPROTO_RDP, ALPROTO_HTTP2, ALPROTO_BITTORRENT_DHT, + ALPROTO_POP3, // signature-only (ie not seen in flow) // HTTP for any version (ALPROTO_HTTP1 (version 1) or ALPROTO_HTTP2) diff --git a/suricata.yaml.in b/suricata.yaml.in index da13df91a770..e2e392c1eaa3 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -963,6 +963,8 @@ app-layer: content-inspect-window: 4096 imap: enabled: detection-only + pop3: + enabled: detection-only smb: enabled: yes detection-ports: From 708c05783ec0b2020873ee5fb85786a14592a8df Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:03:56 +0100 Subject: [PATCH 5/5] ftp: protocol detection avoiding FP on POP3 --- src/app-layer-ftp.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/app-layer-ftp.c b/src/app-layer-ftp.c index 4f7ed0b16baf..e6397a3041c0 100644 --- a/src/app-layer-ftp.c +++ b/src/app-layer-ftp.c @@ -949,6 +949,15 @@ static int FTPGetAlstateProgress(void *vtx, uint8_t direction) return FTP_STATE_FINISHED; } +static AppProto FTPUserProbingParser( + Flow *f, uint8_t direction, const uint8_t *input, uint32_t len, uint8_t *rdir) +{ + if (f->alproto_tc == ALPROTO_POP3) { + // POP traffic begins by same "USER" pattern as FTP + return ALPROTO_FAILED; + } + return ALPROTO_FTP; +} static int FTPRegisterPatternsForProtocolDetection(void) { @@ -960,8 +969,8 @@ static int FTPRegisterPatternsForProtocolDetection(void) IPPROTO_TCP, ALPROTO_FTP, "FEAT", 4, 0, STREAM_TOSERVER) < 0) { return -1; } - if (AppLayerProtoDetectPMRegisterPatternCI( - IPPROTO_TCP, ALPROTO_FTP, "USER ", 5, 0, STREAM_TOSERVER) < 0) { + if (AppLayerProtoDetectPMRegisterPatternCSwPP(IPPROTO_TCP, ALPROTO_FTP, "USER ", 5, 0, + STREAM_TOSERVER, FTPUserProbingParser, 5, 5) < 0) { return -1; } if (AppLayerProtoDetectPMRegisterPatternCI(