]> git.ipfire.org Git - thirdparty/suricata.git/commitdiff
app-layer: lower limit for protocol detection on protocol change
authorPhilippe Antoine <contact@catenacyber.fr>
Tue, 6 Oct 2020 13:22:59 +0000 (15:22 +0200)
committerVictor Julien <victor@inliniac.net>
Mon, 16 Nov 2020 13:23:59 +0000 (14:23 +0100)
So that protocol detection does not run for too long because
TCPProtoDetectCheckBailConditions somehow relies on its TCP stream
to start from zero, which is not the case on protocol change

Adds also debug validation checks, such as
both sides are known on protocol change

And only sets once alproto_orig

src/app-layer-detect-proto.c
src/app-layer.c

index 9c27e0703a38a9a6ab31141432a718c053e9a756..8fe62ebcc82d5eede381fe8ea8d727459df3ceaf 100644 (file)
@@ -61,6 +61,7 @@
 #include "util-memcmp.h"
 #include "util-spm.h"
 #include "util-debug.h"
+#include "util-validate.h"
 
 #include "runmodes.h"
 
@@ -1901,6 +1902,15 @@ void AppLayerRequestProtocolChange(Flow *f, uint16_t dp, AppProto expect_proto)
     FlowSetChangeProtoFlag(f);
     f->protodetect_dp = dp;
     f->alproto_expect = expect_proto;
+    DEBUG_VALIDATE_BUG_ON(f->alproto == ALPROTO_UNKNOWN);
+    f->alproto_orig = f->alproto;
+    // If one side is unknown yet, set it to the other known side
+    if (f->alproto_ts == ALPROTO_UNKNOWN) {
+        f->alproto_ts = f->alproto;
+    }
+    if (f->alproto_tc == ALPROTO_UNKNOWN) {
+        f->alproto_tc = f->alproto;
+    }
 }
 
 /** \brief request applayer to wrap up this protocol and rerun protocol
index c9cf0f4645739695e04a83dca558b519f6fe4922..c5940e098ff42dc9e2d2450e649bb95afd691867 100644 (file)
@@ -70,6 +70,8 @@ struct AppLayerThreadCtx_ {
 #endif
 };
 
+#define FLOW_PROTO_CHANGE_MAX_DEPTH 4096
+
 #define MAX_COUNTER_SIZE 64
 typedef struct AppLayerCounterNames_ {
     char name[MAX_COUNTER_SIZE];
@@ -551,7 +553,18 @@ static int TCPProtoDetect(ThreadVars *tv,
             }
         } else {
             /* both sides unknown, let's see if we need to give up */
-            TCPProtoDetectCheckBailConditions(tv, f, ssn, p);
+            if (FlowChangeProto(f)) {
+                /* TCPProtoDetectCheckBailConditions does not work well because
+                 * size_tc from STREAM_RIGHT_EDGE is not reset to zero
+                 * so, we set a lower limit to the data we inspect
+                 * We could instead have set ssn->server.sb.stream_offset = 0;
+                 */
+                if (data_len >= FLOW_PROTO_CHANGE_MAX_DEPTH || (flags & STREAM_EOF)) {
+                    DisableAppLayer(tv, f, p);
+                }
+            } else {
+                TCPProtoDetectCheckBailConditions(tv, f, ssn, p);
+            }
         }
     }
     SCReturnInt(0);
@@ -619,13 +632,13 @@ int AppLayerHandleTCPData(ThreadVars *tv, TcpReassemblyThreadCtx *ra_ctx,
      * We receive 2 stream init msgs (one for each direction) but we
      * only run the proto detection once. */
     if (alproto == ALPROTO_UNKNOWN && (flags & STREAM_START)) {
+        DEBUG_VALIDATE_BUG_ON(FlowChangeProto(f));
         /* run protocol detection */
         if (TCPProtoDetect(tv, ra_ctx, app_tctx, p, f, ssn, stream,
                            data, data_len, flags) != 0) {
             goto failure;
         }
     } else if (alproto != ALPROTO_UNKNOWN && FlowChangeProto(f)) {
-        f->alproto_orig = f->alproto;
         SCLogDebug("protocol change, old %s", AppProtoToString(f->alproto_orig));
         void *alstate_orig = f->alstate;
         AppLayerParserState *alparser = f->alparser;