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

invalid feature request: disable parse fullRequest #2701

Closed
liudongmiao opened this issue Mar 16, 2022 · 1 comment
Closed

invalid feature request: disable parse fullRequest #2701

liudongmiao opened this issue Mar 16, 2022 · 1 comment

Comments

@liudongmiao
Copy link
Contributor

In our case, disable parse query_string / www-form-based request_body / cookies, the performance increase 50%.

And in our case, we disable requestBody for multi-part request (which most of time include large files), and full request too.

There even a comment on full request in current code base, lol.

    /**
     * FIXME: This variable should be calculated on demand, it is
     * computationally intensive.
     */
@martinhsv
Copy link
Contributor

@liudongmiao ,

It is important to keep in mind that there are two separate pieces of functionality that can affect performance:

  1. for each portion of the request, the actual parsing that occurs within ModSecurity, regardless of what rule set you are using
  2. after the parsing is complete, the set of rules (possibly large) that runs against the parsed content

In the case of query arguments and cookies, it is quite unlikely that it is the parsing (1) that is causing significant performance impact.

Depending upon the number of each, query arguments and/or the number of cookies can be expensive if run against a large enough set of rules (i.e. 2 above) -- especially if many of the rules use relatively more expensive operators (e.g. rx vs. streq).

However, there already are ways for the rule writer to omit this step (2) -- see, for example, the SecRuleUpdateTarget* family of configuration items.

The above, combined with the fact that I would expect a desire to avoid inspecting query args and cookies to be quite rare (this is a WAF, after all), I would not be in favour of introducing new controls to omit parsing of query arguments and cookies.

Performance issues related to the request body are a little different, partly (but not solely) because of the much larger potential size. Of course, any situations where (2) is the issue, the normal ways to address them are already available.

For performance related to populating request-body-related ModSecurity variables (parsing, etc.) there are a mixture of controls currently available including SecRequestBodyLimit, SecRequestBodyNoFilesLimit (not available in v3.0.6 but in v3/master), SecArgumentsLimit, and SecRequestBodyJsonDepthLimit -- and all of these are in addition to options available via SecRule.

In the case of the REQUEST_BODY variable specifically, I doubt that mere population of that variable (1) is causing large performance impacts. However, insofar as that might the case, any "don't populate" solution for this would likely be either a) at least partly subsumed under #2465, or if we are become convinced that mere population of the variable is a major problem, a duplicate of #2146.

As a short summary, I would tentatively plan to close this item.
For Query Arguments/Cookies: Won't implement
For request body: either won't implement or duplicate

I will leave this open temporarily, however, in case you have any questions.

@liudongmiao liudongmiao changed the title feature request: disable parse args_get / args_post / cookies / fullRequest / requestBody invalid feature request: disable parse args_get / args_post / cookies / fullRequest / requestBody Mar 19, 2022
@liudongmiao liudongmiao changed the title invalid feature request: disable parse args_get / args_post / cookies / fullRequest / requestBody invalid feature request: disable parse fullRequest Mar 19, 2022
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

No branches or pull requests

2 participants