From: Jason Ish <jason.ish@oisf.net>
Date: Wed, 20 Nov 2024 16:46:38 +0000 (-0600)
Subject: requires: treat unknown requires keywords as unmet requirements
X-Git-Tag: suricata-8.0.0-beta1~673
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=820a3e51b752867da1322f29d542e5844bb6e727;p=thirdparty%2Fsuricata.git

requires: treat unknown requires keywords as unmet requirements

For example, "requires: foo bar" is an unknown requirement, however
its not tracked, nor an error as it follows the syntax. Instead,
record these unknown keywords, and fail the requirements check if any
are present.

A future version of Suricata may have new requires keywords, for
example a check for keywords.

Ticket: #7418
---

diff --git a/doc/userguide/rules/meta.rst b/doc/userguide/rules/meta.rst
index 1ceb5fe834..89a98972e7 100644
--- a/doc/userguide/rules/meta.rst
+++ b/doc/userguide/rules/meta.rst
@@ -212,6 +212,8 @@ If the value is src_ip then the source IP in the generated event (src_ip
 field in JSON) is the target of the attack. If target is set to dest_ip
 then the target is the destination IP in the generated event.
 
+.. _keyword_requires:
+
 requires
 --------
 
@@ -220,6 +222,11 @@ features to be enabled, or the Suricata version to match an
 expression. Rules that do not meet the requirements will by ignored,
 and Suricata will not treat them as errors.
 
+Requirements that follow the valid format of ``<keyword>
+<expression>`` but are not known to Suricata are allowed for future
+compatiblity, however unknown requirement expressions will lead to the
+requirement not being met, skipping the rule.
+
 When parsing rules, the parser attempts to process the ``requires``
 keywords before others. This allows it to occur after keywords that
 may only be present in specific versions of Suricata, as specified by
diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst
index 63e2146280..7172c6b1c8 100644
--- a/doc/userguide/upgrade.rst
+++ b/doc/userguide/upgrade.rst
@@ -79,6 +79,9 @@ Major changes
     - sip.content_length
 - Napatech support has been moved to a capture plugin. See :doc:`Napatech plugin
   <upgrade/8.0-napatech-plugin>`.
+- Unknown requirements in the ``requires`` keyword will now be treated
+  as unmet requirements, causing the rule to not be loaded. See
+  :ref:`keyword_requires`.
 
 Removals
 ~~~~~~~~
diff --git a/rust/src/detect/requires.rs b/rust/src/detect/requires.rs
index e9e1acac50..2635605d26 100644
--- a/rust/src/detect/requires.rs
+++ b/rust/src/detect/requires.rs
@@ -57,6 +57,9 @@ enum RequiresError {
 
     /// Passed in requirements not a valid UTF-8 string.
     Utf8Error,
+
+    /// An unknown requirement was provided.
+    UnknownRequirement(String),
 }
 
 impl RequiresError {
@@ -70,6 +73,7 @@ impl RequiresError {
             Self::BadRequires => "Failed to parse requires expression\0",
             Self::MultipleVersions => "Version may only be specified once\0",
             Self::Utf8Error => "Requires expression is not valid UTF-8\0",
+            Self::UnknownRequirement(_) => "Unknown requirements\0",
         };
         msg.as_ptr() as *const c_char
     }
@@ -169,6 +173,9 @@ struct Requires {
     /// - All of the inner most must evaluate to true.
     /// - To pass, any of the outer must be true.
     pub version: Vec<Vec<RuleRequireVersion>>,
+
+    /// Unknown parameters to requires.
+    pub unknown: Vec<String>,
 }
 
 fn parse_op(input: &str) -> IResult<&str, VersionCompareOp> {
@@ -242,6 +249,7 @@ fn parse_requires(mut input: &str) -> Result<Requires, RequiresError> {
                 // Unknown keyword, allow by warn in case we extend
                 // this in the future.
                 SCLogWarning!("Unknown requires keyword: {}", keyword);
+                requires.unknown.push(format!("{} {}", keyword, value));
             }
         }
 
@@ -291,6 +299,12 @@ fn check_version(
 fn check_requires(
     requires: &Requires, suricata_version: &SuricataVersion,
 ) -> Result<(), RequiresError> {
+    if !requires.unknown.is_empty() {
+        return Err(RequiresError::UnknownRequirement(
+            requires.unknown.join(","),
+        ));
+    }
+
     if !requires.version.is_empty() {
         let mut errs = VecDeque::new();
         let mut ok = 0;
@@ -594,6 +608,7 @@ mod test {
                         patch: 0,
                     }
                 }]],
+                unknown: vec![],
             }
         );
 
@@ -610,6 +625,7 @@ mod test {
                         patch: 0,
                     }
                 }]],
+                unknown: vec![],
             }
         );
 
@@ -626,6 +642,7 @@ mod test {
                         patch: 2,
                     }
                 }]],
+                unknown: vec![],
             }
         );
 
@@ -652,6 +669,7 @@ mod test {
                         }
                     }
                 ]],
+                unknown: vec![],
             }
         );
     }
@@ -748,11 +766,11 @@ mod test {
         assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 0)).is_err());
 
         // Unknown keyword.
-        let requires = parse_requires("feature lua, foo bar, version >= 7.0.3").unwrap();
+        let requires = parse_requires("feature true_lua, foo bar, version >= 7.0.3").unwrap();
         assert_eq!(
             requires,
             Requires {
-                features: vec!["lua".to_string()],
+                features: vec!["true_lua".to_string()],
                 version: vec![vec![RuleRequireVersion {
                     op: VersionCompareOp::Gte,
                     version: SuricataVersion {
@@ -761,8 +779,14 @@ mod test {
                         patch: 3,
                     }
                 }]],
+                unknown: vec!["foo bar".to_string()],
             }
         );
+
+        // This should not pass the requires check as it contains an
+        // unknown requires keyword.
+        //check_requires(&requires, &SuricataVersion::new(8, 0, 0)).unwrap();
+        assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0)).is_err());
     }
 
     #[test]
@@ -802,4 +826,10 @@ mod test {
             ]
         );
     }
+
+    #[test]
+    fn test_requires_keyword() {
+        let requires = parse_requires("keyword true_bar").unwrap();
+        assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0)).is_err());
+    }
 }