From: Victor Julien <vjulien@oisf.net>
Date: Fri, 6 Sep 2024 11:14:48 +0000 (+0200)
Subject: detect/app-layer-proto: don't run detection on ALPROTO_UNKNOWN
X-Git-Tag: suricata-8.0.0-beta1~893
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=refs%2Fpull%2F11732%2Fhead;p=thirdparty%2Fsuricata.git

detect/app-layer-proto: don't run detection on ALPROTO_UNKNOWN

The `app-layer-protocol` keyword inconsistently checks whether the
alproto is ALPROTO_UNKNOWN. In the regular match function it isn't
checked, in the prefilter function its checked for all but the "either"
mode.

This leads to false positives for negated matching, as an expression
like "!tls" will match if checked against ALPROTO_UNKNOWN.

This patch adds the checking everywhere. The keyword returns no match as
long as the alproto is ALPROTO_UNKNOWN.

Bug: #7241.
---

diff --git a/src/detect-app-layer-protocol.c b/src/detect-app-layer-protocol.c
index 938b96a5b4..c706e5567b 100644
--- a/src/detect-app-layer-protocol.c
+++ b/src/detect-app-layer-protocol.c
@@ -78,24 +78,38 @@ static int DetectAppLayerProtocolPacketMatch(
     switch (data->mode) {
         case DETECT_ALPROTO_DIRECTION:
             if (p->flowflags & FLOW_PKT_TOSERVER) {
+                if (f->alproto_ts == ALPROTO_UNKNOWN)
+                    SCReturnInt(0);
                 r = AppProtoEquals(data->alproto, f->alproto_ts);
             } else {
+                if (f->alproto_tc == ALPROTO_UNKNOWN)
+                    SCReturnInt(0);
                 r = AppProtoEquals(data->alproto, f->alproto_tc);
             }
             break;
         case DETECT_ALPROTO_ORIG:
+            if (f->alproto_orig == ALPROTO_UNKNOWN)
+                SCReturnInt(0);
             r = AppProtoEquals(data->alproto, f->alproto_orig);
             break;
         case DETECT_ALPROTO_FINAL:
+            if (f->alproto == ALPROTO_UNKNOWN)
+                SCReturnInt(0);
             r = AppProtoEquals(data->alproto, f->alproto);
             break;
         case DETECT_ALPROTO_TOSERVER:
+            if (f->alproto_ts == ALPROTO_UNKNOWN)
+                SCReturnInt(0);
             r = AppProtoEquals(data->alproto, f->alproto_ts);
             break;
         case DETECT_ALPROTO_TOCLIENT:
+            if (f->alproto_tc == ALPROTO_UNKNOWN)
+                SCReturnInt(0);
             r = AppProtoEquals(data->alproto, f->alproto_tc);
             break;
         case DETECT_ALPROTO_EITHER:
+            if (f->alproto_ts == ALPROTO_UNKNOWN && f->alproto_tc == ALPROTO_UNKNOWN)
+                SCReturnInt(0);
             r = AppProtoEquals(data->alproto, f->alproto_tc) ||
                 AppProtoEquals(data->alproto, f->alproto_ts);
             break;
@@ -279,9 +293,11 @@ PrefilterPacketAppProtoMatch(DetectEngineThreadCtx *det_ctx, Packet *p, const vo
         case DETECT_ALPROTO_EITHER:
             // check if either protocol toclient or toserver matches
             // the one in the signature ctx
-            if (AppProtoEquals(ctx->v1.u16[0], f->alproto_tc) ^ negated) {
+            if (f->alproto_tc != ALPROTO_UNKNOWN &&
+                    AppProtoEquals(ctx->v1.u16[0], f->alproto_tc) ^ negated) {
                 PrefilterAddSids(&det_ctx->pmq, ctx->sigs_array, ctx->sigs_cnt);
-            } else if (AppProtoEquals(ctx->v1.u16[0], f->alproto_ts) ^ negated) {
+            } else if (f->alproto_ts != ALPROTO_UNKNOWN &&
+                       AppProtoEquals(ctx->v1.u16[0], f->alproto_ts) ^ negated) {
                 PrefilterAddSids(&det_ctx->pmq, ctx->sigs_array, ctx->sigs_cnt);
             }
             // We return right away to avoid calling PrefilterAddSids again