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.

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

Allow square brackets in variables (macro expansion) #1226

Merged
merged 2 commits into from
Nov 21, 2024

Conversation

geoolekom
Copy link
Contributor

Thank you for contributing to Coraza WAF, your effort is greatly appreciated
Before submitting check if what you want to add to coraza list meets quality standards before sending pull request. Thanks!

Make sure that you've checked the boxes below before you submit PR:

Thanks for your contribution ❤️

Summary

This update allows square brackets ([]) in variable names, enabling compatibility with parameters like ARGS.db-reset-tables[]. Previously, these would result in errors, limiting Coraza's ability to handle some ModSecurity rule sets.

Why This Matters?

Square brackets are widely used in GET parameters and filenames. For example:

  • PHP use them for array-like parameters (e.g., fields[name]=value or ARGS.fields[]).
  • jQuery serialize array inputs with square brackets in their names.

Without support for square brackets, users are forced to rewrite or bypass standard rules to handle these cases, reducing security coverage.

What's Changed:

  • Updated the parser to accept [] in variable names.
  • Added tests to confirm proper handling of variables with [].

With this change, rules like the following are now supported:

SecRule ARGS.db-reset-tables[] "@contains sensitive" "id:1001,phase:2,deny"

This brings Coraza closer to full compatibility with ModSecurity configurations.

@geoolekom geoolekom requested a review from a team as a code owner November 20, 2024 07:55
@M4tteoP
Copy link
Member

M4tteoP commented Nov 21, 2024

Hey @geoolekom, thanks for this!
I just wish to double-check the scope of the PR, because the description seems not fully aligned with the code.

With this change, rules like the following are now supported:

SecRule ARGS.db-reset-tables[] "@contains sensitive" "id:1001,phase:2,deny"

I tested the following rules, and they actually worked as expected.

SecRule ARGS:key[] "@contains sensitive" "id:1001,phase:2,deny,msg:'%{MATCHED_VAR_NAME} %{MATCHED_VAR}'"
SecRule ARGS:key[value] "@contains sensitive" "id:1002,phase:2,deny,msg:'%{MATCHED_VAR_NAME} %{MATCHED_VAR}'"

What is not supported, and I think is the scope of the issue (indeed code changes are about macro expansion), is:

SecRule ARGS:key2[] "@contains sensitive" "id:1003,phase:2,deny,msg:'following marco expansion is not supported: %{ARGS.key2[]}'"

Are we on the same page? Thanks!

@geoolekom
Copy link
Contributor Author

@M4tteoP oh, yes. Indeed, I meant the variables in macro. I missed a msg part in my example, my bad. Fixed:

SecRule ARGS.db-reset-tables[] "@contains sensitive" "id:1001,phase:2,deny,msg:'following marco expansion is not supported: %{ARGS.db-reset-tables[]}'"

@M4tteoP
Copy link
Member

M4tteoP commented Nov 21, 2024

Ok, great, thanks for confirming it!

Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.66%. Comparing base (244ba00) to head (1f4db6a).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1226   +/-   ##
=======================================
  Coverage   81.66%   81.66%           
=======================================
  Files         168      168           
  Lines        9655     9655           
=======================================
  Hits         7885     7885           
  Misses       1519     1519           
  Partials      251      251           
Flag Coverage Δ
coraza.rule.case_sensitive_args_keys 81.62% <100.00%> (ø)
coraza.rule.multiphase_valuation 81.66% <100.00%> (ø)
coraza.rule.no_regex_multiline 81.60% <100.00%> (ø)
default 81.66% <100.00%> (ø)
examples+ 16.45% <0.00%> (ø)
examples+coraza.rule.case_sensitive_args_keys 81.62% <100.00%> (ø)
examples+coraza.rule.multiphase_valuation 81.50% <100.00%> (ø)
examples+coraza.rule.no_regex_multiline 81.52% <100.00%> (ø)
examples+memoize_builders 81.62% <100.00%> (ø)
examples+no_fs_access 80.94% <100.00%> (ø)
ftw 81.66% <100.00%> (ø)
memoize_builders 81.76% <100.00%> (ø)
no_fs_access 81.10% <100.00%> (ø)
tinygo 81.63% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Member

@M4tteoP M4tteoP left a comment

Choose a reason for hiding this comment

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

LGTM! I would also create a new test under https://github.com/corazawaf/coraza/blob/main/testing/engine/variables.go with some rules playing with square brackets in both variables and macro expanded ones, but it can be done in a followup PR

@M4tteoP M4tteoP changed the title Allow square brackets in variables Allow square brackets in variables (macro expansion) Nov 21, 2024
@M4tteoP M4tteoP merged commit 606a1bb into corazawaf:main Nov 21, 2024
71 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants