You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: unescape double quotes in operator parameters - #3753
keeps the literal two-character sequence \" in the operator parameter instead of the intended ", and the operator never matches a request argument containing test()"quote".
Fix
Add a small unescapeOperatorParam helper in the scanner, applied in the four actions of the two double-quoted operator-param lexer states (with and without macro expansion, with and without leading escape). The helper walks the parameter text: a backslash sets an escape flag and is kept; if the flagged character turns out to be a ", the pending backslash is removed and a literal " is emitted; every other character is copied verbatim. Net effect: \" becomes " (the reported bug), and a backslash before any other character is preserved verbatim, so regex escapes such as \d are untouched.
One escaping backslash is removed before a quote when the run has odd length; every other backslash remains unchanged. This matches the reported one-backslash case, not full Apache unescaping.
The generated seclang-scanner.cc is regenerated with flex 2.6.4 (the CI image's version) through the repo's --enable-parser-generation path; its diff is line-number churn plus the four helper calls.
Tests
New regression file test/test-cases/regression/operator-contains-escaped-quote.json (plus one line in test-suite.in) with four cases:
contains positive: the issue's rule matches a body containing test()"quote" (Rule returned 1);
contains negative: the same rule does not match when the unescaped parameter is absent;
rx operator: @rx ^\d+$ still works, proving regex backslash escapes survive the helper;
default (rx) operator: an escaped quote inside an implicit-rx parameter is also unescaped.
The file fails with the base scanner and passes after the change. Additional local boundary checks (macro expansion inside the parameter, backslash runs before a quote) pass; those are local verification, not part of the committed test file.
Test results (local)
Build env: pcre2 10.39, yajl 2.1.0, flex 2.6.4/bison, configure --without-curl --without-lua --without-maxmind --without-geoip --without-ssdeep --without-lmdb --without-libxml --enable-assertions=yes --enable-parser-generation (jammy packages, same versions as the CI image).
make check: TOTAL 5148, PASS 5053, SKIP 95, FAIL 0, ERROR 0. The 95 skips sit in test files for the disabled optional features (geo/maxmind, XML/libxml, Lua, inspectFile, fuzzyHash/ssdeep, remote-rules/curl, exec, and one pcre dollar-endonly variant); those code paths were not exercised in this environment, so they are unverified here rather than proven unaffected. The 4 new cases pass.
./unit_tests (broader than the automake list): 4336 total, 11 failed, 5 skipped. The 11 failures are all in transformations/phpArgsNames.json, match the comparison build with both scanner sources restored from the base commit; the new regression tests remained present, and that file is not in the automake make check TESTS list upstream; the 5 skips are geoLookup (feature disabled).
Static analysis: cppcheck with the repo's flags over the changed-code neighborhood (44 files: src/operators/*, run_time_string.cc, rule_with_operator.cc, parser/driver.cc) reports no non-missingInclude findings. This is a subset, not the full make check-static gate; the upstream gate excludes the generated parser/scanner sources (-i flags), which are the only changed .cc files.
Scope note
The change touches only the scanner's double-quoted operator-param states. Single-quoted params, variable names, actions, and non-quoted free text are untouched.
Summary by CodeRabbit
Bug Fixes
Operator parameters containing escaped double quotes are now handled correctly. Rules using text matching and regular expressions can match these parameters as expected, while parameters without the quoted text remain unmatched. This resolves incorrect rule results for escaped-quote inputs.
Within a double quoted operator parameter, an escaped double quote (")
now stands for a literal quote character, matching ModSecurity v2
behavior (Apache's ap_getword_conf performed this unescaping for v2).
Previously the parameter reached the operators with the escape
backslashes intact, so a rule such as
SecRule ARGS "@contains test()\"quote\"" never matched.
Fixesowasp-modsecurity#3750.
Co-authored-by: GeneralTony007 <144252170+GeneralTony007@users.noreply.github.com>
The scanner now removes the escape backslash before double quotes in operator parameters. Four regression cases cover @contains, @rx, and default-operator matching, including a non-match case.
Changes: Operator parameter unescaping
Layer / File(s)
Summary
Unescape operator parameters and validate matching src/parser/seclang-scanner.ll, test/test-cases/regression/operator-contains-escaped-quote.json, test/test-suite.in
The scanner removes a backslash immediately before a double quote in both quoted operator-parameter paths. Regression cases check @contains matching and non-matching, an @rx match, and a default-operator match. The test-suite manifest includes the regression case.
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
The title "fix: unescape double quotes in operator parameters" matches the main change. The scanner change removes the escape backslash before a double quote in double-quoted operator parameters, whic…
Linked Issues check
Issue #3750 requires that an escaped double quote in an @contains parameter match a literal quote, so that ARGS containing test()"quote" is blocked. At the reviewed head, the scanner helper unescapeOp…
Out of Scope Changes check
All changed source hunks in src/parser/seclang-scanner.ll touch only the helper and the four double-quoted operator-parameter lexer actions. The helper is the mechanism named in issue #3750. The regre…
Full details: Title check
Explanation
The title "fix: unescape double quotes in operator parameters" matches the main change. The scanner change removes the escape backslash before a double quote in double-quoted operator parameters, which is what the title describes. The title is short, specific, and uses a conventional fix: prefix. It does not cover the regression tests or the test-suite manifest update, which is acceptable because those are supporting changes.
Full details: Linked Issues check
Explanation
Issue #3750 requires that an escaped double quote in an @contains parameter match a literal quote, so that ARGS containing test()"quote" is blocked. At the reviewed head, the scanner helper unescapeOperatorParam in src/parser/seclang-scanner.ll removes the escape backslash before a double quote. It applies to the double-quoted operator-parameter states, both with and without macro expansion and with and without leading escape. For SecRule ARGS "@contains test()"quote"", the operator receives test()"quote", which matches the issue's request argument. The helper's logic was read in the diff and matches the description: an odd run of backslashes before a quote loses one backslash, and other backslashes are kept, so regex escapes such as \d are preserved. The regression cases in test/test-cases/regression/operator-contains-escaped-quote.json were not read directly. The PR reports that they pass locally. The regenerated seclang-scanner.cc was not inspected. It is generated from the .ll file, which was reviewed.
Full details: Out of Scope Changes check
Explanation
All changed source hunks in src/parser/seclang-scanner.ll touch only the helper and the four double-quoted operator-parameter lexer actions. The helper is the mechanism named in issue #3750. The regression JSON file and the one-line test-suite.in entry support the new behavior. The regenerated seclang-scanner.cc is the build output of the same .ll change, as stated in the PR description. No unrelated changes were identified in the inspected hunks. Single-quoted parameters, variable names, and actions are not altered by the inspected diff.
Finishing Touches
Generate unit tests (beta)
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Review coverage is incomplete: 1 file could not be fully reviewed. Findings from completed review steps are included; see review info for details.
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.
Files not fully reviewed because they exceed a per-file size limit: 1
src/parser/seclang-scanner.cc
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3750.
Problem
In v3, an escaped double quote inside a double-quoted operator parameter is not unescaped by the SecLang scanner, so a rule like the issue's:
keeps the literal two-character sequence
\"in the operator parameter instead of the intended", and the operator never matches a request argument containingtest()"quote".Fix
Add a small
unescapeOperatorParamhelper in the scanner, applied in the four actions of the two double-quoted operator-param lexer states (with and without macro expansion, with and without leading escape). The helper walks the parameter text: a backslash sets an escape flag and is kept; if the flagged character turns out to be a", the pending backslash is removed and a literal"is emitted; every other character is copied verbatim. Net effect:\"becomes"(the reported bug), and a backslash before any other character is preserved verbatim, so regex escapes such as\dare untouched.One escaping backslash is removed before a quote when the run has odd length; every other backslash remains unchanged. This matches the reported one-backslash case, not full Apache unescaping.
The generated
seclang-scanner.ccis regenerated with flex 2.6.4 (the CI image's version) through the repo's--enable-parser-generationpath; its diff is line-number churn plus the four helper calls.Tests
New regression file
test/test-cases/regression/operator-contains-escaped-quote.json(plus one line intest-suite.in) with four cases:test()"quote"(Rule returned 1);@rx ^\d+$still works, proving regex backslash escapes survive the helper;The file fails with the base scanner and passes after the change. Additional local boundary checks (macro expansion inside the parameter, backslash runs before a quote) pass; those are local verification, not part of the committed test file.
Test results (local)
Build env: pcre2 10.39, yajl 2.1.0, flex 2.6.4/bison, configure
--without-curl --without-lua --without-maxmind --without-geoip --without-ssdeep --without-lmdb --without-libxml --enable-assertions=yes --enable-parser-generation(jammy packages, same versions as the CI image).make check: TOTAL 5148, PASS 5053, SKIP 95, FAIL 0, ERROR 0. The 95 skips sit in test files for the disabled optional features (geo/maxmind, XML/libxml, Lua, inspectFile, fuzzyHash/ssdeep, remote-rules/curl, exec, and one pcre dollar-endonly variant); those code paths were not exercised in this environment, so they are unverified here rather than proven unaffected. The 4 new cases pass../unit_tests(broader than the automake list): 4336 total, 11 failed, 5 skipped. The 11 failures are all intransformations/phpArgsNames.json, match the comparison build with both scanner sources restored from the base commit; the new regression tests remained present, and that file is not in the automakemake checkTESTS list upstream; the 5 skips are geoLookup (feature disabled).make check-staticgate; the upstream gate excludes the generated parser/scanner sources (-iflags), which are the only changed .cc files.Scope note
The change touches only the scanner's double-quoted operator-param states. Single-quoted params, variable names, actions, and non-quoted free text are untouched.
Summary by CodeRabbit