From 991ed81c480ff61b66ea4e51db832b46015ccf80 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Tue, 1 Nov 2022 15:01:52 +0100 Subject: [PATCH 1/6] 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 3a4e0574d1eff420095418fa7e46bf7e71d4b0fe Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 10:51:54 +0100 Subject: [PATCH 2/6] 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 035b349485e96e9b9f220e74c8899ea2e320399d Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:48:20 +0100 Subject: [PATCH 3/6] 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 e68207ffc4ecd3b13ef196c8e5b6a89aab7888a2 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:12:08 +0100 Subject: [PATCH 4/6] 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 9638a25d08e6..7afb1354050e 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 557db8817abf..332f557df372 100644 --- a/doc/userguide/rules/intro.rst +++ b/doc/userguide/rules/intro.rst @@ -96,6 +96,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 0321e2df82cc..2a69be629ed7 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 f27a6f1480aab938b5511a6f96f8d0a1873ff710 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 2 Feb 2023 11:03:56 +0100 Subject: [PATCH 5/6] 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 a47b0502ed59..e92fc22e0214 100644 --- a/src/app-layer-ftp.c +++ b/src/app-layer-ftp.c @@ -952,6 +952,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) { @@ -963,8 +972,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( From 97ff54f68fb19ba0140a562818c939425982b975 Mon Sep 17 00:00:00 2001 From: Philippe Antoine Date: Thu, 4 May 2023 11:42:52 +0200 Subject: [PATCH 6/6] flow/stats: count app-layer for detection-only protocols As these have no StateAlloc, AppLayerParserParse disables app-layer parsing, which prevents other side protocol detection... --- src/app-layer.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/app-layer.c b/src/app-layer.c index 9748938d022a..12c8cbce2c5d 100644 --- a/src/app-layer.c +++ b/src/app-layer.c @@ -514,6 +514,10 @@ static int TCPProtoDetect(ThreadVars *tv, if (r < 0) { goto parser_error; } + // If AppLayerParserParse disabled us (because of detection-only) + if (ssn->flags & STREAMTCP_FLAG_APP_LAYER_DISABLED) { + AppLayerIncFlowCounter(tv, f); + } } else { /* if the ssn is midstream, we may end up with a case where the * start of an HTTP request is missing. We won't detect HTTP based