app-layer: check ipproto enabled setting first

If the key `app-layer.protocols.PROTO.enabled` is present, the protocol
is enabled for all carrier protocols. This is not ideal.
Only if the key `app-layer.protocols.PROTO.enabled` is missing, an
attempt is made to look for a setting specific to the ipproto passed
at the time of registration e.g.
`app-layer.protocols.PROTO.udp.enabled`.

By default, check for carrier proto specific setting. If it is not
found, then fall back to the generic setting.
Issue a warning in case an inconsistent combination of global and
ipproto specific setting is found.

Bug 8205

doc/userguide/configuration/suricata-yaml.rst

@@ -1535,6 +1535,14 @@ the default behavior).
 
 Each supported protocol has a dedicated subsection under ``protocols``.
 
+.. note:: All applayer parsers can be enabled or disabled for specific carrier
+   protocols. Suricata first looks for carrier protocol specific setting and
+   if not found, falls back to the common enabled setting. e.g. if ``sip`` is
+   being registered, Suricata will first look if ``app-layer.protocols.sip.tcp.enabled``
+   and ``app-layer.protocols.sip.udp.enabled`` are set. If not, then a search would be
+   made for ``app-layer.protocols.sip.enabled`` and that setting would apply to both
+   SIP/TCP as well as SIP/UDP.
+
 Asn1_max_frames
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

doc/userguide/upgrade.rst

@@ -79,6 +79,10 @@ Other Changes
   really enforced and there will be no hassh computation
   even if rules try to use it.
 
+- Any inconsistent protocol enable/disable settings will issue a warning and would
+  favor ipproto specific settings over protocol's global enable/disable settings. e.g.
+  ``app-layer.protocols.sip.tcp.enabled`` would be read and preferred over
+  ``app-layer.protocols.sip.enabled``.
 
 Upgrading to 8.0.1
 ------------------

src/app-layer-detect-proto.c

