ModSecurity icon indicating copy to clipboard operation
ModSecurity copied to clipboard

Published 20 hours ago • verified publisher icon owasp-modsecurity

WIP: Add RE2 regexp engine support

Open WGH- opened this issue 6 years ago • 11 comments

See #1996.

Every commit has detailed description.

I've implemented the fallback approach so far: if RE2 support is enabled, every regular expression is compiled with RE2, and if that fails, regular expression is compiled with libpcre instead. Please see #1996 for more detailed discussion why this approach is necessary.

There's also lots of refactoring of regexp related code.

Open questions:

  • [ ] Perhaps, since RE2 support is considered experimental, configure script should not even attempt to build it unless explicit --with-re is supplied?
  • [ ] There's no easy way to know which regular expression uses which engine. It might feel like kind of implicit magic, which isn't really good.
  • [x] There's no error handling when regular expression is invalid. This isn't a regression introduced by this PR, but now might be the time to do something about it.
  • [ ] PR needs code review, obviously.
  • [ ] Changelog entry
  • [ ] Documentation
  • [ ] Maybe something else I missed.

WGH- avatar Jan 30 '19 14:01 WGH-

I've just force-pushed updated PR.

  • New commit with fixed error handling for the rx operator.
  • get_pattern was renamed to getPattern to adhere to naming convention used elsewhere in ModSecurity.
  • verifyCC conversion to the new classes, which was done very sloppily earlier, has been re-done properly.

WGH- avatar Feb 05 '19 14:02 WGH-

Since my changes have been essentially merged into v3/dev/re2, I updated the PR to target v3/master branch now.

WGH- avatar Feb 06 '19 19:02 WGH-

Overall, the performance for the libpcre users was not impacted by this pull request:

screenshot_20190206_164323

The chart was generated using the code merged into v3/dev/re2.

zimmerle avatar Feb 06 '19 19:02 zimmerle

Ping.

WGH- avatar Nov 08 '19 10:11 WGH-

Just in case, even though it's marked WIP, it's more than ready for the first round of review.

WGH- avatar Jan 14 '20 21:01 WGH-

Rebased on top of the current master.

WGH- avatar Sep 05 '20 14:09 WGH-

Very interesting commit. We recently ran into performance issues with modsecurity. Any news on how much improvement we might see?

eddie4 avatar Sep 28 '20 14:09 eddie4

Very interesting commit. We recently ran into performance issues with modsecurity. Any news on how much improvement we might see?

Hi @eddie4,

That is not the single front that we have on the performance side. Check the branch v3/dev/3.1-experimental. We have had great progress there. This is still on the Queue, to be merged once other pieces of v3.1 are complete.

zimmerle avatar Sep 28 '20 14:09 zimmerle

Very interesting commit. We recently ran into performance issues with modsecurity. Any news on how much improvement we might see?

@eddie4 in my experience, it doesn't really make it much faster in average, however, it eliminates ReDoS worst cases.

WGH- avatar Sep 28 '20 14:09 WGH-

@zimmerle please ping me when it's time to rebase this on top v3/dev/3.1-experimental.

WGH- avatar Oct 29 '20 19:10 WGH-

@zimmerle please ping me when it's time to rebase this on top v3/dev/3.1-experimental.

That is definitively something that I want to see happening. Lets have 3.1-experimental stable first. Working on the new issues that you have reported on #2376

zimmerle avatar Oct 29 '20 22:10 zimmerle