Stock OWASP CRS does not evaluate correctly: skipAfter, TX key case, block, and unimplemented variables

[raffaelschneider]

Problem Statement

Four engine defects make stock OWASP CRS evaluate incorrectly. All four were confirmed by probe against main @ 3905ee1 with CRS main (4.30.0-dev), not by reading the code.

They mask each other, so they should be fixed and tested together. Fixing any one alone will look like a regression:

• (3) makes every matching rule block, so traffic is stopped — which hides (1) and (2).
• (2) stops CRS's paranoia gates from ever firing, which hides (1).
• Fix (3) alone and the WAF stops blocking anything at all, because (1) and (2) mean the anomaly score never reaches 949110.

Ordered by impact.

---

1. skipAfter in phase ≥ 2 discards the rest of the phase

SecMarker is registered only under Phase::RequestHeaders, regardless of the phase of the rules around it:

// src/engine/ruleset.rs:290
Directive::SecMarker(marker) => {
    // Add marker at current position in default phase
    let phase = Phase::RequestHeaders;

but the runtime only resumes when the marker's phase matches the phase being evaluated:

// src/engine/transaction.rs:427
if marker_phase == phase && marker_idx > idx {
    idx = marker_idx;
    skip_after = None;
    continue;
}
// Marker not found or in different phase, continue
idx += 1;
continue;

For a phase-2 skipAfter the marker is filed under phase 1, the branch is never taken, and the loop walks to the end of the phase skipping every remaining rule.

This is the highest-impact of the four: CRS gates every rule file with a phase-2 paranoia check (id:942012,phase:2,...,skipAfter:END-REQUEST-942-APPLICATION-ATTACK-SQLI), and 949110 — the rule that actually blocks on anomaly score — sits after all of them. So anomaly-score blocking can never run.

// phase 1: correct -> ["1", "3"]
SecAction "id:1,phase:1,pass,nolog,skipAfter:M1"
SecRule REQUEST_METHOD "@streq POST" "id:2,phase:1,pass,nolog"
SecMarker M1
SecRule REQUEST_METHOD "@streq POST" "id:3,phase:1,pass,nolog"

// phase 2: wrong -> ["11"], expected ["11", "13"]
SecAction "id:11,phase:2,pass,nolog,skipAfter:M2"
SecRule ARGS "@rx hello" "id:12,phase:2,pass,nolog"
SecMarker M2
SecRule ARGS "@rx hello" "id:13,phase:2,pass,nolog"

A marker needs to be recorded per phase — at the current index of each phase it separates — rather than at a single index in phase 1.

---

2. TX collection keys are case-sensitive

ModSecurity treats collection keys case-insensitively. CRS relies on this: it writes lowercase and reads uppercase.

SecAction "id:1,phase:2,pass,nolog,setvar:'tx.blocking_paranoia_level=1'"

SecRule TX:blocking_paranoia_level "@ge 1" "id:2,phase:2,deny"  // fires
SecRule TX:BLOCKING_PARANOIA_LEVEL "@ge 1" "id:3,phase:2,deny"  // does NOT fire

Macro expansion has the same problem — %{TX.CRITICAL_ANOMALY_SCORE} does not resolve a value set as tx.critical_anomaly_score.

Consequence: 949060 (SecRule TX:BLOCKING_PARANOIA_LEVEL "@ge 1" ... setvar:'tx.blocking_inbound_anomaly_score=+%{tx.inbound_anomaly_score_pl1}') never runs, so per-rule anomaly scores are never summed and 949110 has nothing to compare. Verified: after 15 CRS attack rules match a SQLi request, tx.blocking_inbound_anomaly_score is still unset.

setvar increment semantics themselves are correct — =+, macro increments, and increment-from-unset all behave — this is purely key lookup.

---

3. block is an immediate deny instead of inheriting SecDefaultAction

In ModSecurity, block means "use the disruptive action from SecDefaultAction". CRS sets:

SecDefaultAction "phase:1,log,auditlog,pass"
SecDefaultAction "phase:2,log,auditlog,pass"

so in anomaly-scoring mode block must resolve to pass, leaving the decision to 949110. Here it is its own disruptive action:

// src/actions/mod.rs:171
DisruptiveAction::Block => DisruptiveOutcome::Block,

and merge_default_actions (src/parser/mod.rs:468) drops the default pass because both are Action::Disruptive, so the rule's block wins.

SecDefaultAction "phase:2,log,auditlog,pass"
SecRule REQUEST_METHOD "@streq POST" "id:3,phase:2,block"   // intervenes; should not

Effect: CRS runs as block-on-first-match rather than anomaly scoring — including on protocol-enforcement false positives, and with no threshold, no paranoia levels and no scoring.

Note SecDefaultAction is also stored as a single flat Vec<Action> (src/parser/mod.rs:46) with no phase dimension, though CRS sets one per phase. Secondary, but the same fix area.

---

4. Unimplemented variables resolve to empty, and negation inverts that into "always match"

resolver.rs:249 ends the match with _ => vec![], and an empty variable list under a negated operator is reported as a match:

// src/engine/transaction.rs:707
// Variables were specified but resolved to nothing (e.g. absent header).
return Ok(RuleOutcome {
    matched: rule.operator_negated,

REQUEST_LINE is one of the unimplemented ones, and CRS 920100 is a negated regex over it — so it matches every request:

SecRuleEngine On
SecRule REQUEST_LINE "!@rx (?i)^(?:connect ...|[a-z]{3,10}[\s\x0b]+...)$" "id:920100,phase:2,deny"
// benign POST /api HTTP/1.1 -> BLOCKED

With stock CRS loaded, 920100 denies 100% of traffic. REQUEST_LINE "@rx ." matches nothing, confirming the variable is simply absent.

Variables CRS references that are accepted by the parser but unimplemented in the resolver:

Variable	CRS rules	Notes
REQUEST_LINE	7	incl. 920100, negated → always fires
REQUEST_BASENAME	5	incl. one negated !@endsWith .pdf → always fires
XML	176	see zentinelproxy/zentinel#441
ARGS_COMBINED_SIZE	1
FILES_COMBINED_SIZE	1
UNIQUE_ID	1

Two things worth separating here: implementing the missing variables, and deciding what an unimplemented-but-parsed variable should do. The precedent from #25 (report unusable @rx patterns rather than match nothing) argues for a load-time diagnostic rather than a silent empty resolve — and note #25's own trap applies again, that preserving negation over a never-matching variable turns the rule into one that matches everything.

---

Notes for whoever verifies this

Three things cost me wrong readings before I isolated them:

• matched_rules() records a chained rule's ID when only its first leg matched, even though the rule did not fire. It is not a list of rules that fired.
• has_intervention() is transaction-wide, so appending a probe rule and reading it tells you nothing if an earlier rule already intervened — under stock CRS, 920100 intervenes on everything.
• ModSecurity::from_string does not resolve relative Include paths; a CRS config loaded that way silently has no rules in it. Use from_file with absolute includes.

Chaining, & count, and setvar increments were all checked and are correct — they are not part of this.

[raffaelschneider]

Follow-up on scope: all four reproduce against the published 0.3.0 on crates.io, not just main. I had originally verified against main @ 3905ee1; re-running the same probes as a downstream consumer of zentinel-modsec = "=0.3.0" gives identical results for the phase-2 skipAfter, the TX key case-sensitivity, block-as-deny, and REQUEST_LINE. So this is shipped behaviour.

That matters because zentinel-agent-zentinelsec — the consumer — depends on zentinel-modsec = "0.3.0" and does not bundle CRS; its README documents pointing --rules at a stock CRS checkout including rules/*.conf, which pulls in REQUEST-920-PROTOCOL-ENFORCEMENT.conf.

Replicating that agent's exact loader over 703 stock CRS rules, with its default --block-mode true:

GET   /                      blocked=true   ["920100"]
GET   /index.html            blocked=true   ["920100"]
GET   /api/v1/health         blocked=true   ["920100"]
POST  /api/v1/orders         blocked=true   ["920100"]
GET   /static/app.css        blocked=true   ["920100"]

Every request denied, by defect 4 alone.

Two notes for sequencing the fix:

• The consumer's tests currently pass partly because of defect 3. With block acting as a deny, a "was this blocked?" assertion passes for the wrong reason. Fix 3 on its own and CRS stops blocking altogether, because 1 and 2 mean the anomaly score never reaches 949110. This is the concrete case for landing all four together rather than as separate PRs.
• A regression test should assert which rule IDs fired, not merely whether the request was blocked. Under the current engine everything blocks, so the weaker assertion is satisfied by the bug. The natural fixture is stock CRS loaded the way a deployment loads it — crs-setup.conf plus rules/*.conf, not a hand-picked subset — with one attack payload and one benign request, asserting the expected IDs on the former and none on the latter.

Consumer-side issues (its rule loading defeats CRS data-path resolution, and its CRS integration tests have never run in CI) are filed separately as zentinelproxy/zentinel-agent-zentinelsec#7.

[raffaelschneider]

Landing order

Everything for this issue is now in review. The order below is not arbitrary — the dependencies are real, and two of them will look like regressions if taken out of sequence.

#	PR	Base	Why here
1	#36 CRS conformance CI	main	It is the instrument the rest are measured with. Landing it first means every later PR shows its effect in CI rather than in a claim.
2	#30 the four defects	main	The only one that changes what a deployment does. Raises MIN_PASSING 4080 → 4103 and un-ignores stock_crs_blocks_attacks_and_passes_ordinary_traffic.
3	#35 + decoding	main	Independent; +125 corpus cases.
4	#33 t:cmdLine	main	Independent, but worth ~+67 rather than its solo +10 once #35 is in — they are coupled.
5	#32 XML: targets	#30	Stacked. Must not land before #30: a never-matching target with negation preserved inflates !@rx into match-everything, which is the 920100 bug again.
6	#37 measured README	#36	Cites the conformance job.

MIN_PASSING should be raised in whichever PR moves it, so a gain cannot be lost silently. Expected end state: 4272+ of 5033, and the blocking-behaviour test passing.

Then the release — not started, and not something I should start

zentinel-modsec releases are manual: tag, hand-written GitHub Release, separate cargo publish. There is no release workflow and Cargo.lock is gitignored.

Nothing above reaches a user until that happens. Concretely, today:

• zentinel-agent-zentinelsec depends on zentinel-modsec = "0.3.0" from crates.io.
• 0.3.0 contains all four defects, so its documented deployment denies every request.
• Merging these PRs changes nothing for that agent until a new version is published.

Suggested version: 0.4.0 rather than 0.3.1. Three of the changes alter behaviour rather than fixing it invisibly — block now inherits SecDefaultAction, a negated operator no longer matches an absent variable, and + now decodes to a space. Anyone with rules tuned against the old semantics should see a minor bump.

Worth putting in the release notes explicitly:

A stock OWASP CRS deployment on 0.3.0 denies every request, including GET /.
If you are running 0.3.0 in blocking mode with stock CRS, this is the upgrade
that fixes it. If you cannot upgrade yet, run detection-only.

After publishing

zentinel-agent-zentinelsec#8 bumps the dependency and removes the #[ignore] from its two false-positive tests — they fail today only because of 0.3.0's 920100 behaviour, and are the acceptance criteria for the bump. That PR should be rebased onto the published version, not merged before it exists.

---

I have not tagged, published, or cut anything, and I am not going to without an explicit go-ahead — the version number, the timing, and the release notes are judgement calls that belong to a maintainer.