From 6b50a71d1a118aff4fc1d745d931dfce0bc3162c Mon Sep 17 00:00:00 2001
From: Philippe Antoine <contact@catenacyber.fr>
Date: Tue, 6 Oct 2020 15:22:59 +0200
Subject: [PATCH] app-layer: lower limit for protocol detection on protocol
 change

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 | 10 ++++++++++
 src/app-layer.c              | 17 +++++++++++++++--
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/app-layer-detect-proto.c b/src/app-layer-detect-proto.c
index 9c27e0703a..8fe62ebcc8 100644
--- a/src/app-layer-detect-proto.c
+++ b/src/app-layer-detect-proto.c
@@ -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
diff --git a/src/app-layer.c b/src/app-layer.c
index c9cf0f4645..c5940e098f 100644
--- a/src/app-layer.c
+++ b/src/app-layer.c
@@ -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;