From 1aa0259876c49389c1d2df47472bd99c1f53e62f Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Wed, 19 Aug 2026 16:42:08 +0200 Subject: [PATCH 1/3] firewall: share the generic app hook name helper Mapping an app-layer progress state to its generic request-/response- hook alias was hardcoded in multiple places. This commit adds a wrapper to unify it to a single function. It returns the config-form (hyphenated) name, or NULL for an intermediate state. Ticket: 8712 --- src/detect-engine-analyzer.c | 14 ++++---------- src/detect-parse.c | 14 ++++++++++++++ src/detect-parse.h | 3 +++ 3 files changed, 21 insertions(+), 10 deletions(-) diff --git a/src/detect-engine-analyzer.c b/src/detect-engine-analyzer.c index 0f952ad34b6e..f8d14cbc8de3 100644 --- a/src/detect-engine-analyzer.c +++ b/src/detect-engine-analyzer.c @@ -2349,11 +2349,8 @@ int FirewallAnalyzer(const DetectEngineCtx *de_ctx) const char *name = AppLayerParserGetStateNameById(IPPROTO_TCP, a, state, STREAM_TOSERVER); if (name == NULL) { - if (state == 0) - name = "request-started"; - else if (state == complete_state_ts) - name = "request-complete"; - else + name = DetectFirewallAppGenericHookName(state, complete_state_ts, STREAM_TOSERVER); + if (name == NULL) name = "unknown"; } @@ -2375,11 +2372,8 @@ int FirewallAnalyzer(const DetectEngineCtx *de_ctx) const char *name = AppLayerParserGetStateNameById(IPPROTO_TCP, a, state, STREAM_TOCLIENT); if (name == NULL) { - if (state == 0) - name = "response-started"; - else if (state == complete_state_tc) - name = "response-complete"; - else + name = DetectFirewallAppGenericHookName(state, complete_state_tc, STREAM_TOCLIENT); + if (name == NULL) name = "unknown"; } char table_name[128]; diff --git a/src/detect-parse.c b/src/detect-parse.c index 8829b6859cab..2ad60be0566c 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -1145,6 +1145,20 @@ static bool IsBuiltIn(const char *n) strcmp(n, "request_complete") == 0 || strcmp(n, "response_complete") == 0; } +/** + * \brief Generic start/complete hook alias for an app progress state, in config + * form (hyphens), or NULL for intermediate states. + */ +const char *DetectFirewallAppGenericHookName( + const uint8_t state, const uint8_t complete_state, const int direction) +{ + if (state == 0) + return (direction == STREAM_TOSERVER) ? "request-started" : "response-started"; + if (state == complete_state) + return (direction == STREAM_TOSERVER) ? "request-complete" : "response-complete"; + return NULL; +} + /** \brief register app hooks as generic lists * * Register each hook in each app protocol as: diff --git a/src/detect-parse.h b/src/detect-parse.h index 4083163ea867..196ead6606e0 100644 --- a/src/detect-parse.h +++ b/src/detect-parse.h @@ -118,6 +118,9 @@ void DetectListSupportedProtocols(void); const char *ActionScopeToString(enum ActionScope s); +const char *DetectFirewallAppGenericHookName( + const uint8_t state, const uint8_t complete_state, const int direction); + struct DetectFirewallPolicy; void DetectFirewallPolicyToString(const struct DetectFirewallPolicy *p, char *out, size_t out_size); int DetectFirewallInitDefaultPolicies(DetectEngineCtx *); From 0aaa80d4f3a749c3033056d7b0f91658619e9af9 Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:09:52 +0200 Subject: [PATCH 2/3] firewall: add default-policy to policy config Every hook has a built-in default policy, but expressing anything other than the built-in meant naming each hook explicitly. This commit adds a `default-policy` setting that covers all hooks below it. For any hook the most specific setting present wins. DoParseAppSubStatePolicy() collapses into DoParseAppPolicy() as a sub state hook only differs by an extra path segment. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 48 ++-- src/detect-parse.c | 314 +++++++++++---------- suricata.yaml.in | 6 +- 3 files changed, 207 insertions(+), 161 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index ffee070fe25c..4ad1e6df1c06 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -349,32 +349,48 @@ The example below accepts ARP again, using this mechanism. Default policies ================ -Each hook has a default policy. By default ``packet.filter`` enforces a ``drop:packet`` policy and the -``app`` hooks apply ``drop:flow``. +Each hook has a default policy applied to traffic that no firewall rule handled. +By default ``packet.filter`` enforces ``drop:packet``, ``packet.pre-flow`` and +``packet.pre-stream`` enforce ``accept:hook``, and every ``app`` hook enforces +``drop:flow``. -The policies can be configured in ``firewall`` block in the config. Packet hooks -live under ``packet`` and app-layer hooks under ``app``, keyed by protocol. - -Example for ``packet.filter``, to use reject instead of drop:: +Defaults are configured in the ``firewall.policies`` block. A ``default-policy`` +for any hook may be given at several levels and the most specific present +setting wins:: firewall: policies: + default-policy: ["accept:hook"] # global fallback (all hooks) packet: - filter: [ "reject:packet" ] - - -Example for DNS:: - - firewall: - policies: + default-policy: ["drop:packet"] # fallback for packet hooks + filter: ["reject:packet"] + pre-flow: ["accept:hook"] + pre-stream: ["accept:hook"] app: + default-policy: ["drop:flow"] # fallback for all app hooks dns: + default-policy: ["drop:flow"] # fallback for dns hooks request-started: ["accept:hook"] - # Drop and alert on all DNS requests that are not allowed in # firewall.rules. request-complete: ["drop:flow", "alert"] - # Accept all responses. response-started: ["accept:tx"] - + # Define default policies for protocols with sub states + http2: + default-policy: ["drop:flow"] # fallback for all http2 hooks + stream: + default-policy: ["drop:flow"] # fallback for http2 stream hooks + request-started: ["accept:hook"] + global: + request-started: ["accept:hook"] + +Precedence: + +* packet hook: ``packet.`` > ``packet.default-policy`` > + ``policies.default-policy`` > built-in (``drop:packet`` or ``accept:hook``) +* app hook: ``app..`` > ``app..default-policy`` > + ``app.default-policy`` > ``policies.default-policy`` > built-in (``drop:flow``) +* app hook in a sub state: ``app...`` > + ``app...default-policy`` > ``app..default-policy`` > + ``app.default-policy`` > ``policies.default-policy`` > built-in (``drop:flow``) diff --git a/src/detect-parse.c b/src/detect-parse.c index 2ad60be0566c..dc3b9660825e 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -109,6 +109,22 @@ typedef struct SignatureParser_ { char opts[DETECT_MAX_RULE_SIZE]; } SignatureParser; +/** \brief max length of a firewall.policies YAML config path */ +#define FW_POLICY_YAML_PATH_MAX 320 +/** \brief max length of a single YAML path leaf segment (a hook or sub state name) */ +#define FW_POLICY_YAML_PATH_NAME_MAX 64 +/** \brief max number of config paths consulted to resolve one policy */ +#define FW_POLICY_CHAIN_MAX 6 + +/** + * \brief Ordered, most-specific-first list of config paths a single policy can + * be configured at. + */ +typedef struct FirewallPolicyChain { + char path[FW_POLICY_CHAIN_MAX][FW_POLICY_YAML_PATH_MAX]; + uint8_t len; +} FirewallPolicyChain; + const char *DetectListToHumanString(int list) { #define CASE_CODE_STRING(E, S) case E: return S; break @@ -4173,134 +4189,138 @@ static int DoParsePolicy(const char *policy_name, struct DetectFirewallPolicy *p return 1; } -static int DoParseAppSubStatePolicy(const char *prefix, const AppProto app_proto, - const uint8_t sub_state, const char *sub_state_name, const uint8_t state, - const char *hookname, const uint8_t complete_state, const int direction, - struct DetectFirewallPolicies *fw_policies) +/** + * \brief Append a unique inheritance tier to the chain of firewall policies to query. + */ +static void ATTR_FMT_PRINTF(2, 3) + FirewallPolicyChainAdd(FirewallPolicyChain *chain, const char *fmt, ...) { - char policy_name[256]; - BUG_ON(sub_state_name == NULL); - BUG_ON(hookname == NULL); - - char *nname = SCStrdup(hookname); - if (nname == NULL) - return -1; - for (int i = 0; nname[i] != '\0'; i++) { - if (nname[i] == '_') - nname[i] = '-'; + if (chain->len >= FW_POLICY_CHAIN_MAX) { + FatalError("too many firewall YAML config paths, max %u", FW_POLICY_CHAIN_MAX); } + char *path = chain->path[chain->len]; - const char *app_name = AppProtoToStringRaw(app_proto); - int r = snprintf(policy_name, sizeof(policy_name), "%s.app.%s.%s.%s", prefix, app_name, - sub_state_name, nname); - SCLogDebug("policy_name %s", policy_name); - SCFree(nname); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); + va_list ap; + va_start(ap, fmt); + int r = vsnprintf(path, FW_POLICY_YAML_PATH_MAX, fmt, ap); + va_end(ap); + if (r < 0) { + FatalError("%s: firewall YAML config path formatting failed", fmt); + } + if ((size_t)r >= FW_POLICY_YAML_PATH_MAX) { + FatalError("%s: firewall YAML config path too long", path); } - struct DetectFirewallAppPolicy *app_pol = SCCalloc(1, sizeof(*app_pol)); - if (app_pol == NULL) - return -1; - - app_pol->alproto = app_proto; - app_pol->sub_state = sub_state; - app_pol->progress = state; - app_pol->direction = (uint8_t)direction; - /* init to drop:flow by default, will be overwritten by DoParsePolicy if there - * is a config for this hook. */ - app_pol->policy.action = ACTION_DROP; - app_pol->policy.action_scope = ACTION_SCOPE_FLOW; + for (uint8_t i = 0; i < chain->len; i++) { + if (strcmp(chain->path[i], path) == 0) + return; + } + chain->len++; +} - r = DoParsePolicy(policy_name, &app_pol->policy); - if (r < 0) { - SCFree(app_pol); - return -1; +/** + * \brief Resolve a firewall policy from its config path chain. + * + * The first path in the chain that has a policy configured wins. + * + * \retval 1 a config source was used and stored in \p out + * \retval 0 no source present, \p out is unmodified + * \retval -1 parse error, e.g. an empty policy + */ +static int ResolveFirewallPolicy(struct DetectFirewallPolicy *out, const FirewallPolicyChain *chain) +{ + for (uint8_t i = 0; i < chain->len; i++) { + const char *path = chain->path[i]; + struct DetectFirewallPolicy tmp = { 0 }; + int r = DoParsePolicy(path, &tmp); + if (r < 0) { + return -1; + } + if (r == 1) { + if (tmp.action == 0) { + SCLogError("%s: policy is set but empty", path); + return -1; + } + *out = tmp; + return 1; + } } + return 0; +} - if (HashTableAdd(fw_policies->app_policies, app_pol, 0) != 0) { - FatalError("internal error: insert policy into hash table"); +static void FirewallHookNameConvertUnderscoreToDash(const char *in, char *out, size_t out_size) +{ + if (strlcpy(out, in, out_size) >= out_size) { + FatalError("%s: firewall policy config name too long", in); } - /* for policies with an alert action, create a policy sig */ - if (r == 1 && app_pol->policy.action & ACTION_ALERT) { - SCLogDebug("adding policy signature"); - return AddAppPolicySignature(app_pol); + for (size_t i = 0; out[i] != '\0'; i++) { + if (out[i] == '_') + out[i] = '-'; } - SCLogDebug("r %d", r); - return r; } -static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const char *hookname, - const uint8_t state, const uint8_t complete_state, const int direction, +/** + * \brief Resolve and store one app-layer hook default policy. + * + * Handles both plain hooks (\p sub_state_name NULL) and sub state hooks, which + * only differ by an extra path segment. + */ +static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const uint8_t sub_state, + const char *sub_state_name, const char *hookname, const uint8_t state, + const uint8_t complete_state, const int direction, struct DetectFirewallPolicies *fw_policies) { - char policy_name[256]; - const char *in_name = hookname; - if (hookname == NULL) { - if (state == 0) { - if (direction == STREAM_TOSERVER) - hookname = "request-started"; - else - hookname = "response-started"; - } else if (state == complete_state) { - if (direction == STREAM_TOSERVER) - hookname = "request-complete"; - else - hookname = "response-complete"; - } - if (hookname == NULL) - return 0; - } - char *nname = SCStrdup(hookname); - if (nname == NULL) + const char *app_proto_str = AppProtoToStringRaw(app_proto); + if (app_proto_str == NULL) { + SCLogError("Unknown app proto %u", (unsigned)app_proto); return -1; - for (int i = 0; nname[i] != '\0'; i++) { - if (nname[i] == '_') - nname[i] = '-'; } - const char *app_name = AppProtoToStringRaw(app_proto); - int r = snprintf(policy_name, sizeof(policy_name), "%s.app.%s.%s", prefix, app_name, nname); - SCFree(nname); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); + const char *generic_hook = DetectFirewallAppGenericHookName(state, complete_state, direction); + char hook[FW_POLICY_YAML_PATH_NAME_MAX] = ""; + if (hookname != NULL) { + FirewallHookNameConvertUnderscoreToDash(hookname, hook, sizeof(hook)); } + /* optional "." path segment, empty for plain hooks */ + char sub[FW_POLICY_YAML_PATH_NAME_MAX + 1] = ""; + if (sub_state_name != NULL) { + sub[0] = '.'; + FirewallHookNameConvertUnderscoreToDash(sub_state_name, sub + 1, sizeof(sub) - 1); + } + + FirewallPolicyChain chain = { .len = 0 }; + if (hookname != NULL) { + /* .app.[.]. */ + FirewallPolicyChainAdd(&chain, "%s.app.%s%s.%s", prefix, app_proto_str, sub, hook); + } + if (generic_hook != NULL) { + /* .app.[.]. */ + FirewallPolicyChainAdd(&chain, "%s.app.%s%s.%s", prefix, app_proto_str, sub, generic_hook); + } + /* .app.[.].default-policy */ + FirewallPolicyChainAdd(&chain, "%s.app.%s%s.default-policy", prefix, app_proto_str, sub); + /* .app..default-policy */ + FirewallPolicyChainAdd(&chain, "%s.app.%s.default-policy", prefix, app_proto_str); + /* .app.default-policy */ + FirewallPolicyChainAdd(&chain, "%s.app.default-policy", prefix); + /* .default-policy */ + FirewallPolicyChainAdd(&chain, "%s.default-policy", prefix); + struct DetectFirewallAppPolicy *app_pol = SCCalloc(1, sizeof(*app_pol)); if (app_pol == NULL) return -1; app_pol->alproto = app_proto; - app_pol->sub_state = 0; + app_pol->sub_state = sub_state; app_pol->progress = state; app_pol->direction = (uint8_t)direction; - /* init to drop:flow by default, will be overwritten by DoParsePolicy if there + /* init to drop:flow by default, will be overwritten by ResolveFirewallPolicy if there * is a config for this hook. */ app_pol->policy.action = ACTION_DROP; app_pol->policy.action_scope = ACTION_SCOPE_FLOW; - r = DoParsePolicy(policy_name, &app_pol->policy); - if (r == 0 && in_name != NULL) { - if (state == 0) { - if (direction == STREAM_TOSERVER) - hookname = "request-started"; - else - hookname = "response-started"; - } else if (state == complete_state) { - if (direction == STREAM_TOSERVER) - hookname = "request-complete"; - else - hookname = "response-complete"; - } - if (hookname == NULL) - return 0; - r = snprintf(policy_name, sizeof(policy_name), "%s.app.%s.%s", prefix, app_name, hookname); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); - } - - r = DoParsePolicy(policy_name, &app_pol->policy); - } + int r = ResolveFirewallPolicy(&app_pol->policy, &chain); if (r < 0) { SCFree(app_pol); return -1; @@ -4345,10 +4365,52 @@ int DetectFirewallInitDefaultPolicies(DetectEngineCtx *de_ctx) return 0; } +/** + * \brief Resolve and store one packet-hook default policy. + */ +static int DetectFirewallLoadPacketPolicy(struct DetectFirewallPolicies *fw_policies, + const char *prefix, enum DetectFirewallPacketPolicies id, const char *leaf) +{ + /* inheritance tiers, most specific first */ + FirewallPolicyChain chain = { .len = 0 }; + /* .packet. */ + FirewallPolicyChainAdd(&chain, "%s.packet.%s", prefix, leaf); + /* .packet.default-policy */ + FirewallPolicyChainAdd(&chain, "%s.packet.default-policy", prefix); + /* .default-policy */ + FirewallPolicyChainAdd(&chain, "%s.default-policy", prefix); + + struct DetectFirewallPolicy *pol = &fw_policies->pkt[id]; // built-in default + int r = ResolveFirewallPolicy(pol, &chain); + if (r < 0) { + return -1; + } + if (r == 1 && (pol->action & ACTION_ALERT)) { + return AddPktPolicySignature(fw_policies, pol, id); + } + return 0; +} + +/** + * \brief Load the packet-hook default policies. + */ +static int DetectFirewallLoadPacketPolicies( + struct DetectFirewallPolicies *fw_policies, const char *prefix) +{ + if (DetectFirewallLoadPacketPolicy( + fw_policies, prefix, DETECT_FIREWALL_POLICY_PACKET_FILTER, "filter") < 0) + return -1; + if (DetectFirewallLoadPacketPolicy( + fw_policies, prefix, DETECT_FIREWALL_POLICY_PRE_FLOW, "pre-flow") < 0) + return -1; + if (DetectFirewallLoadPacketPolicy( + fw_policies, prefix, DETECT_FIREWALL_POLICY_PRE_STREAM, "pre-stream") < 0) + return -1; + return 0; +} + int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) { - int r; - char policy_name[256]; char prefix[96] = "firewall.policies"; if (strlen(de_ctx->config_prefix) > 0) { snprintf(prefix, sizeof(prefix), "%s.firewall.policies", de_ctx->config_prefix); @@ -4358,42 +4420,8 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) if (fw_policies == NULL) return -1; - r = snprintf(policy_name, sizeof(policy_name), "%s.packet.filter", prefix); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); - } - r = DoParsePolicy(policy_name, &fw_policies->pkt[DETECT_FIREWALL_POLICY_PACKET_FILTER]); - if (r < 0) - return -1; - if (fw_policies->pkt[DETECT_FIREWALL_POLICY_PACKET_FILTER].action & ACTION_ALERT) - if (AddPktPolicySignature(fw_policies, - &fw_policies->pkt[DETECT_FIREWALL_POLICY_PACKET_FILTER], - DETECT_FIREWALL_POLICY_PACKET_FILTER) < 0) - return -1; - - r = snprintf(policy_name, sizeof(policy_name), "%s.packet.pre-flow", prefix); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); - } - r = DoParsePolicy(policy_name, &fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_FLOW]); - if (r < 0) + if (DetectFirewallLoadPacketPolicies(fw_policies, prefix) < 0) return -1; - if (fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_FLOW].action & ACTION_ALERT) - if (AddPktPolicySignature(fw_policies, &fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_FLOW], - DETECT_FIREWALL_POLICY_PRE_FLOW) < 0) - return -1; - - r = snprintf(policy_name, sizeof(policy_name), "%s.packet.pre-stream", prefix); - if (r < 0 || (size_t)r >= sizeof(policy_name)) { - FatalError("internal error: failed to assemble firewall policy config string"); - } - r = DoParsePolicy(policy_name, &fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_STREAM]); - if (r < 0) - return -1; - if (fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_STREAM].action & ACTION_ALERT) - if (AddPktPolicySignature(fw_policies, &fw_policies->pkt[DETECT_FIREWALL_POLICY_PRE_STREAM], - DETECT_FIREWALL_POLICY_PRE_STREAM) < 0) - return -1; for (AppProto a = 0; a < g_alproto_max; a++) { if (!AppProtoIsValid(a)) @@ -4421,8 +4449,8 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) BUG_ON(state_name == NULL); SCLogDebug("protocol %s: sub state:%s state:%s", AppProtoToString(a), sub_state_name, state_name); - if (DoParseAppSubStatePolicy(prefix, a, s, sub_state_name, state, state_name, - max_state, STREAM_TOSERVER, fw_policies) < 0) + if (DoParseAppPolicy(prefix, a, s, sub_state_name, state_name, state, max_state, + STREAM_TOSERVER, fw_policies) < 0) return -1; } /* to_client */ @@ -4434,8 +4462,8 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) BUG_ON(state_name == NULL); SCLogDebug("protocol %s: to_client: sub state:%s state:%s", AppProtoToString(a), sub_state_name, state_name); - if (DoParseAppSubStatePolicy(prefix, a, s, sub_state_name, state, state_name, - max_state, STREAM_TOCLIENT, fw_policies) < 0) + if (DoParseAppPolicy(prefix, a, s, sub_state_name, state_name, state, max_state, + STREAM_TOCLIENT, fw_policies) < 0) return -1; } } @@ -4446,8 +4474,8 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) for (uint8_t state = 0; state <= complete_state_ts; state++) { const char *name = AppLayerParserGetStateNameById(IPPROTO_TCP, a, state, STREAM_TOSERVER); - if (DoParseAppPolicy(prefix, a, name, state, complete_state_ts, STREAM_TOSERVER, - fw_policies) < 0) + if (DoParseAppPolicy(prefix, a, 0, NULL, name, state, complete_state_ts, + STREAM_TOSERVER, fw_policies) < 0) return -1; } const uint8_t complete_state_tc = @@ -4456,8 +4484,8 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) for (uint8_t state = 0; state <= complete_state_tc; state++) { const char *name = AppLayerParserGetStateNameById(IPPROTO_TCP, a, state, STREAM_TOCLIENT); - if (DoParseAppPolicy(prefix, a, name, state, complete_state_tc, STREAM_TOCLIENT, - fw_policies) < 0) + if (DoParseAppPolicy(prefix, a, 0, NULL, name, state, complete_state_tc, + STREAM_TOCLIENT, fw_policies) < 0) return -1; } } diff --git a/suricata.yaml.in b/suricata.yaml.in index c84ded125184..c4a4983bd6ba 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -2392,11 +2392,13 @@ firewall: # Default policies # - # Choose a default policy for each firewall hook. - # It is also possible to specify policies by app-layer protocol. + # Choose a default policy for each firewall hook. A `default-policy` covers + # every hook below it, so hooks that are not listed still get a policy. + # The most specific setting wins. # DNS example: Drop and alert on all DNS requests that are not allowed in firewall.rules, accept all responses. # #policies: + # default-policy: ["drop:flow"] # packet: # filter: ["drop:packet"] # app: From 092ae272e2bc040b39d30cad19e7baf344318ef8 Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:09:52 +0200 Subject: [PATCH 3/3] firewall: validate action scope against the hook class Validate the resolved scope against the class of hook it is being applied to and fail at startup if it does not fit. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 6 ++ src/detect-parse.c | 90 ++++++++++++++++++++-- 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 4ad1e6df1c06..b2eb9d38c721 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -394,3 +394,9 @@ Precedence: * app hook in a sub state: ``app...`` > ``app...default-policy`` > ``app..default-policy`` > ``app.default-policy`` > ``policies.default-policy`` > built-in (``drop:flow``) + +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``. +Cover such hooks with a more specific setting so the incompatible default never +reaches them. diff --git a/src/detect-parse.c b/src/detect-parse.c index dc3b9660825e..44d546984c19 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -109,6 +109,23 @@ typedef struct SignatureParser_ { char opts[DETECT_MAX_RULE_SIZE]; } SignatureParser; +/** Valid action scopes per firewall hook class. */ +enum DetectFirewallPolicyClass { + DETECT_FIREWALL_POLICY_CLASS_PACKET, + DETECT_FIREWALL_POLICY_CLASS_APP +}; + +static const uint8_t fw_packet_hook_scopes[] = { + ACTION_SCOPE_PACKET, + ACTION_SCOPE_HOOK, + ACTION_SCOPE_FLOW, +}; +static const uint8_t fw_app_hook_scopes[] = { + ACTION_SCOPE_FLOW, + ACTION_SCOPE_TX, + ACTION_SCOPE_HOOK, +}; + /** \brief max length of a firewall.policies YAML config path */ #define FW_POLICY_YAML_PATH_MAX 320 /** \brief max length of a single YAML path leaf segment (a hook or sub state name) */ @@ -4189,6 +4206,59 @@ 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. */ @@ -4221,13 +4291,16 @@ static void ATTR_FMT_PRINTF(2, 3) /** * \brief Resolve a firewall policy from its config path chain. * - * The first path in the chain that has a policy configured wins. + * The first path in the chain that has a policy configured wins, with its + * action scope validated against the target hook class. * * \retval 1 a config source was used and stored in \p out * \retval 0 no source present, \p out is unmodified - * \retval -1 parse error, e.g. an empty policy + * \retval -1 parse error, e.g. an empty policy, or invalid scope for the target class */ -static int ResolveFirewallPolicy(struct DetectFirewallPolicy *out, const FirewallPolicyChain *chain) + +static int ResolveFirewallPolicy(struct DetectFirewallPolicy *out, + enum DetectFirewallPolicyClass pol_class, const FirewallPolicyChain *chain) { for (uint8_t i = 0; i < chain->len; i++) { const char *path = chain->path[i]; @@ -4241,6 +4314,13 @@ static int ResolveFirewallPolicy(struct DetectFirewallPolicy *out, const Firewal SCLogError("%s: policy is set but empty", path); return -1; } + if (!FirewallScopeValidForClass(tmp.action_scope, pol_class)) { + char hint[32]; // space to combine ActionScopeToString results + FirewallScopeHintForClass(pol_class, hint, sizeof(hint)); + SCLogError("%s: action scope (\"%s\") is not valid. Valid scopes: %s", path, + ActionScopeToString(tmp.action_scope), hint); + return -1; + } *out = tmp; return 1; } @@ -4320,7 +4400,7 @@ static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const app_pol->policy.action = ACTION_DROP; app_pol->policy.action_scope = ACTION_SCOPE_FLOW; - int r = ResolveFirewallPolicy(&app_pol->policy, &chain); + int r = ResolveFirewallPolicy(&app_pol->policy, DETECT_FIREWALL_POLICY_CLASS_APP, &chain); if (r < 0) { SCFree(app_pol); return -1; @@ -4381,7 +4461,7 @@ static int DetectFirewallLoadPacketPolicy(struct DetectFirewallPolicies *fw_poli FirewallPolicyChainAdd(&chain, "%s.default-policy", prefix); struct DetectFirewallPolicy *pol = &fw_policies->pkt[id]; // built-in default - int r = ResolveFirewallPolicy(pol, &chain); + int r = ResolveFirewallPolicy(pol, DETECT_FIREWALL_POLICY_CLASS_PACKET, &chain); if (r < 0) { return -1; }