detect: honor the P+1 eof rule for generic hook list engines

DetectEngineInspectGenericList returned NO_MATCH for every keyword miss,
also when the tx had moved past the phase the engine is registered at.
Unlike the buffer engines it never produced a terminal no match, so a
firewall LTE rule using such a keyword (tls.version on a hook) stayed
provisional and was accepted for the rest of the session.

Make a miss terminal at P + 1. This engine also backs the lua
on-request/on-response hooks and the tls_cert_*, dnp3/dnp3_ind and
ftpdata/ftp mode keywords, so a plain IDS rule using them keeps its
verdict once the tx has moved past the registered phase.

Export the parser's end state computation as AppLayerParserGetTxEndState()
here, so the version keyword can use it; the remaining call sites are
converted later.

Ticket: #8944.

rust/sys/src/sys.rs

@@ -21,6 +21,7 @@ pub const SIGMATCH_INFO_UINT32: u32 = 65536;
 pub const SIGMATCH_INFO_UINT64: u32 = 131072;
 pub const SIGMATCH_INFO_MULTI_UINT: u32 = 262144;
 pub const SIGMATCH_INFO_ENUM_UINT: u32 = 524288;
+pub const SIGMATCH_STATEFUL: u32 = 8388608;
 pub const SIGMATCH_INFO_BITFLAGS_UINT: u32 = 1048576;
 pub const SIGMATCH_BAN_FIREWALL_RULE: u32 = 2097152;
 pub const SIGMATCH_BAN_FIREWALL_MODE: u32 = 4194304;

src/app-layer-parser.c

@@ -177,7 +177,6 @@ struct AppLayerParserState_ {
     FramesContainer *frames;
 };
 
-static inline uint8_t GetTxEndProgress(uint8_t ipproto, AppProto alproto, void *tx, uint8_t flags);
 static inline uint8_t GetTxdEndProgress(uint8_t ipproto, AppProto alproto,
         const AppLayerTxData *txd, uint8_t flags, uint8_t complete);
 
@@ -1185,20 +1184,21 @@ static inline int StateGetProgressCompletionStatus(const AppProto alproto, const
  *  If the TX is supporting sub-states, return the value from the txd.
  *  \param tx pointer to the transaction
  */
-static inline uint8_t GetTxEndProgress(uint8_t ipproto, AppProto alproto, void *tx, uint8_t flags)
+uint8_t AppLayerParserGetTxEndState(uint8_t ipproto, AppProto alproto, void *tx, uint8_t flags)
 {
     const AppLayerTxData *txd = AppLayerParserGetTxData(ipproto, alproto, tx);
-    DEBUG_VALIDATE_BUG_ON(txd == NULL);
-    uint8_t tx_end_state;
-    if (txd->tx_type == 0) {
-        tx_end_state = (uint8_t)AppLayerParserGetStateProgressCompletionStatus(alproto, flags);
-    } else {
-        if (flags & STREAM_TOSERVER)
-            tx_end_state = txd->tx_type_eop_ts;
-        else
-            tx_end_state = txd->tx_type_eop_tc;
+    if (txd == NULL || txd->tx_type == 0) {
+        return (uint8_t)AppLayerParserGetStateProgressCompletionStatus(alproto, flags);
+    }
+    const uint8_t eop = (flags & STREAM_TOSERVER) ? txd->tx_type_eop_ts : txd->tx_type_eop_tc;
+    if (unlikely(eop == 0)) {
+        /* a parser with tx types must fill both end-of-phase fields; without
+         * the fallback 0 would make the first state look final in release
+         * builds */
+        DEBUG_VALIDATE_BUG_ON(1);
+        return (uint8_t)AppLayerParserGetStateProgressCompletionStatus(alproto, flags);
     }
-    return tx_end_state;
+    return eop;
 }
 
 /** \internal
@@ -1239,7 +1239,7 @@ int AppLayerParserGetStateProgress(uint8_t ipproto, AppProto alproto, void *tx,
     SCEnter();
     int r;
     if (unlikely(IS_DISRUPTED(flags))) {
-        r = (int)GetTxEndProgress(ipproto, alproto, tx, flags);
+        r = (int)AppLayerParserGetTxEndState(ipproto, alproto, tx, flags);
     } else {
         const uint8_t direction = flags & (STREAM_TOCLIENT | STREAM_TOSERVER);
         r = alp_ctx.ctxs[alproto][FlowGetProtoMapping(ipproto)].StateGetProgress(tx, direction);

src/app-layer-parser.h

@@ -71,6 +71,12 @@ typedef struct AppLayerGetFileState AppLayerGetFileState;
 
 int AppLayerParserProtoIsRegistered(uint8_t ipproto, AppProto alproto);
 
+/** \brief get the end state (progress) for a transaction.
+ *
+ *  Uses the transaction type specific end state when the parser provides one,
+ *  else the protocol completion status. Tolerates a NULL tx data pointer. */
+uint8_t AppLayerParserGetTxEndState(uint8_t ipproto, AppProto alproto, void *tx, uint8_t flags);
+
 /** progress values need to stay under this. */
 #define APP_LAYER_MAX_PROGRESS 48
 

src/app-layer-ssl.c

@@ -1624,6 +1624,9 @@ static int TLSDecodeHandshakeHello(SSLState *ssl_state,
     if (SC_ATOMIC_GET(ssl_config.enable_ja3) && ssl_state->curr_connp->ja3_hash == NULL) {
         ssl_state->curr_connp->ja3_hash = Ja3GenerateHash(ssl_state->curr_connp->ja3_str);
     }
+    /* the hello, supported_versions included, decoded cleanly: its version
+     * is final for this connp */
+    ssl_state->curr_connp->hello_decoded = true;
     return 0;
 
 fail:

src/app-layer-ssl.h

@@ -217,6 +217,11 @@ typedef struct SSLStateConnp_ {
      * ServerHello completion or a certificate header) with an
      * incomplete chain */
     bool cert_chain_final;
+    /* a hello (including its supported_versions extension) decoded cleanly:
+     * this connp's version is final from here on. A failed hello sets the
+     * legacy version but leaves this clear, so a later record must not turn
+     * a partially decoded hello into a version match */
+    bool hello_decoded;
     int64_t cert0_not_before;
     int64_t cert0_not_after;
     char *cert0_fingerprint;

src/detect-engine-prefilter.c

@@ -148,8 +148,9 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
                         tx->tx_progress, engine->ctx.app.tx_min_progress);
 
                 /* if state value is at or beyond engine state, we can skip it. It means we ran at
-                 * least once already. */
-                if (tx->detect_progress > engine->ctx.app.tx_min_progress) {
+                 * least once already. A run-always engine (stateful keyword) is revisited on
+                 * every update, so detect_progress must not skip it. */
+                if (!engine->run_always && tx->detect_progress > engine->ctx.app.tx_min_progress) {
                     SCLogDebug("tx already marked progress as beyond engine: %u > %u",
                             tx->detect_progress, engine->ctx.app.tx_min_progress);
                     goto next;
@@ -177,7 +178,13 @@ void DetectRunPrefilterTx(DetectEngineThreadCtx *det_ctx,
                  * though a parser may use 0 as a valid value. */
                 // tx->tx_ptr == tx_ptr ensures we do not use a dns engine progress
                 // to update a HTTP2 tx detect_progress in case of DOH2
-                tx->detect_progress = engine->ctx.app.tx_min_progress + 1;
+                //
+                // monotonic: a run-always engine at a lower progress is revisited
+                // after the tx moved on, and must not re-enable the engines it
+                // already completed
+                if (tx->detect_progress < engine->ctx.app.tx_min_progress + 1) {
+                    tx->detect_progress = engine->ctx.app.tx_min_progress + 1;
+                }
                 SCLogDebug("tx->tx_progress %d engine->ctx.app.tx_min_progress %d "
                            "engine->is_last_for_progress %d => tx->detect_progress updated to %02x",
                         tx->tx_progress, engine->ctx.app.tx_min_progress,
@@ -739,6 +746,10 @@ struct TxNonPFData {
     uint8_t progress; /**< progress state value to register at */
     int sig_list; /**< special handling: normally 0, but for special cases (app-layer-state,
                      app-layer-event) use the list id to create separate engines */
+    /** the buffer holds a stateful keyword: evaluate its engine on every tx
+     *  update (engine progress -1) so a provisional miss can be revisited as
+     *  the transaction advances. */
+    bool run_always;
     uint32_t sigs_cnt;
     struct PrefilterNonPFDataSig *sigs;
     const char *engine_name; /**< pointer to name owned by DetectEngineCtx::non_pf_engine_names */
@@ -747,7 +758,8 @@ struct TxNonPFData {
 static uint32_t TxNonPFHash(HashListTable *h, void *data, uint16_t _len)
 {
     struct TxNonPFData *d = data;
-    return (d->alproto + d->sub_state + d->progress + d->dir + d->sig_list) % h->array_size;
+    return (d->alproto + d->sub_state + d->progress + d->dir + d->sig_list + d->run_always) %
+           h->array_size;
 }
 
 static char TxNonPFCompare(void *data1, uint16_t _len1, void *data2, uint16_t len2)
@@ -755,7 +767,8 @@ static char TxNonPFCompare(void *data1, uint16_t _len1, void *data2, uint16_t le
     struct TxNonPFData *d1 = data1;
     struct TxNonPFData *d2 = data2;
     return d1->alproto == d2->alproto && d1->sub_state == d2->sub_state &&
-           d1->progress == d2->progress && d1->dir == d2->dir && d1->sig_list == d2->sig_list;
+           d1->progress == d2->progress && d1->dir == d2->dir && d1->sig_list == d2->sig_list &&
+           d1->run_always == d2->run_always;
 }
 
 static void TxNonPFFree(void *data)
@@ -767,7 +780,7 @@ static void TxNonPFFree(void *data)
 
 static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash,
         const AppProto alproto, const uint8_t sub_state, const int dir, const uint8_t progress,
-        const int sig_list, const char *name, const Signature *s)
+        const int sig_list, const char *name, const Signature *s, const bool run_always)
 {
     const uint32_t max_sids = DetectEngineGetMaxSigId(de_ctx);
 
@@ -777,6 +790,7 @@ static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash
         .dir = dir,
         .progress = progress,
         .sig_list = sig_list,
+        .run_always = run_always,
         .sigs_cnt = 0,
         .sigs = NULL,
         .engine_name = NULL,
@@ -809,6 +823,7 @@ static int TxNonPFAddSig(DetectEngineCtx *de_ctx, HashListTable *tx_engines_hash
     add->alproto = alproto;
     add->progress = progress;
     add->sig_list = sig_list;
+    add->run_always = run_always;
     add->sigs = SCCalloc(max_sids, sizeof(struct PrefilterNonPFDataSig));
     if (add->sigs == NULL) {
         SCFree(add);
@@ -992,7 +1007,7 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
                 const int sm_list =
                         DetectEngineAppHookToSmlist(s->alproto, sub_state, state, direction);
                 if (TxNonPFAddSig(de_ctx, tx_engines_hash, s->alproto, sub_state, dir, state,
-                            sm_list, pname, s) != 0) {
+                            sm_list, pname, s, false) != 0) {
                     goto error;
                 }
                 tx_non_pf = true;
@@ -1062,9 +1077,12 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
                     int sig_list = 0;
                     if (list_id == app_state_list_id)
                         sig_list = app_state_list_id;
+                    /* buffers marked run-always (stateful keywords) must be
+                     * revisited as the tx advances. */
+                    const bool run_always = buf != NULL && buf->run_always;
                     const uint8_t sub_state = app->sub_state;
                     if (TxNonPFAddSig(de_ctx, tx_engines_hash, app->alproto, sub_state, app->dir,
-                                app->progress, sig_list, buf->name, s) != 0) {
+                                app->progress, sig_list, buf->name, s, run_always) != 0) {
                         goto error;
                     }
                     tx_non_pf = true;
@@ -1086,8 +1104,8 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
 
             uint8_t sub_state = s->init_data->hook.t.app.sub_state;
             if (TxNonPFAddSig(de_ctx, tx_engines_hash, s->alproto, sub_state, dir,
-                        s->init_data->hook.t.app.app_progress, s->init_data->hook.sm_list, pname,
-                        s) != 0) {
+                        s->init_data->hook.t.app.app_progress, s->init_data->hook.sm_list, pname, s,
+                        false) != 0) {
                 goto error;
             }
             tx_non_pf = true;
@@ -1163,11 +1181,13 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
             continue;
         }
 
-        /* register special progress value to indicate we need to run it all the time */
         DEBUG_VALIDATE_BUG_ON(t->progress > INT8_MAX);
         int8_t engine_progress = (int8_t)t->progress;
         if (t->sig_list == app_state_list_id) {
-            SCLogDebug("engine %s for state list", t->engine_name);
+            /* app-layer-state engines use the -1 "run every update" sentinel;
+             * they are proto-agnostic (ALPROTO_UNKNOWN), which that path
+             * requires. */
+            SCLogDebug("engine %s for state list, run always", t->engine_name);
             engine_progress = -1;
         }
 
@@ -1184,6 +1204,15 @@ static int SetupNonPrefilter(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
             SCFree(data);
             goto error;
         }
+        if (t->run_always) {
+            /* A stateful keyword must be revisited as the tx advances. Keep
+             * its real progress (so the progress bookkeeping runs and the
+             * proto-agnostic -1 invariant holds) and carry an explicit flag. */
+            PrefilterEngineList *tail = sgh->init->tx_engines;
+            while (tail->next != NULL)
+                tail = tail->next;
+            tail->run_always = true;
+        }
     }
     HashListTableFree(tx_engines_hash);
     tx_engines_hash = NULL;
@@ -1360,6 +1389,7 @@ int PrefilterSetupRuleGroup(DetectEngineCtx *de_ctx, SigGroupHead *sgh)
             e->alproto = el->alproto;
             e->ctx.app.tx_min_progress = el->tx_min_progress;
             e->ctx.app.sub_state = el->sub_state;
+            e->run_always = el->run_always;
             e->cb.PrefilterTx = el->PrefilterTx;
             e->pectx = el->pectx;
             el->pectx = NULL; // e now owns the ctx

src/detect-engine-register.h

@@ -350,6 +350,10 @@ extern int DETECT_TBLSIZE_IDX;
 #define SIGMATCH_INFO_MULTI_UINT (1UL << (18))
 /** keyword is an uint with enumeration stringer */
 #define SIGMATCH_INFO_ENUM_UINT (1UL << (19))
+/** keyword matches mutable state that can change as the transaction advances
+ *  (e.g. ssl_state): a no match must stay revisitable instead of becoming
+ *  final at P + 1. */
+#define SIGMATCH_STATEFUL (1UL << (23))
 /** keyword is an uint with bitflags */
 #define SIGMATCH_INFO_BITFLAGS_UINT (1UL << (20))
 /** keyword cannot be used in firewall rules */

src/detect-engine.c

@@ -1558,6 +1558,26 @@ void DetectBufferTypeSetDescriptionByName(const char *name, const char *desc)
     strlcpy(exists->description, desc, sizeof(exists->description));
 }
 
+void DetectBufferTypeSetRunAlways(const char *name)
+{
+    DetectBufferType *exists = DetectBufferTypeLookupByName(name);
+    if (!exists) {
+        return;
+    }
+    exists->run_always = true;
+}
+
+void DetectEngineBufferTypeSetRunAlways(DetectEngineCtx *de_ctx, const int id)
+{
+    const char *name = DetectEngineBufferTypeGetNameById(de_ctx, id);
+    if (name == NULL) {
+        return;
+    }
+    DetectBufferType *exists = DetectEngineBufferTypeLookupByName(de_ctx, name);
+    BUG_ON(!exists);
+    exists->run_always = true;
+}
+
 const char *DetectEngineBufferTypeGetDescriptionById(const DetectEngineCtx *de_ctx, const int id)
 {
     const DetectBufferType *exists = DetectEngineBufferTypeGetById(de_ctx, id);
@@ -2118,6 +2138,9 @@ int DetectEngineReloadIsIdle(void)
  *  \retval 0 no match
  *  \retval 1 match
  */
+static bool DetectTxCompleted(
+        Flow *f, void *txv, uint8_t flags, const DetectEngineAppInspectionEngine *engine);
+
 uint8_t DetectEngineInspectGenericList(DetectEngineCtx *de_ctx, DetectEngineThreadCtx *det_ctx,
         const struct DetectEngineAppInspectionEngine_ *engine, const Signature *s, Flow *f,
         uint8_t flags, void *alstate, void *txv, uint64_t tx_id)
@@ -2131,8 +2154,18 @@ uint8_t DetectEngineInspectGenericList(DetectEngineCtx *de_ctx, DetectEngineThre
             match = sigmatch_table[smd->type].
                 AppLayerTxMatch(det_ctx, f, flags, alstate, txv, s, smd->ctx);
             KEYWORD_PROFILING_END(det_ctx, smd->type, (match == 1));
-            if (match == 0)
-                return DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
+            if (match == 0) {
+                /* like the buffer engines, a no match is final once the tx
+                 * moved past the phase the engine is registered at (P + 1).
+                 * stateful keywords read live tx state, so they stay
+                 * revisitable. */
+                if ((sigmatch_table[smd->type].flags & SIGMATCH_STATEFUL) != 0) {
+                    return DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
+                }
+                return DetectTxCompleted(f, txv, flags, engine)
+                               ? DETECT_ENGINE_INSPECT_SIG_CANT_MATCH
+                               : DETECT_ENGINE_INSPECT_SIG_NO_MATCH;
+            }
             if (match == 2) {
                 return DETECT_ENGINE_INSPECT_SIG_CANT_MATCH;
             }
@@ -5653,9 +5686,25 @@ static int DetectEngineThreadCtxInitKeywordFailTest(void)
 
 #endif
 
+#ifdef UNITTESTS
+/** \test SIGMATCH_STATEFUL uses a unique bit and only stateful keywords set it */
+static int DetectEngineStatefulFlagTest01(void)
+{
+    FAIL_IF((SIGMATCH_STATEFUL & SIGMATCH_INFO_BITFLAGS_UINT) != 0);
+    FAIL_IF((SIGMATCH_STATEFUL & SIGMATCH_BAN_FIREWALL_RULE) != 0);
+    FAIL_IF((SIGMATCH_STATEFUL & SIGMATCH_BAN_FIREWALL_MODE) != 0);
+
+    FAIL_IF((sigmatch_table[DETECT_SSL_STATE].flags & SIGMATCH_STATEFUL) == 0);
+    FAIL_IF((sigmatch_table[DETECT_DNP3IND].flags & SIGMATCH_STATEFUL) != 0);
+
+    PASS;
+}
+#endif
+
 void DetectEngineRegisterTests(void)
 {
 #ifdef UNITTESTS
+    UtRegisterTest("DetectEngineStatefulFlagTest01", DetectEngineStatefulFlagTest01);
     UtRegisterTest("DetectEngineTest01", DetectEngineTest01);
     UtRegisterTest("DetectEngineTest02", DetectEngineTest02);
     UtRegisterTest("DetectEngineTest03", DetectEngineTest03);

src/detect-engine.h

@@ -41,6 +41,8 @@ void DetectBufferTypeSupportsMultiInstance(const char *name);
 int DetectBufferTypeMaxId(void);
 void DetectBufferTypeCloseRegistration(void);
 void DetectBufferTypeSetDescriptionByName(const char *name, const char *desc);
+void DetectBufferTypeSetRunAlways(const char *name);
+void DetectEngineBufferTypeSetRunAlways(DetectEngineCtx *de_ctx, const int id);
 void DetectBufferTypeRegisterSetupCallback(const char *name,
         void (*Callback)(const DetectEngineCtx *, Signature *, const DetectBufferType *));
 void DetectBufferTypeRegisterValidateCallback(

src/detect-ssl-state.c

@@ -76,6 +76,8 @@ void DetectSslStateRegister(void)
     sigmatch_table[DETECT_SSL_STATE].desc = "match the state of the SSL connection";
     sigmatch_table[DETECT_SSL_STATE].url = "/rules/tls-keywords.html#ssl-state";
     sigmatch_table[DETECT_SSL_STATE].AppLayerTxMatch = DetectSslStateMatch;
+    /* the value follows the handshake state */
+    sigmatch_table[DETECT_SSL_STATE].flags |= SIGMATCH_STATEFUL;
     sigmatch_table[DETECT_SSL_STATE].Setup = DetectSslStateSetup;
     sigmatch_table[DETECT_SSL_STATE].Free = DetectSslStateFree;
 #ifdef UNITTESTS
@@ -89,6 +91,11 @@ void DetectSslStateRegister(void)
     DetectBufferTypeSetDescriptionByName("tls_generic",
             "generic ssl/tls inspection");
 
+    /* ssl_state reads live handshake state, so its engine must be revisited
+     * as the transaction advances (a miss at the started state must not be
+     * final): run it on every update. */
+    DetectBufferTypeSetRunAlways("tls_generic");
+
     DetectAppLayerInspectEngineRegister(
             "tls_generic", ALPROTO_TLS, SIG_FLAG_TOSERVER, 0, DetectEngineInspectGenericList, NULL);
     DetectAppLayerInspectEngineRegister(

src/detect-ssl-version.c

@@ -71,6 +71,9 @@ void DetectSslVersionRegister(void)
     sigmatch_table[DETECT_SSL_VERSION].AppLayerTxMatch = DetectSslVersionMatch;
     sigmatch_table[DETECT_SSL_VERSION].Setup = DetectSslVersionSetup;
     sigmatch_table[DETECT_SSL_VERSION].Free = DetectSslVersionFree;
+    /* ssl_version matches the record version, which can be seen before the
+     * hello is decoded: a miss must stay revisitable across records. */
+    sigmatch_table[DETECT_SSL_VERSION].flags |= SIGMATCH_STATEFUL;
 #ifdef UNITTESTS
     sigmatch_table[DETECT_SSL_VERSION].RegisterTests = DetectSslVersionRegisterTests;
 #endif

src/detect-tls-version.c

@@ -65,7 +65,11 @@ static int DetectTlsVersionSetup (DetectEngineCtx *, Signature *, const char *);
 static void DetectTlsVersionRegisterTests(void);
 #endif
 static void DetectTlsVersionFree(DetectEngineCtx *, void *);
-static int g_tls_generic_list_id = 0;
+
+/** buffer for the version keywords, registered at the hello states: the
+ *  version is only final once the hello has been decoded (a hello can span
+ *  several records, so the record layer version may be seen first). */
+static int g_tls_version_list_id = 0;
 
 /**
  * \brief Registration function for keyword: tls.version
@@ -78,14 +82,25 @@ void DetectTlsVersionRegister (void)
     sigmatch_table[DETECT_TLS_VERSION].AppLayerTxMatch = DetectTlsVersionMatch;
     sigmatch_table[DETECT_TLS_VERSION].Setup = DetectTlsVersionSetup;
     sigmatch_table[DETECT_TLS_VERSION].Free = DetectTlsVersionFree;
-    sigmatch_table[DETECT_TLS_VERSION].flags = SIGMATCH_SUPPORT_FIREWALL;
+    /* the negotiated version is not final until the hello is decoded (a hello
+     * can span records, and TLS 1.3 reveals the version in supported_versions),
+     * so a no match must stay revisitable until then. */
+    sigmatch_table[DETECT_TLS_VERSION].flags = SIGMATCH_SUPPORT_FIREWALL | SIGMATCH_STATEFUL;
 #ifdef UNITTESTS
     sigmatch_table[DETECT_TLS_VERSION].RegisterTests = DetectTlsVersionRegisterTests;
 #endif
 
     DetectSetupParseRegexes(PARSE_REGEX, &parse_regex);
 
-    g_tls_generic_list_id = DetectBufferTypeRegister("tls_generic");
+    g_tls_version_list_id = DetectBufferTypeRegister("tls_version");
+    DetectBufferTypeSetDescriptionByName("tls_version", "generic tls version inspection");
+    /* the negotiated version is only final once the hello decoded, so the
+     * keyword's engine must be revisited as the tx advances. */
+    DetectBufferTypeSetRunAlways("tls_version");
+    DetectAppLayerInspectEngineRegister("tls_version", ALPROTO_TLS, SIG_FLAG_TOSERVER,
+            TLS_STATE_CLIENT_HELLO, DetectEngineInspectGenericList, NULL);
+    DetectAppLayerInspectEngineRegister("tls_version", ALPROTO_TLS, SIG_FLAG_TOCLIENT,
+            TLS_STATE_SERVER_HELLO, DetectEngineInspectGenericList, NULL);
 }
 
 /**
@@ -96,8 +111,9 @@ void DetectTlsVersionRegister (void)
  * \param p pointer to the current packet
  * \param m pointer to the sigmatch that we will cast into DetectTlsVersionData
  *
- * \retval 0 no match
+ * \retval 0 no match, version not decoded yet (revisitable)
  * \retval 1 match
+ * \retval 2 no match, version decoded and different (final)
  */
 static int DetectTlsVersionMatch (DetectEngineThreadCtx *det_ctx,
         Flow *f, uint8_t flags, void *state, void *txv,
@@ -113,16 +129,43 @@ static int DetectTlsVersionMatch (DetectEngineThreadCtx *det_ctx,
     }
 
     uint16_t version = 0;
+    bool decoded = false;
     SCLogDebug("looking for tls_data->ver 0x%02X (flags 0x%02X)", tls_data->ver, flags);
 
     if (flags & STREAM_TOCLIENT) {
         version = ssl_state->server_connp.version;
-        SCLogDebug("server (toclient) version is 0x%02X", version);
+        /* the version is final only once the hello (including
+         * supported_versions) decoded: the phase can advance on a later
+         * app-data record or EOF even when the hello never decoded */
+        decoded = ssl_state->server_connp.hello_decoded;
+        SCLogDebug("server (toclient) version is 0x%02X decoded %s", version, BOOL2STR(decoded));
     } else if (flags & STREAM_TOSERVER) {
-        version =  ssl_state->client_connp.version;
-        SCLogDebug("client (toserver) version is 0x%02X", version);
+        version = ssl_state->client_connp.version;
+        decoded = ssl_state->client_connp.hello_decoded;
+        SCLogDebug("client (toserver) version is 0x%02X decoded %s", version, BOOL2STR(decoded));
     }
 
+    if (!decoded) {
+        /* the hello (or its supported_versions) is not decoded yet: the
+         * version seen so far (record layer or legacy hello field) is not
+         * final, so the miss must stay revisitable. */
+        SCReturnInt(0);
+    }
+
+    /* The rule's phase: hook rules use the hook's progress, non-hook rules
+     * inspect the direction's hello state. A rule must not match before its
+     * phase. */
+    const uint8_t engine_progress =
+            (tls_data->hook_progress >= 0)
+                    ? (uint8_t)tls_data->hook_progress
+                    : ((flags & STREAM_TOCLIENT) ? (uint8_t)TLS_STATE_SERVER_HELLO
+                                                 : (uint8_t)TLS_STATE_CLIENT_HELLO);
+    const int progress = AppLayerParserGetStateProgress(f->proto, f->alproto, txv, flags);
+    if (progress < 0)
+        SCReturnInt(0);
+    if (progress < engine_progress)
+        SCReturnInt(0);
+
     if ((tls_data->flags & DETECT_TLS_VERSION_FLAG_RAW) == 0) {
         /* Match all TLSv1.3 drafts as TLSv1.3 */
         if (((version >> 8) & 0xff) == 0x7f) {
@@ -130,7 +173,17 @@ static int DetectTlsVersionMatch (DetectEngineThreadCtx *det_ctx,
         }
     }
 
-    SCReturnInt((tls_data->ver == version));
+    if (tls_data->ver == version)
+        SCReturnInt(1);
+
+    /* Decoded but a different version: the mismatch is final only once the
+     * transaction moved past the rule's phase (or reached its end state), so
+     * a rule hooked at a state keeps the same decision point it had before. */
+    if (progress > engine_progress ||
+            progress == AppLayerParserGetTxEndState(f->proto, f->alproto, txv, flags)) {
+        SCReturnInt(2);
+    }
+    SCReturnInt(0);
 }
 
 /**
@@ -235,11 +288,19 @@ static int DetectTlsVersionSetup (DetectEngineCtx *de_ctx, Signature *s, const c
         return -1;
 
     /* keyword supports multiple hooks, so attach to the hook specified in the rule. */
-    int list = g_tls_generic_list_id;
+    int list = g_tls_version_list_id;
+    tls->hook_progress = -1;
     /* Okay so far so good, lets get this into a SigMatch
      * and put it in the Signature. */
     if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_APP) {
         list = s->init_data->hook.sm_list;
+        tls->hook_progress = (int8_t)s->init_data->hook.t.app.app_progress;
+        /* A hook at the first state (e.g. tls:client_started) is evaluated
+         * before the hello decoded and would not be revisited once the tx
+         * advances: run its engine on every update. Later hooks are revisited
+         * by the normal P+1 evaluation. */
+        if (tls->hook_progress == 0)
+            DetectEngineBufferTypeSetRunAlways(de_ctx, list);
     }
 
     if (SCSigMatchAppendSMToList(de_ctx, s, DETECT_TLS_VERSION, (SigMatchCtx *)tls, list) == NULL) {

src/detect-tls-version.h

@@ -29,6 +29,9 @@
 typedef struct DetectTlsVersionData_ {
     uint16_t ver; /** tls version to match */
     uint8_t flags;
+    /** for hook rules: progress of the hook the rule is attached to. -1 for
+     *  non-hook rules, which inspect the hello state. */
+    int8_t hook_progress;
 } DetectTlsVersionData;
 
 /* prototypes */

src/detect.c

@@ -1302,6 +1302,10 @@ void *DetectGetInnerTx(void *tx_ptr, AppProto alproto, AppProto engine_alproto,
  *  \retval  1 sig matched
  *  \retval  0 partial incomplete match
  *  \retval -1 failed to match
+ *  \retval -2 no match yet, but not final: the MPM engine will revisit the
+ *              tx as it progresses. The caller must not apply a hook default
+ *              policy for it: that could decide the packet before the revisit
+ *              can turn it into a match.
  */
 static int DetectRunTxInspectRule(ThreadVars *tv, DetectEngineCtx *de_ctx,
         DetectEngineThreadCtx *det_ctx, Packet *p, Flow *f,

src/detect.h

@@ -459,6 +459,9 @@ typedef struct DetectBufferType_ {
     bool mpm;
     bool packet; /**< compat to packet matches */
     bool frame;  /**< is about Frame inspection */
+    /** the buffer's keywords can change as the transaction advances: run
+     *  the inspect engine on every tx update instead of only at its progress. */
+    bool run_always;
     bool supports_transforms;
     bool multi_instance; /**< buffer supports multiple buffer instances per tx */
     void (*SetupCallback)(
@@ -1631,6 +1634,11 @@ typedef struct PrefilterEngineList_ {
     /** Free function for pectx data. If NULL the memory is not freed. */
     void (*Free)(void *pectx);
 
+    /** Run this tx engine on every tx update, regardless of tx progress.
+     *  Only used with Tx Engines. The engine keeps its real tx_min_progress,
+     *  so it still takes part in the progress bookkeeping. */
+    bool run_always;
+
     const char *name;
     /* global id for this prefilter */
     uint32_t gid;
@@ -1659,6 +1667,11 @@ typedef struct PrefilterEngine_ {
     bool is_last;
     bool is_last_for_progress;
 
+    /** Tx engine must run on every tx update even once the tx progressed past
+     *  its tx_min_progress. Kept separate from tx_min_progress (-1 is the
+     *  proto-agnostic packet-style sentinel). */
+    bool run_always;
+
     /** Context for matching. Might be MpmCtx for MPM engines, other ctx'
      *  for other engines. */
     void *pectx;