From a329e0a3b9abb5ee7a844f3acd921fefa93b56b8 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. Ticket: 8967 --- doc/userguide/firewall/firewall-design.rst | 11 +- src/detect-parse.c | 135 ++++++++++++--------- suricata.yaml.in | 1 + 3 files changed, 92 insertions(+), 55 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 0849e36891..681c51e57b 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -420,8 +420,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"] A ``<`` hook rule at the protocol's first state (progress 0) is accepted and is equivalent to the plain hook form, as there are no prior states to auto-accept. diff --git a/src/detect-parse.c b/src/detect-parse.c index 8453dc5a8d..ac48608613 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -111,6 +111,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 }; @@ -119,6 +120,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, @@ -2449,6 +2454,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) @@ -2800,6 +2866,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: @@ -4246,59 +4323,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. */ @@ -4500,8 +4524,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; } diff --git a/suricata.yaml.in b/suricata.yaml.in index bb96a4fa27..49c1df696c 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -2410,6 +2410,7 @@ firewall: # default-policy: ["drop:flow"] # packet: # filter: ["drop:packet"] + # pre-flow: ["accept:hook"] # app: # dns: # request-started: ["accept:hook"]