From 26fd1304126f80486114f74a49c88711e8d22f78 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 20 Nov 2024 10:46:38 -0600 Subject: [PATCH 1/2] 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 (cherry picked from commit 820a3e51b752867da1322f29d542e5844bb6e727) --- doc/userguide/rules/meta.rst | 7 +++++++ doc/userguide/upgrade.rst | 6 ++++++ rust/src/detect/requires.rs | 34 ++++++++++++++++++++++++++++++++-- 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/doc/userguide/rules/meta.rst b/doc/userguide/rules/meta.rst index 1ceb5fe834e0..89a98972e7ae 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 `` +`` 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 170c95e80361..cea1d2ee0f55 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -34,6 +34,12 @@ also check all the new features that have been added but are not covered by this guide. Those features are either not enabled by default or require dedicated new configuration. +Upgrading to 7.0.7 +------------------ +- Unknown requirements in the ``requires`` keyword will now be treated + as unmet requirements, causing the rule to not be loaded. See + :ref:`keyword_requires`. + Upgrading 6.0 to 7.0 -------------------- diff --git a/rust/src/detect/requires.rs b/rust/src/detect/requires.rs index e9e1acac5087..2635605d265d 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>, + + /// Unknown parameters to requires. + pub unknown: Vec, } fn parse_op(input: &str) -> IResult<&str, VersionCompareOp> { @@ -242,6 +249,7 @@ fn parse_requires(mut input: &str) -> Result { // 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()); + } } From d5dadaf4f8bc54e4c9ba473af0f2e9a8b243f643 Mon Sep 17 00:00:00 2001 From: Jason Ish Date: Wed, 4 Dec 2024 11:28:17 -0600 Subject: [PATCH 2/2] requires: add option to ignore unknown requirements The new behavior in 8, and backported is to treat unknown requirements as unsatisfied requirements. For 7.0.8, add a configuration option, "ignore-unknown-requirements" to completely ignore unknown requirements, effectively treating them as available. Ticket: #7434 --- doc/userguide/upgrade.rst | 19 +++++++++-- rust/src/detect/requires.rs | 65 +++++++++++++++++++------------------ src/detect-requires.c | 9 ++++- 3 files changed, 59 insertions(+), 34 deletions(-) diff --git a/doc/userguide/upgrade.rst b/doc/userguide/upgrade.rst index cea1d2ee0f55..6b7f4ab24d78 100644 --- a/doc/userguide/upgrade.rst +++ b/doc/userguide/upgrade.rst @@ -37,8 +37,23 @@ dedicated new configuration. Upgrading to 7.0.7 ------------------ - Unknown requirements in the ``requires`` keyword will now be treated - as unmet requirements, causing the rule to not be loaded. See - :ref:`keyword_requires`. + as unsatisfied requirements, causing the rule to not be loaded. See + :ref:`keyword_requires`. To opt out of this change and to ignore + uknown requirements, effectively treating them as satified the + ``ignore-unknown-requirements`` configuration option can be used. + + Command line example:: + + --set ignore-unknown-requirements=true + + Or as a top-level configuration option in ``suricata.yaml``: + + .. code-block:: yaml + + default-rule-path: /var/lib/suricata/rules + rule-files: + - suricata.rules + ignore-unknown-requirements: true Upgrading 6.0 to 7.0 -------------------- diff --git a/rust/src/detect/requires.rs b/rust/src/detect/requires.rs index 2635605d265d..bce9031ac0ca 100644 --- a/rust/src/detect/requires.rs +++ b/rust/src/detect/requires.rs @@ -297,9 +297,9 @@ fn check_version( } fn check_requires( - requires: &Requires, suricata_version: &SuricataVersion, + requires: &Requires, suricata_version: &SuricataVersion, ignore_unknown: bool, ) -> Result<(), RequiresError> { - if !requires.unknown.is_empty() { + if !ignore_unknown && !requires.unknown.is_empty() { return Err(RequiresError::UnknownRequirement( requires.unknown.join(","), )); @@ -453,7 +453,7 @@ pub unsafe extern "C" fn SCDetectRequiresStatusLog( #[no_mangle] pub unsafe extern "C" fn SCDetectCheckRequires( requires: *const c_char, suricata_version_string: *const c_char, errstr: *mut *const c_char, - status: &mut SCDetectRequiresStatus, + status: &mut SCDetectRequiresStatus, ignore_unknown: c_int, ) -> c_int { // First parse the running Suricata version. let suricata_version = match parse_suricata_version(CStr::from_ptr(suricata_version_string)) { @@ -476,7 +476,9 @@ pub unsafe extern "C" fn SCDetectCheckRequires( } }; - match check_requires(&requires, &suricata_version) { + let ignore_unknown = ignore_unknown != 0; + + match check_requires(&requires, &suricata_version, ignore_unknown) { Ok(()) => 0, Err(err) => { match &err { @@ -497,6 +499,7 @@ pub unsafe extern "C" fn SCDetectCheckRequires( RequiresError::VersionGt => { status.gt_count += 1; } + RequiresError::UnknownRequirement(_) => {} _ => {} } *errstr = err.c_errmsg(); @@ -680,7 +683,7 @@ mod test { let suricata_version = SuricataVersion::new(7, 0, 4); let requires = parse_requires("version >= 8").unwrap(); assert_eq!( - check_requires(&requires, &suricata_version), + check_requires(&requires, &suricata_version, false), Err(RequiresError::VersionLt(SuricataVersion { major: 8, minor: 0, @@ -691,79 +694,79 @@ mod test { // Have 7.0.4, require 7.0.3. let suricata_version = SuricataVersion::new(7, 0, 4); let requires = parse_requires("version >= 7.0.3").unwrap(); - assert_eq!(check_requires(&requires, &suricata_version), Ok(())); + assert_eq!(check_requires(&requires, &suricata_version, false), Ok(())); // Have 8.0.0, require >= 7.0.0 and < 8.0 let suricata_version = SuricataVersion::new(8, 0, 0); let requires = parse_requires("version >= 7.0.0 < 8").unwrap(); assert_eq!( - check_requires(&requires, &suricata_version), + check_requires(&requires, &suricata_version, false), Err(RequiresError::VersionGt) ); // Have 8.0.0, require >= 7.0.0 and < 9.0 let suricata_version = SuricataVersion::new(8, 0, 0); let requires = parse_requires("version >= 7.0.0 < 9").unwrap(); - assert_eq!(check_requires(&requires, &suricata_version), Ok(())); + assert_eq!(check_requires(&requires, &suricata_version, false), Ok(())); // Require feature foobar. let suricata_version = SuricataVersion::new(8, 0, 0); let requires = parse_requires("feature foobar").unwrap(); assert_eq!( - check_requires(&requires, &suricata_version), + check_requires(&requires, &suricata_version, false), Err(RequiresError::MissingFeature("foobar".to_string())) ); // Require feature foobar, but this time we have the feature. let suricata_version = SuricataVersion::new(8, 0, 0); let requires = parse_requires("feature true_foobar").unwrap(); - assert_eq!(check_requires(&requires, &suricata_version), Ok(())); + assert_eq!(check_requires(&requires, &suricata_version, false), Ok(())); let suricata_version = SuricataVersion::new(8, 0, 1); let requires = parse_requires("version >= 7.0.3 < 8").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_err()); + assert!(check_requires(&requires, &suricata_version, false).is_err()); let suricata_version = SuricataVersion::new(7, 0, 1); let requires = parse_requires("version >= 7.0.3 < 8").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_err()); + assert!(check_requires(&requires, &suricata_version, false).is_err()); let suricata_version = SuricataVersion::new(7, 0, 3); let requires = parse_requires("version >= 7.0.3 < 8").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_ok()); + assert!(check_requires(&requires, &suricata_version, false).is_ok()); let suricata_version = SuricataVersion::new(8, 0, 3); let requires = parse_requires("version >= 7.0.3 < 8 | >= 8.0.3").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_ok()); + assert!(check_requires(&requires, &suricata_version, false).is_ok()); let suricata_version = SuricataVersion::new(8, 0, 2); let requires = parse_requires("version >= 7.0.3 < 8 | >= 8.0.3").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_err()); + assert!(check_requires(&requires, &suricata_version, false).is_err()); let suricata_version = SuricataVersion::new(7, 0, 2); let requires = parse_requires("version >= 7.0.3 < 8 | >= 8.0.3").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_err()); + assert!(check_requires(&requires, &suricata_version, false).is_err()); let suricata_version = SuricataVersion::new(7, 0, 3); let requires = parse_requires("version >= 7.0.3 < 8 | >= 8.0.3").unwrap(); - assert!(check_requires(&requires, &suricata_version).is_ok()); + assert!(check_requires(&requires, &suricata_version, false).is_ok()); // Example of something that requires a fix/feature that was // implemented in 7.0.5, 8.0.4, 9.0.3. let requires = parse_requires("version >= 7.0.5 < 8 | >= 8.0.4 < 9 | >= 9.0.3").unwrap(); - assert!(check_requires(&requires, &SuricataVersion::new(6, 0, 0)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 4)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 5)).is_ok()); - assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 3)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 4)).is_ok()); - assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 2)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 3)).is_ok()); - assert!(check_requires(&requires, &SuricataVersion::new(10, 0, 0)).is_ok()); + assert!(check_requires(&requires, &SuricataVersion::new(6, 0, 0), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 4), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 5), false).is_ok()); + assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 3), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 4), false).is_ok()); + assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 2), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 3), false).is_ok()); + assert!(check_requires(&requires, &SuricataVersion::new(10, 0, 0), false).is_ok()); let requires = parse_requires("version >= 8 < 9").unwrap(); - assert!(check_requires(&requires, &SuricataVersion::new(6, 0, 0)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 0)).is_err()); - assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0)).is_ok()); - assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 0)).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(6, 0, 0), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(7, 0, 0), false).is_err()); + assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0), false).is_ok()); + assert!(check_requires(&requires, &SuricataVersion::new(9, 0, 0), false).is_err()); // Unknown keyword. let requires = parse_requires("feature true_lua, foo bar, version >= 7.0.3").unwrap(); @@ -786,7 +789,7 @@ mod test { // 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()); + assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0), false).is_err()); } #[test] @@ -830,6 +833,6 @@ 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()); + assert!(check_requires(&requires, &SuricataVersion::new(8, 0, 0), false).is_err()); } } diff --git a/src/detect-requires.c b/src/detect-requires.c index 4d7f916b3b82..bb478699762c 100644 --- a/src/detect-requires.c +++ b/src/detect-requires.c @@ -20,6 +20,10 @@ #include "detect-engine.h" #include "rust.h" +/* Set to true if unknown requirements should be ingored. In Suricata + * 8, unknown requirements are treated as unsatisfied requirements. */ +static int g_ignore_unknown_requirements = 0; + static int DetectRequiresSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr) { if (de_ctx->requirements == NULL) { @@ -28,7 +32,8 @@ static int DetectRequiresSetup(DetectEngineCtx *de_ctx, Signature *s, const char } const char *errmsg = NULL; - int res = SCDetectCheckRequires(rawstr, PROG_VER, &errmsg, de_ctx->requirements); + int res = SCDetectCheckRequires( + rawstr, PROG_VER, &errmsg, de_ctx->requirements, g_ignore_unknown_requirements); if (res == -1) { // The requires expression is bad, log an error. SCLogError("%s: %s", errmsg, rawstr); @@ -43,6 +48,8 @@ static int DetectRequiresSetup(DetectEngineCtx *de_ctx, Signature *s, const char void DetectRequiresRegister(void) { + ConfGetBool("ignore-unknown-requirements", &g_ignore_unknown_requirements); + sigmatch_table[DETECT_REQUIRES].name = "requires"; sigmatch_table[DETECT_REQUIRES].desc = "require Suricata version or features"; sigmatch_table[DETECT_REQUIRES].url = "/rules/meta-keywords.html#requires";