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

@@ -394,3 +394,9 @@ Precedence:
 * app hook in a sub state: ``app.<proto>.<sub state>.<hook>`` >
   ``app.<proto>.<sub state>.default-policy`` > ``app.<proto>.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.

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;
     }