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.

Sign up for GitHub

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

Jump to bottom

Support configurable limit on number of arguments processed #2234

Closed
wants to merge 1 commit into from
Closed

Support configurable limit on number of arguments processed #2234

wants to merge 1 commit into from

Conversation

martinhsv
Copy link
Contributor

This is basically the same as pull request 2060, but adds automated tests.

@zimmerle zimmerle mentioned this pull request Feb 11, 2020
Closed
@zimmerle
Copy link
Contributor

Making reference to #2060

@zimmerle zimmerle self-assigned this Feb 11, 2020
@zimmerle zimmerle self-requested a review February 11, 2020 13:15
@zimmerle zimmerle added the 3.x Related to ModSecurity version 3.x label Feb 11, 2020
@zimmerle
Copy link
Contributor

@jleproust @martinhsv - Since the configuration directive is just arbitrating on the number of JSON parameters, it would be fair to call it SecJSONArgumentsLimit or maybe other name that references the fact that such configuration is related to the JSON implementation?

@martinhsv
Copy link
Contributor Author

The code in the pull request will limit the number of Arguments (within m_variableArgs) no matter how the elements are created. E.g. if you set the configurable value to '5', and send a request like this:

GET /index.html?k1=v1&my_idd=aaa&z=1&y=2&x=3&w=4&aaa=hello HTTP/1.1

... then the argument 'aaa' will get omitted/skipped.

So, SecArgumentsLimit, is the right choice to go with the rest of the code.

(The JSON-specific portion of the pull request is a minor extra bit of functionality to stop json-parsing if the argument limit has been reached.)

@airween airween mentioned this pull request Feb 11, 2020
Closed
@jleproust
Copy link
Contributor

@martinhsv Thanks a lot for adding the test! It would be really great if we could merge this.

@zimmerle
Copy link
Contributor

Thank you guys @martinhsv @jleproust

@zimmerle
Copy link
Contributor

Minor update, added: bad5892.

secargumentslimit.json is now part of Makefile.am. Otherwise it won't run on make check.

https://github.com/SpiderLabs/ModSecurity/blob/bad5892b93297d2f5284b8a6dc00026b91a57588/Makefile.am#L187

The new version is on the build bots.

@zimmerle
Copy link
Contributor

merged!

@zimmerle zimmerle closed this Feb 17, 2020
@jleproust
Copy link
Contributor

Yay thanks!

@rdegez
Copy link

rdegez commented Jun 18, 2020

Hi,

I thought this change made it to v3.0.4 like the changelog in https://github.com/SpiderLabs/ModSecurity/blob/v3/master/CHANGES file suggest :

v3.0.4 - 2020-Jan-13
--------------------

 - Support configurable limit on number of arguments processed
   [@jleproust, @martinhsv]
<...>

Except that it didn't :-o
It's in master tree but definitely not in v3.0.4 release :-/

I tried to set SecArgumentsLimit and the directive was unknown.

Then I checked in the code :

~/modsecurity-master$ grep -ri CONFIG_DIR_ARGS_LIMIT * | wc -l
23

~/modsecurity-v3.0.4$ grep -ri CONFIG_DIR_ARGS_LIMIT * | wc -l
0

Am I missing something ?

Regards,

@martinhsv
Copy link
Contributor Author

Hi @rdegez ,

You are correct, that code is not in v3.0.4, and the CHANGES file is incorrect. Thanks for raising that.

I will correct it.

martinhsv added a commit that referenced this pull request Jun 18, 2020
@martinhsv
Merge pull request #2342 from martinhsv/v3/master
a1a8c0f 
Correct CHANGES file entry for #2234
@martinhsv
Copy link
Contributor Author

The CHANGES file has been corrected.

@mlosapio
Copy link

I'm fairly certain this issue is only partially fixed with this patch. I'm happy to reopen an issue or file a new one..

Confirmation the patch works when content-type == json:

time curl -d @test.json https://localhost -k  -H 'Content-Type: application/json'
<html>
<head><title>405 Not Allowed</title></head>
<body>
<center><h1>405 Not Allowed</h1></center>
<hr><center>nginx</center>
</body>
</html>

real    1m31.949s
user    0m0.060s
sys     0m0.074s

Switch the feature on:

sed -i 's/#SecArgumentsLimit 6/SecArgumentsLimit 6/g' /etc/nginx/modsec/modsecurity.conf
systemctl restart nginx
time curl -d @test.json https://localhost -k  -H 'Content-Type: application/json'
<html>
<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx</center>
</body>
</html>

real    0m0.200s
user    0m0.050s
sys     0m0.076s

But if you change the Content-Type to something other than json such as "application/x-www-form-urlencoded", the DDoS condition still exists and an nginx thread is consumed.

@martinhsv
Copy link
Contributor Author

Hi @mlosapio ,

This configurable limit accomplishes two things:

  1. Overall, it will stop adding key-value pairs to the relevant ModSecurity variables (e.g ARGS) when the configured limit has been reached and
  2. For json content specifically, it will also stop json parsing once the limit has been reached.

The main performance issue being addressed here is the very large number of key-value pairs that might be processed by a large number of regular expressions in any sort of generic rule set (whether OWASP CRS or other). Change 1 accomplishes that even for non-json content. See the code changes in transaction.cc.

The second change, that applies only to json content, is a more minor benefit.

I'm fairly sure that Change 1 works correctly for a large set of application/x-www-form-urlencoded args. And I do recall trying that while reviewing the change back in February.

Nevertheless, if you think the number of arguments added to ARGS (etc.) is not being limited for non-json content, feel free to submit a pull request, or otherwise outline what you think is going wrong.

@owasp-modsecurity owasp-modsecurity deleted a comment from mlosapio Aug 20, 2020
vladbukin pushed a commit to vladbukin/ModSecurity that referenced this pull request Apr 12, 2022
@martinhsv @vladbukin
Correct CHANGES file entry for owasp-modsecurity#2234
5ce68c4
@martinhsv martinhsv mentioned this pull request May 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimmerle zimmerle Awaiting requested review from zimmerle

Assignees

@zimmerle zimmerle

Labels
3.x Related to ModSecurity version 3.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martinhsv @zimmerle @jleproust @rdegez @mlosapio