detect/firewall: apply accept if last tx was skipped

In firewall mode, a accept:hook or accept:tx needs to lead to a accept
packet when the action is applied to the last TX. For this the code
relied of the `DetectTransaction::is_last` field, where the assumption
was that there would always be an inspection on the last transaction.
This assumption was wrong however, as transactions can be skipped for a
few reasons: not updated, fully inspected, unidirectional for the other
direction. This would cause the accept not be applied to the packet,
leading to a default drop.

The reason this wasn't noticed before is that until now the work had
focused on protocols that used sequential transactions (http/tls),
and/or short lived sequential unidir transactions (dns)

This patch addresses the issue by making a simple assumption: if the
last available transaction in the main detection loop is skipped, it
means it has been accepted before. Therefore we can apply the "accept
packet" logic in this case.

Bug: #8698.
(cherry picked from commit 9e31a21aba)

src/detect.c

@@ -2279,6 +2279,9 @@ static void DetectRunTx(ThreadVars *tv,
 
     uint32_t tx_inspected = 0;
     const bool have_fw_rules = EngineModeIsFirewall();
+    /* if we skipped the last tx, we did not have a chance to apply a fw accept to the packet. Since
+     * the tx is skipped, we should consider it accepted. */
+    bool last_tx_skipped = false;
 
     SCLogDebug("packet %" PRIu64, p->pcap_cnt);
     SCLogDebug("total_txs %" PRIu64, total_txs);
@@ -2295,7 +2298,7 @@ static void DetectRunTx(ThreadVars *tv,
         if (tx.tx_ptr == NULL) {
             SCLogDebug("%p/%"PRIu64" no transaction to inspect",
                     tx.tx_ptr, tx_id_min);
-
+            last_tx_skipped = !ires.has_next;
             tx_id_min++; // next (if any) run look for +1
             goto next;
         }
@@ -2304,6 +2307,7 @@ static void DetectRunTx(ThreadVars *tv,
         tx_inspected++;
 
         SCLogDebug("%p/%" PRIu64 " txd flags %02x", tx.tx_ptr, tx.tx_id, tx.tx_data_ptr->flags);
+        SCLogDebug("%p/%" PRIu64 " is_last %s", tx.tx_ptr, tx.tx_id, BOOL2STR(tx.is_last));
 
         det_ctx->tx_id = tx.tx_id;
         det_ctx->tx_id_set = true;
@@ -2629,10 +2633,18 @@ static void DetectRunTx(ThreadVars *tv,
     }
 
     SCLogDebug("packet %" PRIu64 ": tx_inspected %u", p->pcap_cnt, tx_inspected);
-    /* if all tables have been bypassed, we accept:packet */
-    if (tx_inspected == 0 && have_fw_rules) {
-        SCLogDebug("default accept: no app inspect performed");
-        DetectRunAppendDefaultAccept(det_ctx, p);
+    if (have_fw_rules) {
+        if (tx_inspected == 0) {
+            /* if all tables have been bypassed, we accept:packet */
+            SCLogDebug("default accept: no app inspect performed");
+            DetectRunAppendDefaultAccept(det_ctx, p);
+        } else if (last_tx_skipped) {
+            /* if the last tx was skipped, we need to apply accept:packet */
+            // TODO should we check drops first?
+            DEBUG_VALIDATE_BUG_ON(p->action & ACTION_DROP);
+            SCLogDebug("default accept: last tx skipped");
+            DetectRunAppendDefaultAccept(det_ctx, p);
+        }
     }
 }