@@ -1916,67 +1916,80 @@ int SCAppLayerProtoDetectConfProtoDetectionEnabledDefault(
 
     BUG_ON(ipproto == NULL || alproto == NULL);
 
-    int enabled = 1;
     char param[100];
-    SCConfNode *node;
+    SCConfNode *g_proto, *i_proto;
     int r;
+    bool g_enabled = false;
+    bool i_enabled = false;
 
     if (RunmodeIsUnittests())
-        goto enabled;
+        SCReturnInt(1);
 
 #ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
     // so that fuzzig takes place for DNP3 and such
     default_enabled = true;
 #endif
 
-    r = snprintf(param, sizeof(param), "%s%s%s", "app-layer.protocols.",
-                 alproto, ".enabled");
+    r = snprintf(param, sizeof(param), "%s%s%s%s%s", "app-layer.protocols.", alproto, ".", ipproto,
+            ".enabled");
     if (r < 0) {
         FatalError("snprintf failure.");
     } else if (r > (int)sizeof(param)) {
         FatalError("buffer not big enough to write param.");
     }
+    SCLogDebug("Looking for %s", param);
 
-    node = SCConfGetNode(param);
-    if (node == NULL) {
-        SCLogDebug("Entry for %s not found.", param);
-        r = snprintf(param, sizeof(param), "%s%s%s%s%s", "app-layer.protocols.",
-                     alproto, ".", ipproto, ".enabled");
-        if (r < 0) {
-            FatalError("snprintf failure.");
-        } else if (r > (int)sizeof(param)) {
-            FatalError("buffer not big enough to write param.");
+    i_proto = SCConfGetNode(param);
+    if (i_proto && i_proto->val) {
+        if (SCConfValIsTrue(i_proto->val)) {
+            i_enabled = true;
+        } else if (SCConfValIsFalse(i_proto->val)) {
+            i_enabled = false;
+        } else if (strcasecmp(i_proto->val, "detection-only") == 0) {
+            i_enabled = true;
+        } else {
+            FatalError("Invalid value found for %s.", param);
         }
+    }
 
-        node = SCConfGetNode(param);
-        if (node == NULL) {
-            SCLogDebug("Entry for %s not found.", param);
-            if (default_enabled) {
-                goto enabled;
-            } else {
-                goto disabled;
-            }
-        }
+    r = snprintf(param, sizeof(param), "%s%s%s", "app-layer.protocols.", alproto, ".enabled");
+    if (r < 0) {
+        FatalError("snprintf failure.");
+    } else if (r > (int)sizeof(param)) {
+        FatalError("buffer not big enough to write param.");
     }
 
-    if (node->val) {
-        if (SCConfValIsTrue(node->val)) {
-            goto enabled;
-        } else if (SCConfValIsFalse(node->val)) {
-            goto disabled;
-        } else if (strcasecmp(node->val, "detection-only") == 0) {
-            goto enabled;
+    SCLogDebug("Looking for %s", param);
+    g_proto = SCConfGetNode(param);
+    if (g_proto && g_proto->val) {
+        if (SCConfValIsTrue(g_proto->val)) {
+            g_enabled = true;
+        } else if (SCConfValIsFalse(g_proto->val)) {
+            g_enabled = false;
+        } else if (strcasecmp(g_proto->val, "detection-only") == 0) {
+            g_enabled = true;
+        } else {
+            FatalError("Invalid value found for %s", param);
         }
     }
 
-    /* Invalid or null value. */
-    SCLogError("Invalid value found for %s.", param);
-    exit(EXIT_FAILURE);
+    if ((i_proto && g_proto) && (i_enabled ^ g_enabled)) {
+        SCLogWarning("Inconsistent global (%s) and respective ipproto (%s) settings found for "
+                     "alproto %s and ipproto %s",
+                g_enabled ? "TRUE" : "FALSE", i_enabled ? "TRUE" : "FALSE", alproto, ipproto);
+    }
+
+    if (i_proto) {
+        SCReturnInt(i_enabled);
+    }
+    if (g_proto) {
+        SCReturnInt(g_enabled);
+    }
+    if (!default_enabled) {
+        SCReturnInt(0);
+    }
 
- disabled:
-    enabled = 0;
- enabled:
-    SCReturnInt(enabled);
+    SCReturnInt(1);
 }
 
 int SCAppLayerProtoDetectConfProtoDetectionEnabled(const char *ipproto, const char *alproto)

src/app-layer-parser.c

@@ -346,55 +346,73 @@ int SCAppLayerParserConfParserEnabled(const char *ipproto, const char *alproto_n
 {
     SCEnter();
 
-    int enabled = 1;
     char param[100];
-    SCConfNode *node;
+    SCConfNode *g_proto, *i_proto;
+    bool g_enabled = false;
+    bool i_enabled = false;
     int r;
 
     if (RunmodeIsUnittests())
-        goto enabled;
+        SCReturnInt(1);
 
-    r = snprintf(param, sizeof(param), "%s%s%s", "app-layer.protocols.",
-                 alproto_name, ".enabled");
+    r = snprintf(param, sizeof(param), "%s%s%s%s%s", "app-layer.protocols.", alproto_name, ".",
+            ipproto, ".enabled");
     if (r < 0) {
         FatalError("snprintf failure.");
     } else if (r > (int)sizeof(param)) {
         FatalError("buffer not big enough to write param.");
     }
-
-    node = SCConfGetNode(param);
-    if (node == NULL) {
-        SCLogDebug("Entry for %s not found.", param);
-        r = snprintf(param, sizeof(param), "%s%s%s%s%s", "app-layer.protocols.",
-                     alproto_name, ".", ipproto, ".enabled");
-        if (r < 0) {
-            FatalError("snprintf failure.");
-        } else if (r > (int)sizeof(param)) {
-            FatalError("buffer not big enough to write param.");
+    SCLogDebug("Looking for %s", param);
+
+    i_proto = SCConfGetNode(param);
+    if (i_proto && i_proto->val) {
+        if (SCConfValIsTrue(i_proto->val)) {
+            i_enabled = true;
+        } else if (SCConfValIsFalse(i_proto->val)) {
+            i_enabled = false;
+        } else if (strcasecmp(i_proto->val, "detection-only") == 0) {
+            i_enabled = false;
+        } else {
+            FatalError("Invalid value found for %s.", param);
         }
+    }
 
-        node = SCConfGetNode(param);
-        if (node == NULL) {
-            SCLogDebug("Entry for %s not found.", param);
-            goto enabled;
+    r = snprintf(param, sizeof(param), "%s%s%s", "app-layer.protocols.", alproto_name, ".enabled");
+    if (r < 0) {
+        FatalError("snprintf failure.");
+    } else if (r > (int)sizeof(param)) {
+        FatalError("buffer not big enough to write param.");
+    }
+
+    SCLogDebug("Looking for %s", param);
+    g_proto = SCConfGetNode(param);
+    if (g_proto && g_proto->val) {
+        if (SCConfValIsTrue(g_proto->val)) {
+            g_enabled = true;
+        } else if (SCConfValIsFalse(g_proto->val)) {
+            g_enabled = false;
+        } else if (strcasecmp(g_proto->val, "detection-only") == 0) {
+            g_enabled = false;
+        } else {
+            FatalError("Invalid value found for %s", param);
         }
     }
 
-    if (SCConfValIsTrue(node->val)) {
-        goto enabled;
-    } else if (SCConfValIsFalse(node->val)) {
-        goto disabled;
-    } else if (strcasecmp(node->val, "detection-only") == 0) {
-        goto disabled;
-    } else {
-        SCLogError("Invalid value found for %s.", param);
-        exit(EXIT_FAILURE);
+    if ((i_proto && g_proto) && (i_enabled ^ g_enabled)) {
+        /* these checks are also performed for detection-only, no need to issue double warning */
+        SCLogDebug("Inconsistent global (%s) and respective ipproto (%s) settings found for "
+                   "alproto %s and ipproto %s",
+                g_enabled ? "TRUE" : "FALSE", i_enabled ? "TRUE" : "FALSE", alproto_name, ipproto);
+    }
+
+    if (i_proto) {
+        SCReturnInt(i_enabled);
+    }
+    if (g_proto) {
+        SCReturnInt(g_enabled);
     }
 
- disabled:
-    enabled = 0;
- enabled:
-    SCReturnInt(enabled);
+    SCReturnInt(1);
 }
 
 /***** Parser related registration *****/

suricata.yaml.in

@@ -925,6 +925,8 @@ pcap-file:
 # The option "enabled" takes 3 values - "yes", "no", "detection-only".
 # "yes" enables both detection and the parser, "no" disables both, and
 # "detection-only" enables protocol detection only (parser disabled).
+# This option is first evaluated per carrier protocol e.g. sip.tcp.enabled will be
+# evaluated first and if it is not found then, sip.enabled will be checked.
 app-layer:
   # error-policy: ignore
   protocols: