From 6ac6a4e86a19cc8be496c3de96c2a8502cdb9323 Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Wed, 26 Aug 2026 19:29:49 +0200 Subject: [PATCH] firewall: reject flow scope for the pre-flow hook Action scopes for packet hook default policies were validated per hook class, so `flow` was accepted for every packet hook. The pre_flow hook is a special case as it is evaluated before Suricata obtains a flow record. Firewall rules on the pre_flow hook were not validated at all, so `flow` and `tx` scopes were accepted there as well. This commit adds extra validation to scope parsing for the pre_flow hook default policy and validates pre_flow firewall rules against the same scopes. Backport adaptation: the `suricata.yaml.in` hunk is left out, as the 8.0.x yaml has no `firewall.policies` example to extend. Ticket: 8968 (cherry picked from commit a329e0a3b9abb5ee7a844f3acd921fefa93b56b8) --- doc/userguide/firewall/firewall-design.rst | 11 +- src/detect-parse.c | 135 ++++++++++++--------- 2 files changed, 91 insertions(+), 55 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 1ddb364bc7..a59384b054 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -373,8 +373,17 @@ Precedence: An action scope must be valid for the hook it is applied to. For example, defining ``accept:tx`` as a global default policy will fail to start Suricata, because ``packet`` policies do not accept ``tx``. +Similarly, ``packet.pre-flow`` only accepts the ``packet`` and ``hook`` scopes, +so a ``drop:flow`` set in ``policies.default-policy`` or +``packet.default-policy`` will fail to start Suricata. Cover such hooks with a more specific setting so the incompatible default never -reaches them. +reaches them:: + + firewall: + policies: + default-policy: ["drop:flow"] + packet: + pre-flow: ["accept:hook"] ARP handling in bridge mode diff --git a/src/detect-parse.c b/src/detect-parse.c index 52ac401bb1..c945f41f40 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -112,6 +112,7 @@ typedef struct SignatureParser_ { /** Valid action scopes per firewall hook class. */ enum DetectFirewallPolicyClass { DETECT_FIREWALL_POLICY_CLASS_PACKET, + DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW, DETECT_FIREWALL_POLICY_CLASS_APP }; @@ -120,6 +121,10 @@ static const uint8_t fw_packet_hook_scopes[] = { ACTION_SCOPE_HOOK, ACTION_SCOPE_FLOW, }; +static const uint8_t fw_packet_pre_flow_hook_scopes[] = { + ACTION_SCOPE_PACKET, + ACTION_SCOPE_HOOK, +}; static const uint8_t fw_app_hook_scopes[] = { ACTION_SCOPE_FLOW, ACTION_SCOPE_TX, @@ -2252,6 +2257,67 @@ void SigFree(DetectEngineCtx *de_ctx, Signature *s) SCFree(s); } +static bool FirewallScopeValidForClass(uint8_t scope, enum DetectFirewallPolicyClass pol_class) +{ + const uint8_t *set = NULL; + size_t n = 0; + switch (pol_class) { + case DETECT_FIREWALL_POLICY_CLASS_PACKET: + set = fw_packet_hook_scopes; + n = ARRAY_SIZE(fw_packet_hook_scopes); + break; + case DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW: + set = fw_packet_pre_flow_hook_scopes; + n = ARRAY_SIZE(fw_packet_pre_flow_hook_scopes); + break; + case DETECT_FIREWALL_POLICY_CLASS_APP: + set = fw_app_hook_scopes; + n = ARRAY_SIZE(fw_app_hook_scopes); + break; + default: + FatalError("Invalid firewall policy class %u", (unsigned)pol_class); + } + for (size_t i = 0; i < n; i++) { + if (set[i] == scope) { + return true; + } + } + return false; +} + +/** + * \brief Render the valid scopes for a hook class to a string. + */ +static void FirewallScopeHintForClass( + enum DetectFirewallPolicyClass pol_class, char *out, size_t out_size) +{ + const uint8_t *set = NULL; + size_t n = 0; + switch (pol_class) { + case DETECT_FIREWALL_POLICY_CLASS_PACKET: + set = fw_packet_hook_scopes; + n = ARRAY_SIZE(fw_packet_hook_scopes); + break; + case DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW: + set = fw_packet_pre_flow_hook_scopes; + n = ARRAY_SIZE(fw_packet_pre_flow_hook_scopes); + break; + case DETECT_FIREWALL_POLICY_CLASS_APP: + set = fw_app_hook_scopes; + n = ARRAY_SIZE(fw_app_hook_scopes); + break; + default: + FatalError("Invalid firewall policy class %u", (unsigned)pol_class); + } + out[0] = '\0'; + for (size_t i = 0; i < n; i++) { + if ((i > 0 && strlcat(out, "/", out_size) >= out_size) || + strlcat(out, ActionScopeToString((enum ActionScope)set[i]), out_size) >= out_size) { + FatalError("firewall policy scope hint too long"); + } + } +} + /** * \brief this function is used to set multiple possible app-layer protos * \brief into the current signature (for example ja4 for both tls and quic) @@ -2603,6 +2669,17 @@ static bool DetectFirewallRuleValidate(const DetectEngineCtx *de_ctx, const Sign s->id); return false; } + if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_PKT && + s->init_data->hook.t.pkt.ph == SIGNATURE_HOOK_PKT_PRE_FLOW && + !FirewallScopeValidForClass( + s->action_scope, DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW)) { + char hint[32]; // space to combine ActionScopeToString results + FirewallScopeHintForClass(DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW, hint, sizeof(hint)); + SCLogError("rule %u: action scope (\"%s\") is not valid for the \"pre_flow\" hook. Valid " + "scopes: %s", + s->id, ActionScopeToString((enum ActionScope)s->action_scope), hint); + return false; + } if (s->init_data->hook.type == SIGNATURE_HOOK_TYPE_APP) { switch (s->action_scope) { case ACTION_SCOPE_PACKET: @@ -3964,59 +4041,6 @@ static int DoParsePolicy(const char *policy_name, struct DetectFirewallPolicy *p return 1; } -static bool FirewallScopeValidForClass(uint8_t scope, enum DetectFirewallPolicyClass pol_class) -{ - const uint8_t *set = NULL; - size_t n = 0; - switch (pol_class) { - case DETECT_FIREWALL_POLICY_CLASS_PACKET: - set = fw_packet_hook_scopes; - n = ARRAY_SIZE(fw_packet_hook_scopes); - break; - case DETECT_FIREWALL_POLICY_CLASS_APP: - set = fw_app_hook_scopes; - n = ARRAY_SIZE(fw_app_hook_scopes); - break; - default: - FatalError("Invalid firewall policy class %u", (unsigned)pol_class); - } - for (size_t i = 0; i < n; i++) { - if (set[i] == scope) { - return true; - } - } - return false; -} - -/** - * \brief Render the valid scopes for a hook class to a string. - */ -static void FirewallScopeHintForClass( - enum DetectFirewallPolicyClass pol_class, char *out, size_t out_size) -{ - const uint8_t *set = NULL; - size_t n = 0; - switch (pol_class) { - case DETECT_FIREWALL_POLICY_CLASS_PACKET: - set = fw_packet_hook_scopes; - n = ARRAY_SIZE(fw_packet_hook_scopes); - break; - case DETECT_FIREWALL_POLICY_CLASS_APP: - set = fw_app_hook_scopes; - n = ARRAY_SIZE(fw_app_hook_scopes); - break; - default: - FatalError("Invalid firewall policy class %u", (unsigned)pol_class); - } - out[0] = '\0'; - for (size_t i = 0; i < n; i++) { - if ((i > 0 && strlcat(out, "/", out_size) >= out_size) || - strlcat(out, ActionScopeToString((enum ActionScope)set[i]), out_size) >= out_size) { - FatalError("firewall policy scope hint too long"); - } - } -} - /** * \brief Append a unique inheritance tier to the chain of firewall policies to query. */ @@ -4212,8 +4236,11 @@ static int DetectFirewallLoadPacketPolicy(struct DetectFirewallPolicies *fw_poli /* .default-policy */ FirewallPolicyChainAdd(&chain, "%s.default-policy", prefix); + const enum DetectFirewallPolicyClass pol_class = + (id == DETECT_FIREWALL_POLICY_PRE_FLOW) ? DETECT_FIREWALL_POLICY_CLASS_PACKET_PRE_FLOW + : DETECT_FIREWALL_POLICY_CLASS_PACKET; struct DetectFirewallPolicy *pol = &fw_policies->pkt[id]; // built-in default - int r = ResolveFirewallPolicy(pol, DETECT_FIREWALL_POLICY_CLASS_PACKET, &chain); + int r = ResolveFirewallPolicy(pol, pol_class, &chain); if (r < 0) { return -1; }