detect: allow bypass keyword for fw rules in firewall mode (only) - v6

doc/userguide/firewall/firewall-banned-keywords.rst

@@ -0,0 +1,43 @@
+Firewall Mode Banned Rules
+==========================
+
+.. note:: In Suricata 8 the firewall mode is experimental and subject to change.
+
+Certain rule keywords are banned in firewall mode, either completely or based
+on rulesets.
+
+A rule may be banned from firewall rules, threat detection rules, or from firewall mode altogether.
+
+This is done when there is a chance that the keyword would lead to the firewall
+verdict being skipped, or contradicted, or when there could happen inconsistent
+states from interactions between firewall and threat detection rules matching.
+
+.. note:: Future support may be added to keywords in upcoming releases, in
+    certain cases.
+
+Banned Keywords
+---------------
+
+Firewall Mode
+~~~~~~~~~~~~~
+
+* Replace
+
+Firewall Rules
+~~~~~~~~~~~~~~
+
+* Threshold
+
+``threshold`` is not currently supported for firewall rules: some of its types
+withhold a rule's alert while still applying its actions, which would leave the
+firewall verdict and the logged record disagreeing. The restriction also covers
+the ``threshold.config`` file, whose entries may not name a firewall rule.
+
+``detection_filter`` is not restricted. It either applies a rule or it does not,
+so below its rate a firewall rule simply does not take effect and evaluation
+continues with the next rule. See :doc:`../rules/thresholding`.
+
+Threat Detection Rules
+~~~~~~~~~~~~~~~~~~~~~~
+
+* Bypass

doc/userguide/firewall/firewall-design.rst

@@ -31,12 +31,16 @@ counted as ``ips.accepted``. If it was dropped by firewall, only ``firewall.bloc
 will be incremented. No ``ips.*`` counter will be updated as conceptually the TD
 instance won't have seen the packet.
 
+.. note:: If a firewall rule uses the :ref:`bypass keyword<bypass-keyword>`, an
+   accepted packet will not be passed along to the TD step of the pipeline.
+
 Tables
 ------
 
 A ``table`` is a collection of rules with different properties. These tables are built-in.
 No custom tables can be created. Tables are available within the scope of packet layer
-and application layer (if available). Each rule can define its own :ref:`action scope<ips_action_scopes>`.
+and application layer (if available). Each rule can define its own
+:ref:`action scope<ips_action_scopes>`.
 
 Packet layer tables
 ~~~~~~~~~~~~~~~~~~~

doc/userguide/firewall/firewall-stats.rst

@@ -14,14 +14,25 @@ Statistics counters for the firewall mode cover:
 These will be present in the stats logs if the engine is run in firewall mode,
 only.
 
+Bypassed packets
+================
+
+As the firewall bypass does not happen as the primary action in a firewall
+policy/rule, the stats counters for bypassed packets continue to be the ones
+that already exist. A `bypassed` packet will be counted as an `accepted` packet
+in firewall stats.
+
 Drop reasons
 ============
 
-If a drop was caused by the firewall, the corresponding counter will be incremented. The existing ones are:
+If a drop was caused by the firewall, the corresponding counter will be
+incremented. The existing ones are:
 
     - ``rules``: a firewall rule triggered the drop
-    - ``default_packet_policy``: drop caused by the default fail closed firewall behavior, on the packet hook level
-    - ``default_app_policy``: drop caused by the default fail close firewall behavior, on the app-layer hook level
+    - ``default_packet_policy``: drop caused by the default fail closed firewall
+      behavior, on the packet hook level
+    - ``default_app_policy``: drop caused by the default fail close firewall
+      behavior, on the app-layer hook level
     - ``pre_flow_hook``: drop caused by the pre-flow hook
     - ``pre_stream_hook``: drop caused by the pre-stream hook
     - ``flow_drop``: the whole flow was dropped after a firewall action.

doc/userguide/firewall/index.rst

@@ -6,3 +6,4 @@ Firewall Mode
    firewall-design
    firewall-example
    firewall-stats
+   firewall-banned-keywords

doc/userguide/rules/bypass-keyword.rst

@@ -1,3 +1,5 @@
+.. _bypass-keyword:
+
 Bypass Keyword
 ==============
 
@@ -13,8 +15,11 @@ The ``bypass`` keyword is considered a post-match keyword.
 
 .. note::
 
-   ``bypass`` cannot be used in firewall mode, not even with Threat Detection
-   rules, as this could lead to bypassing the firewall altogether.
+    In firewall mode, ``bypass`` can only be used in firewall rules. If a threat
+    detection rule uses the ``bypass`` keyword and you want to run Suricata in
+    the offending rule will produce an error and won't be loaded. This is to
+    prevent a threat detection rule from bypassing the firewall altogether.
+    (To make the engine error out in such a case, use ``--init-errors-fatal``).
 
 bypass
 ------
