diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index ffee070fe25c..aa68719159ea 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/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`, 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`. +and application layer (if available). Each rule can define its own +:ref:`action scope`. Packet layer tables ~~~~~~~~~~~~~~~~~~~ diff --git a/doc/userguide/firewall/firewall-stats.rst b/doc/userguide/firewall/firewall-stats.rst index d8561e34dcbe..4fb80c4344a4 100644 --- a/doc/userguide/firewall/firewall-stats.rst +++ b/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. diff --git a/doc/userguide/rules/bypass-keyword.rst b/doc/userguide/rules/bypass-keyword.rst index 6572f72f0152..eeae2f0b1b9c 100644 --- a/doc/userguide/rules/bypass-keyword.rst +++ b/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. diff --git a/rust/sys/src/sys.rs b/rust/sys/src/sys.rs index 0f0e7d3d65c8..d6096b58ef4a 100644 --- a/rust/sys/src/sys.rs +++ b/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)] diff --git a/scripts/check-doc-rules.py b/scripts/check-doc-rules.py index 2785039c539f..3124c1383286 100644 --- a/scripts/check-doc-rules.py +++ b/scripts/check-doc-rules.py @@ -90,6 +90,14 @@ 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) @@ -97,11 +105,6 @@ def resolve_suricata_bin(repo_root: Path, configured: Optional[str]) -> Path: 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: diff --git a/src/decode.c b/src/decode.c index e6800d80063a..ca3a56fde9b7 100644 --- a/src/decode.c +++ b/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))) { diff --git a/src/decode.h b/src/decode.h index c7e8e31d9869..40f8f6ea368b 100644 --- a/src/decode.h +++ b/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) diff --git a/src/detect-bypass.c b/src/detect-bypass.c index 61f93c05648c..b18e9d408487 100644 --- a/src/detect-bypass.c +++ b/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) diff --git a/src/detect-engine-alert.c b/src/detect-engine-alert.c index 38d262c364ea..40d09b89e70b 100644 --- a/src/detect-engine-alert.c +++ b/src/detect-engine-alert.c @@ -589,7 +589,7 @@ 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 { @@ -597,9 +597,43 @@ static struct DetectFirewallPolicy HandleFirewallRule( } } } + + /* 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)); diff --git a/src/detect-engine-register.c b/src/detect-engine-register.c index 845521acac1e..c74174d644ec 100644 --- a/src/detect-engine-register.c +++ b/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); diff --git a/src/detect-engine-register.h b/src/detect-engine-register.h index 221b25ef6997..41d15a2e438d 100644 --- a/src/detect-engine-register.h +++ b/src/detect-engine-register.h @@ -356,6 +356,16 @@ extern int DETECT_TBLSIZE_IDX; #define SIGMATCH_BAN_FIREWALL_RULE (1UL << (21)) /** keyword cannot be used in firewall mode */ #define SIGMATCH_BAN_FIREWALL_MODE (1UL << (22)) +/** keyword cannot be used in td rules with firewall mode */ +#define SIGMATCH_BAN_TD_FIREWALL_MODE (1UL << (23)) +/** keyword cannot be used in combination with indicated action scope */ +#define SIGMATCH_BAN_FIREWALL_SCOPE_PACKET (1UL << (24)) +#define SIGMATCH_BAN_FIREWALL_SCOPE_TX (1UL << (25)) +#define SIGMATCH_BAN_FIREWALL_SCOPE_HOOK (1UL << (26)) +/** keyword cannot be unsed in combination with indicated action */ +#define SIGMATCH_BAN_ACTION_CONFIG (1UL << (27)) +#define SIGMATCH_BAN_ACTION_DROP (1UL << (28)) +#define SIGMATCH_BAN_ACTION_REJECT (1UL << (29)) int SigTableList(const char *keyword); void SigTableCleanup(void); diff --git a/src/detect-parse.c b/src/detect-parse.c index 8829b6859cab..b8a03efb8e84 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -868,6 +868,59 @@ static int DetectSetupDirection(Signature *s, char **str, bool only_dir) return 0; } +/** + * \brief called only with firewall rules, to validate options + * + * It is valid to call this before the keyword's value, if any, has been parsed. + * + * \retval false if the keyword is not allowed for this rule's action and/or action scope + */ +static bool SigParseFirewallRuleAllowed( + uint8_t action, uint8_t action_scope, uint32_t sig_flags, const char *optname) +{ + if ((sig_flags & SIGMATCH_BAN_FIREWALL_RULE) != 0) { + SCLogError("keyword \'%s\' is not allowed with firewall rules", optname); + return false; + } + if ((sig_flags & SIGMATCH_BAN_FIREWALL_MODE) != 0) { + SCLogError("keyword \'%s\' is not allowed in firewall mode", optname); + return false; + } + if ((action & ACTION_CONFIG) != 0 && (sig_flags & SIGMATCH_BAN_ACTION_CONFIG) != 0) { + SCLogError("keyword \'%s\' cannot be used in combination with \'config\' action", optname); + return false; + } + if ((action & ACTION_REJECT_ANY) != 0 && (sig_flags & SIGMATCH_BAN_ACTION_REJECT) != 0) { + SCLogError("keyword \'%s\' cannot be used in combination with \'reject\' action", optname); + return false; + } + if ((action & ACTION_DROP) != 0 && (sig_flags & SIGMATCH_BAN_ACTION_DROP) != 0) { + SCLogError("keyword \'%s\' cannot be used in combination with \'drop\' action", optname); + return false; + } + if (action_scope == (uint8_t)ACTION_SCOPE_PACKET) { + if ((sig_flags & SIGMATCH_BAN_FIREWALL_SCOPE_PACKET) != 0) { + SCLogError( + "keyword \'%s\' cannot be used in combination with \'packet\' scope", optname); + return false; + } + } + if (action_scope == (uint8_t)ACTION_SCOPE_TX) { + if ((sig_flags & SIGMATCH_BAN_FIREWALL_SCOPE_TX) != 0) { + SCLogError("keyword \'%s\' cannot be used in combination with \'tx\' scope", optname); + return false; + } + } + if (action_scope == (uint8_t)ACTION_SCOPE_HOOK) { + if ((sig_flags & SIGMATCH_BAN_FIREWALL_SCOPE_HOOK) != 0) { + SCLogError("keyword \'%s\' cannot be used in combination with \'hook\' scope", optname); + return false; + } + } + /* for most cases, firewall rules should be allowed */ + return true; +} + static int SigParseOptions(DetectEngineCtx *de_ctx, Signature *s, char *optstr, char *output, size_t output_size, bool requires) { @@ -968,16 +1021,24 @@ static int SigParseOptions(DetectEngineCtx *de_ctx, Signature *s, char *optstr, #undef URL } - if (s->init_data->firewall_rule && (st->flags & SIGMATCH_BAN_FIREWALL_RULE) != 0) { - SCLogError("keyword \'%s\' is not allowed with firewall rules", optname); + if (EngineModeIsFirewall() && s->init_data->firewall_rule && + !SigParseFirewallRuleAllowed(s->action, s->action_scope, st->flags, optname)) { goto error; } - if (EngineModeIsFirewall() && (st->flags & SIGMATCH_BAN_FIREWALL_MODE) != 0) { + /* For non-firewall rules */ + if (EngineModeIsFirewall() && !s->init_data->firewall_rule && + (st->flags & SIGMATCH_BAN_FIREWALL_MODE) != 0) { SCLogError("keyword \'%s\' is not allowed in firewall mode", optname); goto error; } + if (EngineModeIsFirewall() && !s->init_data->firewall_rule && + (st->flags & SIGMATCH_BAN_TD_FIREWALL_MODE) != 0) { + SCLogError("keyword \'%s\' is not allowed in threat detection rules with firewall mode", + optname); + goto error; + } int setup_ret = 0; /* Validate double quoting, trimming trailing white space along the way. */ @@ -2711,6 +2772,7 @@ static bool DetectRuleValidateTable(const Signature *s) return true; } +/** \brief validates firewall rule action scope */ static bool DetectFirewallRuleValidate(const DetectEngineCtx *de_ctx, const Signature *s) { if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_NOT_SET) {