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

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/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,51 @@ 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.
+
+.. note:: If `bypass` is used in a rule together with thresholding, the bypass
+   could be silent, if the alert is suppressed. (This can be checked with the
+   stats counter: ``detect.alerts_suppressed``).
+
+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.

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)]

scripts/check-doc-rules.py

@@ -90,18 +90,21 @@ def iter_rst_files(path: Path) -> Iterable[Path]:
 
 
 def resolve_suricata_bin(repo_root: Path, configured: Optional[str]) -> Path:
+    candidates = [repo_root / "src" / "suricata", repo_root / "suricata"]
+
+    # First check for local repo, as there may be patches to test.
+    # Then look for the Path option.
+    for candidate in candidates:
+        if candidate.exists():
+            return candidate
+
     if configured:
         return Path(configured)
 
     in_path = shutil.which("suricata")
     if in_path:
         return Path(in_path)
 
-    candidates = [repo_root / "src" / "suricata", repo_root / "suricata"]
-    for candidate in candidates:
-        if candidate.exists():
-            return candidate
-
     raise SystemExit(
         "Unable to find Suricata binary. Use --suricata-bin to provide it."
     )
@@ -118,24 +121,39 @@ def check_rule_with_suricata(
         shutil.copytree(data_dir, tmpdir, dirs_exist_ok=True)
         rule_file.write_text(rule + "\n", encoding="utf-8")
 
-        cmd = [
-            str(suricata_bin),
-            "-T",
-            "-c", str(suricata_yaml),
-            "--data-dir="+tmpdir,
-            "-S", str(rule_file),
-            '--strict-rule-keywords=all',
-            "-l", tmpdir,
-        ]
-        proc = subprocess.run(
-            cmd,
-            check=False,
-            capture_output=True,
-            text=True,
+        load_modes = (
+            ("detection", ["-S", str(rule_file)]),
+            ("firewall", ["--firewall-rules-exclusive=" + str(rule_file)]),
         )
 
-        combined = proc.stderr.strip()
-        return proc.returncode == 0, combined
+        # Check against both Threat Detection and Firewall rule parsers
+        # before failing the example rules
+        failures: List[str] = []
+        for label, load_args in load_modes:
+            cmd = [
+                str(suricata_bin),
+                "-T",
+                "-c", str(suricata_yaml),
+                "--data-dir="+tmpdir,
+                *load_args,
+                '--strict-rule-keywords=all',
+                "-l", tmpdir,
+            ]
+            proc = subprocess.run(
+                cmd,
+                check=False,
+                capture_output=True,
+                text=True,
+            )
+
+            if proc.returncode == 0:
+                return True, ""
+
+            failures.append(
+                f"--- rejected as {label} rule ---\n{proc.stderr.strip()}"
+                )
+
+        return False, "\n\n".join(failures)
 
 
 def main() -> int:

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

@@ -589,17 +589,51 @@ static struct DetectFirewallPolicy HandleFirewallRule(
             }
         }
         /* add the alert for logging if required. */
-        if (s->action & ACTION_ALERT) {
+        if ((s->action & ACTION_ALERT) && res != 2) {
             if (p->alerts.cnt < packet_alert_max) {
                 p->alerts.alerts[p->alerts.cnt++] = *pa;
             } else {
                 p->alerts.firewall_discarded++;
             }
         }
     }
+
+    /* threshold removed the alert; account for it as the TD path does */
+    if ((res == 0 || res == 2) && (s->action & ACTION_ALERT)) {
+        p->alerts.suppressed++;
+    }
     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 +684,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;
@@ -789,7 +830,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-register.c

@@ -343,6 +343,48 @@ static void PrintFeatureList(const SigTableElmt *e, char sep)
         printf("banned from firewall mode");
         prev = 1;
     }
+    if (flags & SIGMATCH_BAN_TD_FIREWALL_MODE) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from threat detection rules in firewall mode");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_FIREWALL_SCOPE_TX) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'tx\' scope");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_FIREWALL_SCOPE_HOOK) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'hook\' scope");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_ACTION_CONFIG) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'config\' action");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_ACTION_DROP) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'drop\' action");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_ACTION_REJECT) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'reject\' action");
+        prev = 1;
+    }
+    if (flags & SIGMATCH_BAN_FIREWALL_SCOPE_PACKET) {
+        if (prev == 1)
+            printf("%c", sep);
+        printf("banned from firewall rules with \'packet\' scope");
+        prev = 1;
+    }
     if (e->Transform) {
         if (prev == 1)
             printf("%c", sep);