From 5391c4427ba7e34b574425c4cf6222b2e0a0feb0 Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:08:32 +0200 Subject: [PATCH 1/4] detect/firewall: address HTTP/1 policies as http1 AppProtoToString(ALPROTO_HTTP1) returns "http", so an HTTP/1 policy had to be written as `http:` while its rule hooks were already spelled `http1:`. Use the same name in both places. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 2 +- doc/userguide/firewall/firewall-example.rst | 2 +- src/detect-parse.c | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 36951962f851..b795e6bb51b7 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -62,7 +62,7 @@ Application layer tables ~~~~~~~~~~~~~~~~~~~~~~~~ If applayer is available, rules from the following tables apply. The tables for the -application layer are per app layer protocol and per protocol state. e.g. ``http:request_line``. +application layer are per app layer protocol and per protocol state. e.g. ``http1:request_line``. .. table:: diff --git a/doc/userguide/firewall/firewall-example.rst b/doc/userguide/firewall/firewall-example.rst index 002de6da7eeb..a4f6049c86e7 100644 --- a/doc/userguide/firewall/firewall-example.rst +++ b/doc/userguide/firewall/firewall-example.rst @@ -67,7 +67,7 @@ In the example below: the config auto accepts various hooks, leaving just ``http firewall: policies: - http: + http1: request-started: - "accept:hook" request-line: diff --git a/src/detect-parse.c b/src/detect-parse.c index 758ca54d030a..b0dc5f5fa3ee 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -4178,7 +4178,7 @@ static int DoParseAppSubStatePolicy(const char *prefix, const AppProto app_proto nname[i] = '-'; } - const char *app_name = AppProtoToString(app_proto); + const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(app_proto); int r = snprintf(policy_name, sizeof(policy_name), "%s.%s.%s.%s", prefix, app_name, sub_state_name, nname); SCLogDebug("policy_name %s", policy_name); @@ -4247,7 +4247,7 @@ static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const nname[i] = '-'; } - const char *app_name = AppProtoToString(app_proto); + const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(app_proto); int r = snprintf(policy_name, sizeof(policy_name), "%s.%s.%s", prefix, app_name, nname); SCFree(nname); if (r < 0 || (size_t)r >= sizeof(policy_name)) { From 2b17b7ee838af69325a8298b4a5d15bb649041fb Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:09:29 +0200 Subject: [PATCH 2/4] detect/firewall: group policies under packet and app nodes The policy config was a flat map mixing packet hooks and app-layer protocols: `packet-filter` next to `dns`. There was no node that meant "the packet hooks" or "the app-layer hooks", so a setting could not be scoped to one group. Move each group under its own node: packet-filter -> packet.filter packet-pre-flow -> packet.pre-flow packet-pre-stream -> packet.pre-stream . -> app.. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 27 +++++----- doc/userguide/firewall/firewall-example.rst | 59 +++++++++++---------- src/detect-parse.c | 12 ++--- suricata.yaml.in | 12 +++-- 4 files changed, 58 insertions(+), 52 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index b795e6bb51b7..ffee070fe25c 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -349,29 +349,32 @@ 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:filter`` hooks applies ``drop:flow``. +Each hook has a default policy. By default ``packet.filter`` enforces a ``drop:packet`` policy and the +``app`` hooks apply ``drop:flow``. -The policies can be configured in ``firewall`` block in the config. +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:: +Example for ``packet.filter``, to use reject instead of drop:: firewall: policies: - packet-filter: [ "reject:packet" ] + packet: + filter: [ "reject:packet" ] Example for DNS:: firewall: policies: - dns: - request-started: ["accept:hook"] + app: + dns: + request-started: ["accept:hook"] - # Drop and alert on all DNS requests that are not allowed in - # firewall.rules. - request-complete: ["drop:flow", "alert"] + # 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"] + # Accept all responses. + response-started: ["accept:tx"] diff --git a/doc/userguide/firewall/firewall-example.rst b/doc/userguide/firewall/firewall-example.rst index a4f6049c86e7..5ada751d03a2 100644 --- a/doc/userguide/firewall/firewall-example.rst +++ b/doc/userguide/firewall/firewall-example.rst @@ -67,35 +67,36 @@ In the example below: the config auto accepts various hooks, leaving just ``http firewall: policies: - http1: - request-started: - - "accept:hook" - request-line: - - "drop:flow" - - "alert" - request-headers: - - "drop:flow" - - "alert" - request-body: - - "accept:hook" - request-trailer: - - "accept:hook" - request-complete: - - "accept:hook" - - response-started: - - "accept:hook" - response-line: - - "drop:flow" - - "alert" - response-headers: - - "accept:hook" - response-body: - - "accept:hook" - response-trailer: - - "accept:hook" - response-complete: - - "accept:hook" + app: + http1: + request-started: + - "accept:hook" + request-line: + - "drop:flow" + - "alert" + request-headers: + - "drop:flow" + - "alert" + request-body: + - "accept:hook" + request-trailer: + - "accept:hook" + request-complete: + - "accept:hook" + + response-started: + - "accept:hook" + response-line: + - "drop:flow" + - "alert" + response-headers: + - "accept:hook" + response-body: + - "accept:hook" + response-trailer: + - "accept:hook" + response-complete: + - "accept:hook" :: diff --git a/src/detect-parse.c b/src/detect-parse.c index b0dc5f5fa3ee..8736094feb8f 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -4179,7 +4179,7 @@ static int DoParseAppSubStatePolicy(const char *prefix, const AppProto app_proto } const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(app_proto); - int r = snprintf(policy_name, sizeof(policy_name), "%s.%s.%s.%s", prefix, app_name, + 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); @@ -4248,7 +4248,7 @@ static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const } const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(app_proto); - int r = snprintf(policy_name, sizeof(policy_name), "%s.%s.%s", prefix, app_name, nname); + 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"); @@ -4282,7 +4282,7 @@ static int DoParseAppPolicy(const char *prefix, const AppProto app_proto, const } if (hookname == NULL) return 0; - r = snprintf(policy_name, sizeof(policy_name), "%s.%s.%s", prefix, app_name, hookname); + 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"); } @@ -4346,7 +4346,7 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) if (fw_policies == NULL) return -1; - r = snprintf(policy_name, sizeof(policy_name), "%s.packet-filter", prefix); + 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"); } @@ -4359,7 +4359,7 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) DETECT_FIREWALL_POLICY_PACKET_FILTER) < 0) return -1; - r = snprintf(policy_name, sizeof(policy_name), "%s.packet-pre-flow", prefix); + 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"); } @@ -4371,7 +4371,7 @@ int DetectFirewallLoadDefaultPolicies(DetectEngineCtx *de_ctx) DETECT_FIREWALL_POLICY_PRE_FLOW) < 0) return -1; - r = snprintf(policy_name, sizeof(policy_name), "%s.packet-pre-stream", prefix); + 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"); } diff --git a/suricata.yaml.in b/suricata.yaml.in index 6ce30560aaae..da5815860e87 100644 --- a/suricata.yaml.in +++ b/suricata.yaml.in @@ -2397,11 +2397,13 @@ firewall: # DNS example: Drop and alert on all DNS requests that are not allowed in firewall.rules, accept all responses. # #policies: - # packet-filter: ["drop:packet"] - # dns: - # request-started: ["accept:hook"] - # request-complete: ["drop:flow", "alert"] - # response-started: ["accept:tx"] + # packet: + # filter: ["drop:packet"] + # app: + # dns: + # request-started: ["accept:hook"] + # request-complete: ["drop:flow", "alert"] + # response-started: ["accept:tx"] ## ## Include other configs From b5ced77f2628d9371234ea5837761093df0879f4 Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:09:52 +0200 Subject: [PATCH 3/4] detect/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. Add a `default-policy` setting that covers all hooks below it, so unlisted hooks still get a policy. For any hook the most specific setting present wins: app... app...default-policy app..default-policy app.default-policy default-policy built-in The packet hooks follow the same pattern under `packet`. Resolution moves into ResolveFirewallPolicy(), which walks the candidate paths most-specific-first and stops at the first one that is configured. A path that is present but empty is now a startup error rather than being treated as unset. DoParseAppSubStatePolicy() collapses into DoParseAppPolicy() as a sub state hook only differs by an extra path segment. Path assembly and hook-name normalisation move to helpers now that both are needed in more places. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 55 ++-- src/detect-parse.c | 330 +++++++++++---------- suricata.yaml.in | 6 +- 3 files changed, 222 insertions(+), 169 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index ffee070fe25c..71e5f99e23fc 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -349,32 +349,51 @@ 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`` +may be given at several levels; for any hook 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: ["drop: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"] +Protocols whose hooks are grouped into sub states, such as HTTP/2, take an extra +level for the sub state name:: + + firewall: + policies: + app: + 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 8736094feb8f..a5c2b52b4639 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -4161,134 +4161,161 @@ 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 Assemble a firewall.policies config path, fatal on truncation. + */ +static void ATTR_FMT_PRINTF(3, 4) + FirewallPolicyPath(char *out_buf, size_t out_buf_sz, 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] = '-'; - } + va_list ap; + va_start(ap, fmt); + int r = vsnprintf(out_buf, out_buf_sz, fmt, ap); + va_end(ap); + if (r < 0 || (size_t)r >= out_buf_sz) + FatalError("Failed to assemble firewall policy config string"); +} - const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(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"); +/** + * \brief Resolve a firewall policy from the list of config paths. + * + * Paths are most-specific-first. The first path 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 char *const *paths, const int npaths) +{ + for (int i = 0; i < npaths; i++) { + if (paths[i] == NULL) { + continue; + } + struct DetectFirewallPolicy tmp = { 0 }; + int r = DoParsePolicy(paths[i], &tmp); + if (r < 0) { + return -1; + } + if (r == 1) { + if (tmp.action == 0) { + SCLogError("%s: policy is set but empty", paths[i]); + return -1; + } + *out = tmp; + return 1; + } } + return 0; +} - 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; - - r = DoParsePolicy(policy_name, &app_pol->policy); - if (r < 0) { - SCFree(app_pol); - return -1; - } +/** + * \brief Generic start/complete hook alias for an app progress state, in config + * form (hyphens), or NULL for intermediate states. + */ +static const char *FirewallAppGenericHookName( + 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; +} - if (HashTableAdd(fw_policies->app_policies, app_pol, 0) != 0) { - FatalError("internal error: insert policy into hash table"); - } - /* 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); +static void FirewallHookNameConvertUnderscoreToDash(const char *in, char *out, size_t out_size) +{ + strlcpy(out, in, out_size); + 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. The policy is resolved most-specific + * first, e.g. for a sub state hook: + * + * .app... + * .app... + * .app...default-policy + * .app..default-policy + * .app.default-policy + * .default-policy + */ +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) - return -1; - for (int i = 0; nname[i] != '\0'; i++) { - if (nname[i] == '_') - nname[i] = '-'; + const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(app_proto); + // Generic serves for the first and the last state, NULL otherwise + const char *generic = FirewallAppGenericHookName(state, complete_state, direction); + + char primary[64]; + if (hookname != NULL) { + FirewallHookNameConvertUnderscoreToDash(hookname, primary, sizeof(primary)); + } else if (generic != NULL) { + strlcpy(primary, generic, sizeof(primary)); + } else { + return 0; } - const char *app_name = (app_proto == ALPROTO_HTTP1) ? "http1" : AppProtoToString(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"); + char scope[256]; + if (sub_state_name != NULL) { + char sub[64]; + FirewallHookNameConvertUnderscoreToDash(sub_state_name, sub, sizeof(sub)); + FirewallPolicyPath(scope, sizeof(scope), "%s.app.%s.%s", prefix, app_name, sub); + } else { + FirewallPolicyPath(scope, sizeof(scope), "%s.app.%s", prefix, app_name); } + char primary_key[320], generic_key[320], hook_dflt[320], proto_dflt[320], app_dflt[320], + global_dflt[320]; + FirewallPolicyPath(primary_key, sizeof(primary_key), "%s.%s", scope, primary); + if (generic != NULL && strcmp(primary, generic) != 0) { + FirewallPolicyPath(generic_key, sizeof(generic_key), "%s.%s", scope, generic); + } else { + generic_key[0] = '\0'; + } + FirewallPolicyPath(hook_dflt, sizeof(hook_dflt), "%s.default-policy", scope); + FirewallPolicyPath( + proto_dflt, sizeof(proto_dflt), "%s.app.%s.default-policy", prefix, app_name); + FirewallPolicyPath(app_dflt, sizeof(app_dflt), "%s.app.default-policy", prefix); + FirewallPolicyPath(global_dflt, sizeof(global_dflt), "%s.default-policy", prefix); + + const char *paths[] = { + // .app.[.]. + primary_key, + // .app.[.]. + generic_key[0] != '\0' ? generic_key : NULL, + // .app.[.].default-policy + hook_dflt, + // .app..default-policy + sub_state_name != NULL ? proto_dflt : NULL, + // .app.default-policy + app_dflt, + // .default-policy + global_dflt, + }; + 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 - * is a config for this hook. */ + /* built-in default, overwritten by ResolveFirewallPolicy if any of the + * config paths above has a policy. */ 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, paths, (int)ARRAY_SIZE(paths)); if (r < 0) { SCFree(app_pol); return -1; @@ -4333,10 +4360,49 @@ 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) +{ + char specific[256], pkt_dflt[256], global_dflt[256]; + FirewallPolicyPath(specific, sizeof(specific), "%s.packet.%s", prefix, leaf); + FirewallPolicyPath(pkt_dflt, sizeof(pkt_dflt), "%s.packet.default-policy", prefix); + FirewallPolicyPath(global_dflt, sizeof(global_dflt), "%s.default-policy", prefix); + + struct DetectFirewallPolicy *pol = &fw_policies->pkt[id]; // built-in default + const char *paths[] = { specific, pkt_dflt, global_dflt }; + int r = ResolveFirewallPolicy(pol, paths, (int)ARRAY_SIZE(paths)); + 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); @@ -4346,42 +4412,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) - 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) + if (DetectFirewallLoadPacketPolicies(fw_policies, prefix) < 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)) @@ -4409,8 +4441,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 */ @@ -4422,8 +4454,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; } } @@ -4434,8 +4466,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 = @@ -4444,8 +4476,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 da5815860e87..e94e00696f82 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 04309fbf36835a7bfc1dec043ec876e37a59c56c Mon Sep 17 00:00:00 2001 From: Lukas Sismis Date: Tue, 28 Jul 2026 18:09:52 +0200 Subject: [PATCH 4/4] detect/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, naming the config path and the scopes that would be accepted there. A global `accept:tx` is now a startup error. Ticket: 8712 --- doc/userguide/firewall/firewall-design.rst | 6 ++ src/detect-parse.c | 87 ++++++++++++++++++++-- src/detect.h | 5 ++ 3 files changed, 92 insertions(+), 6 deletions(-) diff --git a/doc/userguide/firewall/firewall-design.rst b/doc/userguide/firewall/firewall-design.rst index 71e5f99e23fc..96abd4e1558c 100644 --- a/doc/userguide/firewall/firewall-design.rst +++ b/doc/userguide/firewall/firewall-design.rst @@ -397,3 +397,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 a5c2b52b4639..f42f45f59cae 100644 --- a/src/detect-parse.c +++ b/src/detect-parse.c @@ -109,6 +109,19 @@ typedef struct SignatureParser_ { char opts[DETECT_MAX_RULE_SIZE]; } SignatureParser; +/** Valid action scopes per firewall hook class. Single source of truth for both + * scope validation and the human-readable "a/b/c" hint in error messages. */ +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, +}; + const char *DetectListToHumanString(int list) { #define CASE_CODE_STRING(E, S) case E: return S; break @@ -4161,6 +4174,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); + } + strlcat(out, ActionScopeToString((enum ActionScope)set[i]), out_size); + } +} + /** * \brief Assemble a firewall.policies config path, fatal on truncation. */ @@ -4179,14 +4245,14 @@ static void ATTR_FMT_PRINTF(3, 4) * \brief Resolve a firewall policy from the list of config paths. * * Paths are most-specific-first. The first path that has a policy configured - * wins. + * 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 char *const *paths, const int npaths) +static int ResolveFirewallPolicy(struct DetectFirewallPolicy *out, + enum DetectFirewallPolicyClass pol_class, const char *const *paths, const int npaths) { for (int i = 0; i < npaths; i++) { if (paths[i] == NULL) { @@ -4202,6 +4268,13 @@ static int ResolveFirewallPolicy( SCLogError("%s: policy is set but empty", paths[i]); return -1; } + if (!FirewallScopeValidForClass(tmp.action_scope, pol_class)) { + char hint[32]; + FirewallScopeHintForClass(pol_class, hint, sizeof(hint)); + SCLogError("%s: action scope (\"%s\") is not valid. Valid scopes: %s", paths[i], + ActionScopeToString(tmp.action_scope), hint); + return -1; + } *out = tmp; return 1; } @@ -4315,7 +4388,8 @@ 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, paths, (int)ARRAY_SIZE(paths)); + int r = ResolveFirewallPolicy( + &app_pol->policy, DETECT_FIREWALL_POLICY_CLASS_APP, paths, (int)ARRAY_SIZE(paths)); if (r < 0) { SCFree(app_pol); return -1; @@ -4373,7 +4447,8 @@ static int DetectFirewallLoadPacketPolicy(struct DetectFirewallPolicies *fw_poli struct DetectFirewallPolicy *pol = &fw_policies->pkt[id]; // built-in default const char *paths[] = { specific, pkt_dflt, global_dflt }; - int r = ResolveFirewallPolicy(pol, paths, (int)ARRAY_SIZE(paths)); + int r = ResolveFirewallPolicy( + pol, DETECT_FIREWALL_POLICY_CLASS_PACKET, paths, (int)ARRAY_SIZE(paths)); if (r < 0) { return -1; } diff --git a/src/detect.h b/src/detect.h index c71268023669..e60b74c7fd32 100644 --- a/src/detect.h +++ b/src/detect.h @@ -921,6 +921,11 @@ enum DetectEngineType DETECT_ENGINE_TYPE_TENANT = 3, }; +enum DetectFirewallPolicyClass { + DETECT_FIREWALL_POLICY_CLASS_PACKET, + DETECT_FIREWALL_POLICY_CLASS_APP +}; + enum DetectFirewallPacketPolicies { DETECT_FIREWALL_POLICY_PACKET_FILTER, DETECT_FIREWALL_POLICY_PRE_FLOW,