@@ -26,3 +31,47 @@ Bypass a flow on matching http traffic.
   alert http any any -> any any (http.host; \
   content:"suricata.io"; :example-rule-emphasis:`bypass;` \
   sid:10001; rev:1;)
+
+Firewall mode
+-------------
+
+``bypass`` is only accepted with a specific combination of `action` and `scope`:
+``accept:flow``.
+
+Not accepted:
+    - Action: ``config``
+    - Action: ``reject``
+    - Action: ``drop``
+    - Scope: ``packet``
+    - Scope: ``tx``
+    - Scope: ``hook``
+
+.. attention:: `bypass` on a firewall rule is a terminating action. Threat
+  detection rules are not evaluated for the matching packet, respecting the
+  premise of what would happen if Firewall and IPS were two separate devices.
+
+.. note:: The type of bypass will depend on whether the engine is configured
+  for local or capture bypass: offloading is not guaranteed by a firewall
+  bypass rule.
+
+Valid firewall rule with bypass:
+
+.. container:: example-rule
+
+  :example-rule-emphasis:`accept:flow,alert` http1:request_headers any any -> \
+  any any (http.host; content:"suricata.io"; :example-rule-emphasis:`bypass;` \
+  sid:10001; rev:1;)
+
+
+Special hooks
+~~~~~~~~~~~~~
+
+``pre_flow`` hook
+^^^^^^^^^^^^^^^^^
+
+If the ``bypass`` is applied locally, ``pre_flow`` rules will still be processed
+and invoked, due to the fact that the engine can't apply nor control a flow
+bypass at a stage where the packet hasn't been tied to its flow yet.
+
+This won't happen in the case of an offloaded bypass, as there won't be
+anything for the engine to inspect against.

doc/userguide/rules/thresholding.rst

@@ -19,6 +19,19 @@ Thresholds are tracked in a hash table that is sized according to configuration,
   Rule actions ``drop`` (IPS mode) and ``reject`` are applied to each packet
   (not only the one that meets the limit condition).
 
+.. note::
+
+   In firewall mode, the ``threshold`` keyword is not allowed in firewall
+   rules, and ``threshold.config`` entries may not name a firewall rule's
+   signature id.
+   Either leads to a fatal error on engine initialization when
+   ``--init-errors-fatal`` is set, rather than silently ignoring the entry.
+   Without that option, and on a rule reload, the entry is skipped with a
+   warning. Firewall rules matched by a wildcard ``threshold.config`` entry are
+   always skipped with a warning summarizing how many rules were skipped.
+   ``detection_filter`` is allowed in firewall rules. Below its rate the rule
+   does not apply.
+
 threshold
 ---------
 

doc/userguide/upgrade.rst

@@ -77,6 +77,11 @@ Keyword Changes
   being split per direction. This means that some rules should match sooner,
   some rules will have less false negatives, and some rules will trigger once per transaction
   instead of twice (one time for each direction)
+- Thresholding is restricted in firewall mode. The ``threshold`` keyword is
+  rejected in firewall rules, and ``threshold.config`` entries (``threshold``,
+  ``event_filter``, ``suppress``, ``rate_filter``) may no longer name a firewall
+  rule's signature id. Wildcard ``threshold.config`` entries skip firewall
+  rules. ``detection_filter`` and Threat Detection rules are unaffected.
 
 Other Changes
 ~~~~~~~~~~~~~

rust/sys/src/sys.rs

@@ -24,6 +24,13 @@ pub const SIGMATCH_INFO_ENUM_UINT: u32 = 524288;
 pub const SIGMATCH_INFO_BITFLAGS_UINT: u32 = 1048576;
 pub const SIGMATCH_BAN_FIREWALL_RULE: u32 = 2097152;
 pub const SIGMATCH_BAN_FIREWALL_MODE: u32 = 4194304;
+pub const SIGMATCH_BAN_TD_FIREWALL_MODE: u32 = 8388608;
+pub const SIGMATCH_BAN_FIREWALL_SCOPE_PACKET: u32 = 16777216;
+pub const SIGMATCH_BAN_FIREWALL_SCOPE_TX: u32 = 33554432;
+pub const SIGMATCH_BAN_FIREWALL_SCOPE_HOOK: u32 = 67108864;
+pub const SIGMATCH_BAN_ACTION_CONFIG: u32 = 134217728;
+pub const SIGMATCH_BAN_ACTION_DROP: u32 = 268435456;
+pub const SIGMATCH_BAN_ACTION_REJECT: u32 = 536870912;
 pub type __intmax_t = ::std::os::raw::c_long;
 pub type intmax_t = __intmax_t;
 #[repr(u32)]

