diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9699d8e..ed1c28c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,4 +80,11 @@ jobs: restore-keys: ${{ runner.os }}-cargo-test- - name: Install protoc run: sudo apt-get update && sudo apt-get install -y protobuf-compiler + # The CRS integration tests fail loudly without this checkout rather than + # skipping, so that they cannot silently stop running again. + - name: Fetch OWASP CRS + run: | + mkdir -p test-rules + git clone --depth 1 https://github.com/coreruleset/coreruleset.git test-rules/crs + cp test-rules/crs/crs-setup.conf.example test-rules/crs/crs-setup.conf - run: cargo test --workspace diff --git a/src/lib.rs b/src/lib.rs index 811db9b..be5581a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -142,13 +142,25 @@ pub struct ZentinelSecEngine { impl ZentinelSecEngine { /// Create a new ZentinelSec engine with the given configuration pub fn new(config: ZentinelSecConfig) -> Result { - // Build rules string from all rule files + // Hand the engine `Include` directives rather than the text of each + // rule file. + // + // Splicing file contents together loses the one thing the engine needs + // to resolve `@pmFromFile`/`@ipMatchFromFile` data paths and nested + // `Include`s: which file a directive came from. Those paths are + // relative to their own rule file, so a concatenated string sent them + // to the process's working directory instead. Stock CRS references + // `scanners-user-agents.data` and many others, which meant the + // documented deployment only started if it happened to be run from + // /etc/modsecurity/crs/rules. let mut rules_content = String::new(); // Always enable the rule engine rules_content.push_str("SecRuleEngine On\n"); - // Load rules from configured paths + // Resolve the configured patterns ourselves so a path that cannot be + // read is reported against the pattern that produced it, and so the + // file count is available for logging. let mut loaded_count = 0; for path_pattern in &config.rules_paths { // Handle glob patterns @@ -159,13 +171,16 @@ impl ZentinelSecEngine { match entry { Ok(path) => { if path.is_file() { - let content = fs::read_to_string(&path).map_err(|e| { - anyhow::anyhow!("Failed to read rule file {:?}: {}", path, e) + // An absolute path keeps the include independent of + // the working directory; quoting keeps a path with + // spaces in one argument. + let absolute = fs::canonicalize(&path).map_err(|e| { + anyhow::anyhow!("Failed to resolve rule file {:?}: {}", path, e) })?; - rules_content.push_str(&content); - rules_content.push('\n'); + rules_content + .push_str(&format!("Include \"{}\"\n", absolute.display())); loaded_count += 1; - debug!(path = ?path, "Loaded rule file"); + debug!(path = ?absolute, "Including rule file"); } } Err(e) => { @@ -176,7 +191,7 @@ impl ZentinelSecEngine { } // Create the ModSecurity engine - let modsec = if rules_content.trim().is_empty() || loaded_count == 0 { + let modsec = if loaded_count == 0 { // No rules loaded, create with just SecRuleEngine On ModSecurity::from_string("SecRuleEngine On") .map_err(|e| anyhow::anyhow!("Failed to initialize ZentinelSec engine: {}", e))? @@ -194,6 +209,20 @@ impl ZentinelSecEngine { Ok(Self { modsec, config }) } + /// Number of rules the engine loaded. + pub fn rule_count(&self) -> usize { + self.modsec.rule_count() + } + + /// The underlying ModSecurity engine. + /// + /// Exposed so tests can drive transactions through the rules this agent + /// actually loaded, rather than through a separately built engine that + /// might load them differently. + pub fn modsec(&self) -> &ModSecurity { + &self.modsec + } + /// Check if path should be excluded pub fn is_excluded(&self, path: &str) -> bool { self.config @@ -313,12 +342,17 @@ impl ZentinelSecAgent { status = status, "ZentinelSec intervention (headers)" ); - let rule_ids = tx.matched_rules().iter().map(|s| s.to_string()).collect(); - return Ok(Some(( - status, - "Blocked by ZentinelSec".to_string(), - rule_ids, - ))); + // The rules that caused the intervention, not every rule that + // matched: `matched_rules()` also lists CRS setup actions and + // chain starters whose chain never completed, so its first + // entry under stock CRS is a setup rule such as 900990 rather + // than the rule that blocked. + let rule_ids = intervention.rule_ids.clone(); + let message = intervention + .log + .clone() + .unwrap_or_else(|| "Blocked by ZentinelSec".to_string()); + return Ok(Some((status, message, rule_ids))); } } @@ -339,12 +373,12 @@ impl ZentinelSecAgent { status = status, "ZentinelSec intervention (body)" ); - let rule_ids = tx.matched_rules().iter().map(|s| s.to_string()).collect(); - return Ok(Some(( - status, - "Blocked by ZentinelSec".to_string(), - rule_ids, - ))); + let rule_ids = intervention.rule_ids.clone(); + let message = intervention + .log + .clone() + .unwrap_or_else(|| "Blocked by ZentinelSec".to_string()); + return Ok(Some((status, message, rule_ids))); } } } diff --git a/tests/crs_integration.rs b/tests/crs_integration.rs index 1827a1e..635856b 100644 --- a/tests/crs_integration.rs +++ b/tests/crs_integration.rs @@ -1,336 +1,300 @@ -//! Integration tests with real OWASP CRS rules. +//! Integration tests against a real OWASP CRS checkout. //! -//! These tests require the CRS rules to be downloaded: -//! ``` -//! mkdir -p test-rules && cd test-rules -//! git clone --depth 1 https://github.com/coreruleset/coreruleset.git crs -//! cp crs/crs-setup.conf.example crs/crs-setup.conf -//! ``` - -use std::path::Path; - -fn crs_available() -> bool { - Path::new("test-rules/crs/crs-setup.conf").exists() -} - -fn load_crs() -> zentinel_modsec::ModSecurity { - // Load CRS setup first, then specific rule files - let mut rules_content = String::new(); - - // Enable SecRuleEngine - rules_content.push_str("SecRuleEngine On\n"); - rules_content.push_str("SecRequestBodyAccess On\n"); - - // Load crs-setup.conf - let setup = std::fs::read_to_string("test-rules/crs/crs-setup.conf") - .expect("Failed to read crs-setup.conf"); - rules_content.push_str(&setup); - rules_content.push('\n'); - - // Load specific rule files for testing (not all, to keep tests fast) - let rule_files = [ - "test-rules/crs/rules/REQUEST-901-INITIALIZATION.conf", - "test-rules/crs/rules/REQUEST-941-APPLICATION-ATTACK-XSS.conf", - "test-rules/crs/rules/REQUEST-942-APPLICATION-ATTACK-SQLI.conf", - "test-rules/crs/rules/REQUEST-930-APPLICATION-ATTACK-LFI.conf", - "test-rules/crs/rules/REQUEST-932-APPLICATION-ATTACK-RCE.conf", - ]; - - for path in &rule_files { - if Path::new(path).exists() { - let content = - std::fs::read_to_string(path).unwrap_or_else(|_| panic!("Failed to read {}", path)); - rules_content.push_str(&content); - rules_content.push('\n'); - } +//! These load CRS through `ZentinelSecEngine` itself — the same code path the +//! agent uses — rather than building an engine separately, because the way +//! rules are loaded is itself a thing that has broken: splicing rule files +//! together lost the base path CRS needs to resolve its `.data` files, so the +//! documented deployment only started when run from the CRS rules directory. +//! +//! Two properties of this file are deliberate: +//! +//! * **A missing fixture is a failure, not a skip.** These tests previously +//! returned early when the checkout was absent, and CI never fetched it, so +//! they reported green without ever running. Set `ZENTINELSEC_SKIP_CRS_TESTS` +//! to opt out locally; CI must not. +//! * **Assertions name the rules that must fire.** The previous form was +//! `assert!(blocked || !rule_ids.is_empty())`, and `rule_ids` came from +//! `matched_rules()`, which under stock CRS always contains the 901xxx setup +//! rules — so it could never fail, whatever the engine did. + +use std::path::{Path, PathBuf}; +use zentinel_agent_zentinelsec::{ZentinelSecConfig, ZentinelSecEngine}; + +/// Where the CRS checkout lives. Override with `ZENTINELSEC_CRS_DIR`. +fn crs_dir() -> Option { + if std::env::var_os("ZENTINELSEC_SKIP_CRS_TESTS").is_some() { + return None; } + let dir = std::env::var("ZENTINELSEC_CRS_DIR") + .map(PathBuf::from) + .unwrap_or_else(|_| PathBuf::from("test-rules/crs")); - zentinel_modsec::ModSecurity::from_string(&rules_content).expect("Failed to parse CRS rules") + assert!( + dir.join("crs-setup.conf").is_file(), + "OWASP CRS checkout not found at {}.\n\ + Fetch it with:\n\ + mkdir -p test-rules && \\\n\ + git clone --depth 1 https://github.com/coreruleset/coreruleset.git test-rules/crs && \\\n\ + cp test-rules/crs/crs-setup.conf.example test-rules/crs/crs-setup.conf\n\ + Set ZENTINELSEC_CRS_DIR to use another location, or \ + ZENTINELSEC_SKIP_CRS_TESTS=1 to skip these tests locally. \ + CI must not set either.", + dir.display() + ); + Some(dir) } -#[test] -fn test_crs_loads_successfully() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded. Run:"); - eprintln!(" cd test-rules && git clone --depth 1 https://github.com/coreruleset/coreruleset.git crs"); - return; - } - - let modsec = load_crs(); - let rule_count = modsec.rule_count(); - println!("Loaded {} CRS rules", rule_count); - assert!(rule_count > 0, "Expected at least some rules to load"); +/// Load the whole rule set the way the README tells operators to. +fn engine(dir: &Path) -> ZentinelSecEngine { + let config = ZentinelSecConfig { + rules_paths: vec![ + dir.join("crs-setup.conf").display().to_string(), + dir.join("rules/*.conf").display().to_string(), + ], + ..Default::default() + }; + ZentinelSecEngine::new(config).expect("CRS should load through the agent's own loader") } -#[test] -fn test_crs_sql_injection_942100() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; +/// Rule IDs reported for a request, and whether it was interrupted. +fn probe( + e: &ZentinelSecEngine, + method: &str, + uri: &str, + headers: &[(&str, &str)], + body: &[u8], +) -> (bool, Vec) { + let mut tx = e.modsec().new_transaction(); + tx.process_uri(uri, method, "HTTP/1.1").unwrap(); + tx.add_request_header("Host", "example.com").unwrap(); + tx.add_request_header( + "User-Agent", + "Mozilla/5.0 (X11; Linux x86_64) Firefox/128.0", + ) + .unwrap(); + tx.add_request_header("Accept", "text/html,application/xhtml+xml") + .unwrap(); + tx.add_request_header("Accept-Language", "en-US,en;q=0.9") + .unwrap(); + tx.add_request_header("Accept-Encoding", "gzip, deflate") + .unwrap(); + tx.add_request_header("Connection", "keep-alive").unwrap(); + for (k, v) in headers { + tx.add_request_header(k, v).unwrap(); } - - let modsec = load_crs(); - - // Test classic SQL injection patterns - let sqli_payloads = [ - "/api/users?id=1' OR '1'='1", - "/api/users?id=1; DROP TABLE users--", - "/api/users?id=1 UNION SELECT * FROM passwords--", - "/search?q=' OR 1=1--", - "/login?user=admin'--", - ]; - - println!("\n=== CRS SQL Injection Detection ==="); - for payload in &sqli_payloads { - let mut tx = modsec.new_transaction(); - tx.process_uri(payload, "GET", "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.process_request_headers().unwrap(); - - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); - - println!( - " {} => {} {:?}", - payload, - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids - ); - - assert!( - blocked || !rule_ids.is_empty(), - "Expected SQLi to be detected: {}", - payload - ); + if !body.is_empty() { + tx.add_request_header("Content-Length", &body.len().to_string()) + .unwrap(); } -} - -#[test] -fn test_crs_xss_941100() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; + tx.process_request_headers().unwrap(); + if !body.is_empty() { + tx.append_request_body(body).unwrap(); } + tx.process_request_body().unwrap(); - let modsec = load_crs(); - - // Test XSS patterns - let xss_payloads = [ - "/search?q=", - "/search?q=", - "/search?q=javascript:alert(document.cookie)", - "/page?content=", - "/comment?text=", - ]; - - println!("\n=== CRS XSS Detection ==="); - for payload in &xss_payloads { - let mut tx = modsec.new_transaction(); - tx.process_uri(payload, "GET", "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.process_request_headers().unwrap(); - - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); + let ids = tx + .intervention() + .map(|i| i.rule_ids.clone()) + .unwrap_or_default(); + let blocked = tx + .intervention() + .map(|i| i.status != 0 && i.status != 200) + .unwrap_or(false); + (blocked, ids) +} - println!( - " {} => {} {:?}", - payload, - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids - ); +// --------------------------------------------------------------------------- +// Loading +// --------------------------------------------------------------------------- - assert!( - blocked || !rule_ids.is_empty(), - "Expected XSS to be detected: {}", - payload - ); - } +#[test] +fn crs_loads_through_the_agent_loader() { + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + assert!( + e.rule_count() > 500, + "expected the full CRS rule set, loaded {}", + e.rule_count() + ); } #[test] -fn test_crs_path_traversal_930100() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; - } +fn crs_loads_regardless_of_working_directory() { + // The regression this guards: CRS `.data` files are referenced relative to + // the rule file that names them, so a loader that concatenates file + // contents resolves them against the process's working directory instead + // and fails on `scanners-user-agents.data`. + // + // The working directory during `cargo test` is the crate root, which is + // not the CRS rules directory, so loading by absolute path is already the + // case that used to fail. Deliberately no `chdir` here: tests share a + // process, and changing the working directory under them is a race. + let Some(dir) = crs_dir() else { return }; + let dir = std::fs::canonicalize(&dir).expect("CRS dir"); + let e = engine(&dir); + assert!(e.rule_count() > 500); +} - let modsec = load_crs(); +// --------------------------------------------------------------------------- +// Detection — each asserts the rule that must fire +// --------------------------------------------------------------------------- - // Test path traversal patterns - let lfi_payloads = [ - "/download?file=../../../etc/passwd", - "/read?path=....//....//etc/shadow", - "/view?doc=/etc/passwd", - "/include?page=..\\..\\..\\windows\\system32\\config\\sam", +#[test] +fn crs_detects_attacks() { + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + + let cases: &[(&str, &str, &str, &[u8])] = &[ + ( + "SQLi in query", + "GET", + "/u?id=1'+UNION+SELECT+password+FROM+users--+", + b"", + ), + ( + "SQLi in body", + "POST", + "/u", + b"q=1' UNION SELECT password FROM users-- ", + ), + ( + "XSS in query", + "GET", + "/s?q=%3Cscript%3Ealert(1)%3C/script%3E", + b"", + ), + ("LFI in query", "GET", "/f?p=../../../../etc/passwd", b""), + ( + "RCE in body", + "POST", + "/r", + b"cmd=/bin/bash -c \"cat /etc/passwd\"", + ), ]; - println!("\n=== CRS Path Traversal Detection ==="); - for payload in &lfi_payloads { - let mut tx = modsec.new_transaction(); - tx.process_uri(payload, "GET", "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.process_request_headers().unwrap(); - - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); - - println!( - " {} => {} {:?}", - payload, - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids - ); - - assert!( - blocked || !rule_ids.is_empty(), - "Expected LFI to be detected: {}", - payload - ); + for (name, method, uri, body) in cases { + let headers: &[(&str, &str)] = if body.is_empty() { + &[] + } else { + &[("Content-Type", "application/x-www-form-urlencoded")] + }; + let (blocked, ids) = probe(&e, method, uri, headers, body); + assert!(blocked, "{name}: expected CRS to block, rule_ids={ids:?}"); } } #[test] -fn test_crs_command_injection_932100() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; - } - - let modsec = load_crs(); - - // Test command injection patterns - let rce_payloads = [ - "/ping?host=127.0.0.1;cat /etc/passwd", - "/exec?cmd=|ls -la", - "/run?command=`id`", - "/process?input=$(whoami)", +fn crs_detects_attacks_in_xml_bodies() { + // XML is flattened into ARGS, which is what stock CRS rules inspect. + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + let xml: &[(&str, &[u8])] = &[ + ("element text", br#"1' UNION SELECT password FROM users-- "#), + ("attribute", br#""#), + ("soap envelope", br#"1' UNION SELECT password FROM users-- "#), ]; - - println!("\n=== CRS Command Injection Detection ==="); - for payload in &rce_payloads { - let mut tx = modsec.new_transaction(); - tx.process_uri(payload, "GET", "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.process_request_headers().unwrap(); - - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); - - println!( - " {} => {} {:?}", - payload, - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids + for (name, body) in xml { + let (blocked, ids) = probe( + &e, + "POST", + "/u", + &[("Content-Type", "application/xml")], + body, ); - assert!( - blocked || !rule_ids.is_empty(), - "Expected RCE to be detected: {}", - payload + blocked, + "SQLi in XML {name}: expected a block, rule_ids={ids:?}" ); } } -#[test] -fn test_crs_clean_requests_pass() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; - } - - let modsec = load_crs(); +// --------------------------------------------------------------------------- +// False positives — the half that was never tested +// --------------------------------------------------------------------------- - // Test legitimate requests - let clean_requests = [ - ("/", "GET"), - ("/api/users", "GET"), - ("/api/users/123", "GET"), - ("/search?q=hello+world", "GET"), - ("/products?category=electronics&page=1", "GET"), - ("/login", "POST"), +#[test] +#[ignore = "fails against zentinel-modsec 0.3.0: CRS 920100 denies every request because \ +REQUEST_LINE is unimplemented and its negated regex inverts. Fixed by \ +zentinelproxy/zentinel-modsec#30; remove this attribute when the dependency is bumped past it. \ +See zentinelproxy/zentinel-modsec#29."] +fn crs_does_not_block_ordinary_traffic() { + // This is the test whose absence let a 100%-block-rate defect ship: with + // stock CRS every one of these was denied by 920100, including `GET /`. + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + + let cases: &[(&str, &str, &str, &[u8])] = &[ + ("root", "GET", "/", b""), + ("static page", "GET", "/index.html", b""), + ("health check", "GET", "/api/v1/health", b""), + ("stylesheet", "GET", "/static/app.css", b""), + ("pdf download", "GET", "/docs/report.pdf", b""), + ( + "ordinary query", + "GET", + "/search?q=blue+widgets&page=2", + b"", + ), + ("form post", "POST", "/api/orders", b"item=widget&qty=3"), ]; - println!("\n=== CRS Clean Requests ==="); - for (path, method) in &clean_requests { - let mut tx = modsec.new_transaction(); - tx.process_uri(path, method, "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.add_request_header("User-Agent", "Mozilla/5.0").unwrap(); - tx.process_request_headers().unwrap(); - - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); - - println!( - " {} {} => {} {:?}", - method, - path, - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids - ); - - // Clean requests should not be blocked + for (name, method, uri, body) in cases { + let headers: &[(&str, &str)] = if body.is_empty() { + &[] + } else { + &[("Content-Type", "application/x-www-form-urlencoded")] + }; + let (blocked, ids) = probe(&e, method, uri, headers, body); assert!( !blocked, - "Clean request should not be blocked: {} {}", - method, path + "{name}: ordinary traffic must not be blocked, but rule(s) {ids:?} fired" ); } } #[test] -fn test_crs_post_body_sqli() { - if !crs_available() { - eprintln!("Skipping CRS test - rules not downloaded"); - return; - } - - let modsec = load_crs(); - - println!("\n=== CRS POST Body SQL Injection ==="); - - let mut tx = modsec.new_transaction(); - tx.process_uri("/api/login", "POST", "HTTP/1.1").unwrap(); - tx.add_request_header("Host", "example.com").unwrap(); - tx.add_request_header("Content-Type", "application/x-www-form-urlencoded") - .unwrap(); - tx.process_request_headers().unwrap(); - - // Add SQL injection in POST body - let body = b"username=admin&password=' OR '1'='1' --"; - tx.append_request_body(body).unwrap(); - tx.process_request_body().unwrap(); +#[ignore = "fails against zentinel-modsec 0.3.0: CRS 920100 denies every request because \ +REQUEST_LINE is unimplemented and its negated regex inverts. Fixed by \ +zentinelproxy/zentinel-modsec#30; remove this attribute when the dependency is bumped past it. \ +See zentinelproxy/zentinel-modsec#29."] +fn crs_does_not_block_an_ordinary_xml_post() { + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + let (blocked, ids) = probe( + &e, + "POST", + "/api/orders", + &[("Content-Type", "application/xml")], + br#"widget3"#, + ); + assert!( + !blocked, + "benign XML must not be blocked, rule(s) {ids:?} fired" + ); +} - let blocked = tx - .intervention() - .map(|i| i.status != 0 && i.status != 200) - .unwrap_or(false); - let rule_ids: Vec<_> = tx.matched_rules().to_vec(); +// --------------------------------------------------------------------------- +// Reporting +// --------------------------------------------------------------------------- - println!( - " POST body SQLi => {} {:?}", - if blocked { "BLOCKED" } else { "allowed" }, - rule_ids +#[test] +fn a_block_names_the_rule_that_caused_it() { + // `X-WAF-Rule` is built from the first reported id. Taking it from + // `matched_rules()` produced a CRS setup rule such as 900990 rather than + // the rule that blocked, which is useless to whoever is debugging it. + let Some(dir) = crs_dir() else { return }; + let e = engine(&dir); + let (blocked, ids) = probe( + &e, + "GET", + "/u?id=1'+UNION+SELECT+password+FROM+users--+", + &[], + b"", ); - + assert!(blocked); + let first = ids.first().map(String::as_str).unwrap_or(""); assert!( - blocked || !rule_ids.is_empty(), - "Expected POST body SQLi to be detected" + !first.starts_with("900") && !first.starts_with("901"), + "the reported rule should be the one that blocked, got {ids:?}" ); }