Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

7.0.x: backport treating unknown requirements as unsatisfied with opt-out flag - v2 #12226

Closed
wants to merge 2 commits into from
Closed

7.0.x: backport treating unknown requirements as unsatisfied with opt-out flag - v2 #12226

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bdda0c
requires: treat unknown requires keywords as unmet requirements
jasonish Nov 20, 2024
7b63231
requires: add option to ignore unknown requirements
jasonish Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions doc/userguide/rules/meta.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
--------

Expand All @@ -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
Expand Down
21 changes: 21 additions & 0 deletions doc/userguide/upgrade.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,27 @@ 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.8
------------------
- Unknown requirements in the ``requires`` keyword will now be treated
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you thinking about making this a 7.0.x only option? If so, we should probably state that this is not behavior that will continue in 8

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to keep this in 8 and beyond, I'd like it to live under detect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we wouldn't have it at all. So will add a note that it won't be around in 8.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to keep this in 8 and beyond, I'd like it to live under detect.

Also note the reasoning for the location: #12226 (comment)


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

Expand Down
93 changes: 63 additions & 30 deletions rust/src/detect/requires.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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));
}
}

Expand Down Expand Up @@ -289,8 +297,14 @@ fn check_version(
}

fn check_requires(
requires: &Requires, suricata_version: &SuricataVersion,
requires: &Requires, suricata_version: &SuricataVersion, ignore_unknown: bool,
) -> Result<(), RequiresError> {
if !ignore_unknown && !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;
Expand Down Expand Up @@ -439,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)) {
Expand All @@ -462,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 {
Expand All @@ -483,6 +499,7 @@ pub unsafe extern "C" fn SCDetectCheckRequires(
RequiresError::VersionGt => {
status.gt_count += 1;
}
RequiresError::UnknownRequirement(_) => {}
_ => {}
}
*errstr = err.c_errmsg();
Expand Down Expand Up @@ -594,6 +611,7 @@ mod test {
patch: 0,
}
}]],
unknown: vec![],
}
);

Expand All @@ -610,6 +628,7 @@ mod test {
patch: 0,
}
}]],
unknown: vec![],
}
);

Expand All @@ -626,6 +645,7 @@ mod test {
patch: 2,
}
}]],
unknown: vec![],
}
);

Expand All @@ -652,6 +672,7 @@ mod test {
}
}
]],
unknown: vec![],
}
);
}
Expand All @@ -662,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,
Expand All @@ -673,86 +694,86 @@ 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 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 {
Expand All @@ -761,8 +782,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), false).is_err());
}

#[test]
Expand Down Expand Up @@ -802,4 +829,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), false).is_err());
}
}
9 changes: 8 additions & 1 deletion src/detect-requires.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I think this answers my question. So do we need a warning/doc reference about this being 7 only behavior?

static int g_ignore_unknown_requirements = 0;

static int DetectRequiresSetup(DetectEngineCtx *de_ctx, Signature *s, const char *rawstr)
{
if (de_ctx->requirements == NULL) {
Expand All @@ -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);
Expand All @@ -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";
Expand Down
Loading