src/decode.c

@@ -1063,6 +1063,13 @@ static bool VerdictByFirewall(const Packet *p)
     return false;
 }
 
+static bool PacketBypassed(const Packet *p)
+{
+    if ((p->flags & PKT_FW_BYPASSED) != 0)
+        return true;
+    return false;
+}
+
 void CaptureStatsUpdate(ThreadVars *tv, const Packet *p)
 {
     if (!EngineModeIsIPS() || PKT_IS_PSEUDOPKT(p))
@@ -1096,7 +1103,9 @@ void CaptureStatsUpdate(ThreadVars *tv, const Packet *p)
             }
         } else if (PacketCheckAction(p, ACTION_ACCEPT)) {
             StatsCounterIncr(&tv->stats, s->counter_fw_accepted);
-            StatsCounterIncr(&tv->stats, s->counter_ips_accepted);
+            /* A packet bypassed by the firewall isn't seen by IPS */
+            if (!PacketBypassed(p))
+                StatsCounterIncr(&tv->stats, s->counter_ips_accepted);
         }
     } else {
         if (unlikely(PacketCheckAction(p, ACTION_REJECT_ANY))) {

src/decode.h

@@ -1321,7 +1321,10 @@ void DecodeUnregisterCounters(void);
                    depth reached. */
 #define PKT_STREAM_NOPCAPLOG BIT_U32(12)
 
-// vacancy 2x
+/** Packet was bypassed by a (firewall) rule */
+#define PKT_FW_BYPASSED BIT_U32(13)
+
+// vacancy
 
 /** Packet checksum is not computed (TX packet for example) */
 #define PKT_IGNORE_CHECKSUM BIT_U32(15)

src/detect-bypass.c

@@ -64,7 +64,11 @@ void DetectBypassRegister(void)
     sigmatch_table[DETECT_BYPASS].Match = DetectBypassMatch;
     sigmatch_table[DETECT_BYPASS].Setup = DetectBypassSetup;
     sigmatch_table[DETECT_BYPASS].Free  = NULL;
-    sigmatch_table[DETECT_BYPASS].flags = SIGMATCH_NOOPT | SIGMATCH_BAN_FIREWALL_MODE;
+    sigmatch_table[DETECT_BYPASS].flags =
+            SIGMATCH_NOOPT | SIGMATCH_SUPPORT_FIREWALL | SIGMATCH_BAN_TD_FIREWALL_MODE |
+            SIGMATCH_BAN_FIREWALL_SCOPE_PACKET | SIGMATCH_BAN_FIREWALL_SCOPE_TX |
+            SIGMATCH_BAN_FIREWALL_SCOPE_HOOK | SIGMATCH_BAN_ACTION_CONFIG |
+            SIGMATCH_BAN_ACTION_DROP | SIGMATCH_BAN_ACTION_REJECT;
 }
 
 static int DetectBypassSetup(DetectEngineCtx *de_ctx, Signature *s, const char *str)

src/detect-engine-alert.c

@@ -600,6 +600,35 @@ static struct DetectFirewallPolicy HandleFirewallRule(
     return pol;
 }
 
+/**
+ *  \brief see if a firewall rule in the queue bypassed the flow
+ *
+ *  `bypass` is applied from the postmatch list at match time. By the time the queue
+ *  is processed the flow is already out of inspection. So, Threat detection must
+ *  not be consulted for this packet. Especially when a TD rule sorts ahead
+ *  of the firewall rule, as packet:td does relative to app:filter.
+ *
+ *  Independent of the rule's action: `bypass` is a keyword, not an action, and is not tied to
+ * `accept`
+ */
+static inline bool AlertQueueHasFirewallBypass(
+        const DetectEngineThreadCtx *det_ctx, const Packet *p)
+{
+    if (p->flow == NULL || PKT_IS_PSEUDOPKT(p) || !FlowIsBypassed(p->flow))
+        return false;
+
+    for (uint16_t i = 0; i < det_ctx->alert_queue_size; i++) {
+        const Signature *s = det_ctx->alert_queue[i].s;
+        if ((s->flags & (SIG_FLAG_FIREWALL | SIG_FLAG_BYPASS)) ==
+                (SIG_FLAG_FIREWALL | SIG_FLAG_BYPASS)) {
+            SCLogDebug("packet %" PRIu64 ": fw sid %u bypassed the flow, skipping td",
+                    PcapPacketCntGet(p), s->id);
+            return true;
+        }
+    }
+    return false;
+}
+
 /*
  * Queue order after sorting:
  *
@@ -650,6 +679,13 @@ static inline void PacketAlertFinalizeProcessQueue(
 #endif /* DEBUG */
     uint8_t skip_table_id = 0;
     bool skip_table = false;
+
+    if (AlertQueueHasFirewallBypass(det_ctx, p)) {
+        skip_td = true;
+        /* a bypass verdict won't be changed at this point */
+        p->flags |= PKT_FW_BYPASSED;
+    }
+
     for (uint16_t i = 0; i < det_ctx->alert_queue_size; i++) {
         PacketAlert *pa = &det_ctx->alert_queue[i];
         const Signature *s = pa->s;
@@ -707,9 +743,17 @@ static inline void PacketAlertFinalizeProcessQueue(
         /* if a firewall rule told us to skip, we don't count the skipped
          * alerts. */
         if (have_fw_rules && skip_td) {
+            /* a bypass means this packet should never have reached TD, so its
+             * post-match state must not stand. Other skips (pass rules) only
+             * withhold the alert. */
+            if (!(p->flags & PKT_FW_BYPASSED))
+                DetectRunPostMatchDeferred(det_ctx, p, s);
             continue;
         }
 
+        /* run state-changing post-match for remaining TD rules */
+        DetectRunPostMatchDeferred(det_ctx, p, s);
+
         int res = PacketAlertHandle(de_ctx, det_ctx, s, p, pa);
         SCLogDebug("sid %u: res %d", pa->s->id, res);
         if (res > 0) {
@@ -789,7 +833,9 @@ static inline void PacketAlertFinalizeProcessQueue(
 
 fw_dropped:
     /* after threat detection has been handled, see if the fw intended to accept (drop is handled
-     * immediately by the fw), as fw accept can be overruled by td drop. */
+     * immediately by the fw), as fw accept can be overruled by td drop.
+     * (this is not the case if a flow is accepted _and_ bypassed with a firewall rule
+     * (accept:flow+bypass)) */
     if (have_fw_rules) {
         if (p->action & ACTION_DROP) {
             SCLogDebug("packet %" PRIu64 ": dropped by TD", PcapPacketCntGet(p));

src/detect-engine-build.c

@@ -37,6 +37,7 @@
 #include "detect-flow.h"
 #include "detect-config.h"
 #include "detect-flowbits.h"
+#include "detect-xbits.h"
 
 #include "app-layer-events.h"
 
@@ -2212,6 +2213,31 @@ int SigPrepareStage4(DetectEngineCtx *de_ctx)
     SCReturnInt(0);
 }
 
+/** \internal
+ *  \brief post-match keywords whose state outlives the flow
+ *
+ * A threat detection rule can match a packet that a firewall rule then
+ * bypasses. The alert is dropped during alert finalization, but the post-match
+ * list has already run at match time. Flow-scoped entries are harmless -- the
+ * flow is leaving inspection -- but host, ip-pair and packet-mark state
+ * outlive it, so those are held back until the verdict is known.
+ */
+static bool PostMatchIsDeferred(const SigMatchData *smd)
+{
+    switch (smd->type) {
+        case DETECT_XBITS: {
+            const DetectXbitsData *xd = (const DetectXbitsData *)smd->ctx;
+            /* tx-scoped bits die with the bypassed flow */
+            return xd->type != VAR_TYPE_TX_BIT;
+        }
+        case DETECT_HOSTBITS:
+        case DETECT_MARK:
+            return true;
+        default:
+            return false;
+    }
+}
+
 extern bool rule_engine_analysis_set;
 /** \internal
  *  \brief perform final per signature setup tasks
@@ -2239,6 +2265,17 @@ static int SigMatchPrepare(DetectEngineCtx *de_ctx)
             SigMatch *sm = s->init_data->smlists[type];
             s->sm_arrays[type] = SigMatchList2DataArray(sm);
         }
+        /* firewall mode: hold back threat detect post-match state that
+         * outlives the flow, so a firewall bypass can suppress it. */
+        if (EngineModeIsFirewall() && (s->flags & SIG_FLAG_FIREWALL) == 0) {
+            SigMatchData *smd = s->sm_arrays[DETECT_SM_LIST_POSTMATCH];
+            while (smd != NULL) {
+                smd->deferred = PostMatchIsDeferred(smd);
+                if (smd->is_last)
+                    break;
+                smd++;
+            }
+        }
         /* set up the pkt inspection engines */
         DetectEnginePktInspectionSetup(s);