Skip to content

Expand ReDos checks#49

Closed
MegaManSec wants to merge 6 commits intodvershinin:masterfrom
MegaManSec:more-redos
Closed

Expand ReDos checks#49
MegaManSec wants to merge 6 commits intodvershinin:masterfrom
MegaManSec:more-redos

Conversation

@MegaManSec
Copy link

This PR adds more checks for ReDoS locations.

It also introduces a primitive MapDirective and GeoDirective, which can be used later to fully support Map/Geo (looks like that's going to be incredibly difficult, if not impossible, though).

elif directive.name == 'rewrite':
# rewrite ^/1(/.*) $1 break;
regex_patterns.add(directive.pattern)
elif isinstance(directive, MapBlock):
Copy link
Author

Choose a reason for hiding this comment

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

This incorrectly checks every type of map{}, regardless of whether it's `location', 'server_name', 'proxy_redirect', 'if', 'rewrite'. This is kind of deliberate, but I wonder if it can be handled better, i.e. if it could be possible to resolve the variable, determine that it resolves to a Mapblock, and then loop over the Mapblock's children..

@sonarqubecloud
Copy link

@MegaManSec MegaManSec marked this pull request as draft May 13, 2025 21:09
@dvershinin dvershinin closed this Dec 6, 2025
@dvershinin
Copy link
Owner

Closing this draft PR. While expanding ReDoS checks to more directive types is a valid goal, this implementation has some issues:

  1. External server dependency - The regex_redos plugin already requires an external server, and this PR significantly increases the complexity of that interaction
  2. Incomplete abstractions - The MapDirective/GeoDirective are self-described as "primitive" with full support being "incredibly difficult"
  3. Mixed concerns - Includes unrelated fixes to docstrings and proxy_pass_normalized logic

The existing regex_redos plugin checking location blocks covers the most common case. If we expand in the future, we'd prefer:

  • A local library approach (e.g., Python recheck bindings) over external server calls
  • Incremental, focused PRs for each directive type
  • Complete implementations rather than partial abstractions

Thanks for the work on this!

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.

2 participants