[airween]

what

Limit the number of match offsets that @validateByteRange and @rxGlobal add to the rule message reference to 20 per evaluation.

why

@validateByteRange appends one o<offset>,1 entry to RuleMessage::m_reference for every byte outside the allowed range, and @rxGlobal appends one entry for every capture. Neither has an upper limit. For a rule like

SecRule ARGS:p "@validateByteRange 32-126" "id:1,phase:2,deny,status:403,t:none"

and GET /?p= followed by 40 times %01, the error log line ends in

[ref "o0,1o1,1o2,1 ... o38,1o39,1v8,40"]

That is 40 entries for 40 input bytes. When the operator is applied to a large value such as REQUEST_BODY (CRS 920272 and 920273 do this at paranoia levels 3 and 4), each entry takes up to 11 characters, so the reference grows to about ten times the size of the inspected body. Nothing reads the offsets back, and the operator's result only depends on whether there was at least one match. The string is still:

• kept in Transaction::m_rulesMessages for the whole transaction,
• copied again when the audit log is generated,
• written in full to a JSON audit record (src/transaction.cc, LOGFY_ADD("reference", ...)).

The error log and the Native audit log already cut the reference to 200 characters (RuleMessage::_details()), but only after the full string has been built and copied.

how

• Add Operator::MAX_LOGGED_OFFSETS (20) next to logOffset() in src/operators/operator.h. Twenty entries fit in about 220 characters, which matches the 200-character limit the text logs already apply.
• @validateByteRange stops logging offsets after the 20th, but still counts every out-of-range byte.
• @rxGlobal logs the offsets of the first 20 captures only. It still stores every capture in TX when capture is set.

The match result of both operators does not change. The other logOffset() callers log a single offset per evaluation, or one per capture group of the rule's own pattern, so they are not changed. No public header under headers/modsecurity/ is touched.

The reference is not part of the portable logging contract (seclang-spec ADR-0021: only [id], [msg] and [data] are normative, and the JSON audit layout is unspecified). The only visible change is that a JSON audit record lists at most 20 offsets for these two operators. The text logs look the same as before.

tests

Two new regression cases, both in files that are already listed in test/test-suite.in:

• operator-validate-byte-range.json: 40 out-of-range bytes, expects exactly 20 offsets followed by the variable entry.
• operator-rxGlobal.json: 40 matches, same expectation.

Both cases fail without the change and pass with it. make check passes (5144 passed, 2 skipped), make check-style reports nothing new for the touched files, and the seclang-spec libmodsecurity conformance adapter shows no new failures.

AI disclosure

Made with help of Claude Code, Opus 5.5

Summary by CodeRabbit

• Bug Fixes
  • Limited logged match references to approximately 1,024 characters for global regular-expression matches and byte-range validation, keeping audit-log entries more concise for large inputs. A single reference entry may exceed the limit.
  • Requests are still evaluated against the full input, so matching and out-of-range validation outcomes are unchanged.
  • Removed offsets from negated-operator matches no longer carry over into subsequent values or appear in their logged references.

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Operator reference accumulation is capped at 1024 characters. RuleWithOperator, RxGlobal, and ValidateByteRange use bounded appending or stop recording details at the limit. Operator evaluation restores the original reference length when the final result is false. Regression cases check bounded references and negated byte-range matching.

Changes: Bound operator references

Layer / File(s)	Summary
Add bounded reference appends src/operators/operator.h, src/rule_with_operator.cc	Operator adds helpers that skip appends when the current reference is at least 1024 characters. logOffset and RuleWithOperator use the bounded append helper. A single appended entry can exceed the limit.
Apply limits and verify operator references src/operators/rx_global.cc, src/operators/validate_byte_range.cc, src/operators/operator.cc, test/test-cases/regression/operator-rxGlobal.json, test/test-cases/regression/operator-validate-byte-range.json	RxGlobal stops logging capture offsets at the limit. ValidateByteRange stops scanning after logging an out-of-range byte when the limit is reached. Operator::evaluateInternal restores the original reference length when the final result is false. Regression cases check bounded references and negated byte-range matching.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: zimmerle, fzipi

Merge Risk: Minimal · up to 57957

This change caps how large the rule reference can grow for these operators and restores it correctly when a negated operator does not match. No actionable merge-blocking risk remains.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title names the main change: limiting how many offsets @validateByteRange and @rxGlobal add to the rule reference. The objectives confirm this is the core intent. The title is specific and matches…
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Title check

Explanation

The title names the main change: limiting how many offsets @validateByteRange and @rxGlobal add to the rule reference. The objectives confirm this is the core intent. The title is specific and matches the changeset. It says 'limit the offsets' while the implementation caps the reference at 1024 characters, which is a close match to the stated goal.

Finishing Touches

Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

• X
• Mastodon
• Reddit
• LinkedIn

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5a81dabe-2c55-4c07-a63e-dc724ab07cc5

Commits

Reviewing files that changed from the base of the PR and between 77ced5e and 57957d1.

Files selected for processing: 2

• src/operators/operator.cc
• test/test-cases/regression/operator-validate-byte-range.json

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.

[Copilot]

🟢 Approval recommended

No active review issues are reported for the change that limits logged match offsets.

No open issues 🎉

What changed in this PR

This PR bounds match-offset references for @validateByteRange and @rxGlobal, reducing reference growth for large inputs without changing matching results or capture storage.

Changes:

• Adds a shared limit of 20 logged offsets per operator evaluation.
• Applies the limit while preserving full invalid-byte counting and capture processing.
• Adds regression cases that verify 40 matches produce exactly 20 logged offsets.

File	Description
test/​test-cases/​regression/​operator-validate-byte-range.json	Tests the offset limit with 40 out-of-range bytes.
test/​test-cases/​regression/​operator-rxGlobal.json	Tests the offset limit with 40 regex matches.
src/​operators/​validate_byte_range.cc	Caps logged offsets while counting all invalid bytes.
src/​operators/​rx_global.cc	Logs offsets for only the first 20 captures.
src/​operators/​operator.h	Defines and documents the shared offset limit.

_{Review effort: Balanced}

[fzipi]

Thanks, the problem is real and the tests are clear. I think the cap is in the wrong place, though, so the reference can still grow with the input.

1. The cap is per operator call, but m_reference is per rule.
RuleWithOperator::evaluate() passes the same RuleMessage to every target variable (and to chained rules), and appends v<offset>,<len> for each match (src/rule_with_operator.cc, around line 298-304). For a non-multiMatch rule the reference is only cleared after the final performLogging(). So with a rule like CRS 920270 (@validateByteRange on REQUEST_HEADERS|ARGS|ARGS_NAMES) and 1000 args, each holding 20 out-of-range bytes, we still build roughly 21,000 entries (~200 KB) that end up in m_rulesMessages and in the JSON audit reference. The new tests use a single target and only check the error log, which is already cut to 200 characters, so they can't catch this.

2. Suggestion: enforce the limit once, in logOffset().
Something like a size check on the reference itself:

static void logOffset(RuleMessage &ruleMessage, int offset, int len) {
    if (ruleMessage.m_reference.size() >= MAX_REFERENCE_SIZE) {
        return;
    }
    ...
}

with the same guard on the v.../transformation appends in rule_with_operator.cc. That covers all logOffset() callers, the build-up across variables and chained rules, and any future operator, without special cases in rx_global.cc and validate_byte_range.cc. It also fixes two smaller issues with the current version:

• @rxGlobal counts captures, not matches, so with a grouped pattern like (a)(b) the cut lands in the middle of the 7th match.
• The comment says 20 entries fit the 200-character error-log limit, but that only holds for small offsets. At body offsets around 1,000,000, 20 entries are already 200 characters and the v... entry naming the matched variable is cut off.

3. @rxGlobal memory is still unbounded.
Only the logged string is capped. searchGlobal() still collects every match, and with capture each one also goes into TX and m_matched. @rxGlobal . on a 1 MB body still produces about a million entries. That's probably a separate PR, but the description shouldn't suggest the growth is fixed.

Minor:

• In validate_byte_range.cc only count != 0 is used, so the loop could break at the cap instead of scanning the rest of a large body.
• In rx_global.cc, if (captures.size() > 0) return true; return false; is just return !captures.empty();.
• Not caused by this PR: offsets are appended before negation is applied, so with !@validateByteRange the offsets from a variable that did not match stay in the reference and get attached to the next variable that does match (e.g. a=%01%01&b=ok gives o0,1o1,1v..). A reference-level cleanup might be a good moment to fix that too.

[airween]

Thanks, all good points. I've pushed two more commits.

1–2. The limit is now on the reference, in one place. MAX_LOGGED_OFFSETS is gone. Operator::appendReference() drops an entry once m_reference has reached MAX_REFERENCE_SIZE (1024 bytes; one entry may cross it). logOffset() and the origin and transformation appends in RuleWithOperator::evaluate() both go through it, so every logOffset() caller is covered, as well as the build-up across target variables and chained rules. This also takes care of the two smaller issues: @rxGlobal no longer counts captures (it stops when the reference is full), and the 200-character error log limit is no longer the reasoning behind the number.

The tests now check the reference field of the JSON audit record, which isn't truncated: a single large value for both operators, and 60 target variables with 20 out-of-range bytes each for @validateByteRange. All of them fail on v3/master.

When the reference fills up with offsets, the v... entry of that match is dropped as well. I think that's acceptable for a value that has already produced a kilobyte of offsets, but tell me if you'd rather reserve room for it.

3. @rxGlobal memory. Agreed, that's separate. searchGlobal() still collects every match and capture still stores each one. I've changed the description so it doesn't suggest otherwise. Let's handle it separately.

Minor:

• @validateByteRange now stops scanning once the reference is full.
• return !captures.empty(); done.
• The negation leak is fixed in a separate commit: Operator::evaluateInternal() restores the reference to its previous size when the final result is not a match. There's a regression case for your a=%01%01&b=ok example: the reference is now v<offset>,2.

make check passes (5146 passed, 2 skipped), and make check-style and make cppcheck report nothing for the touched files.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[fzipi]

Thanks, this addresses everything from my first pass.

• The bound is now on the reference itself, and every append goes through appendReference(): logOffset() and the origin and transformation entries in RuleWithOperator::evaluate(). So the build-up across targets and chained rules is covered. The multi-variable test (60 args × 20 bytes) checks exactly that case, against the untruncated JSON reference.
• The early break in @validateByteRange is safe, since count is already non-zero when it fires. It also stops at the first bad byte when an earlier variable has already filled the reference.
• Restoring the reference size in evaluateInternal() when the final result is not a match covers both negated operators and any operator that logs before returning false. Since every rule goes through executeOperatorAt(), it's the right single place for it.

On your question about the v... entry: dropping it once a kilobyte of offsets has been logged is fine with me. The variable is still identified by the message and MATCHED_VAR_NAME. Reserving room for it would bring back per-variable growth unless it had a second bound, which isn't worth it for a highlighting aid.

One nit, not blocking: appendReference() is really a RuleMessage concern, but moving it there would touch the public header, so keeping it on Operator is a reasonable trade-off.

LGTM.