Skip to content

ModSecurity DoS Vulnerability in JSON Parsing (CVE-2021-42717) #2647

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

Closed
martinhsv opened this issue Nov 23, 2021 · 11 comments
Closed

ModSecurity DoS Vulnerability in JSON Parsing (CVE-2021-42717) #2647

martinhsv opened this issue Nov 23, 2021 · 11 comments

Comments

@martinhsv
Copy link
Contributor

Some individuals in the ModSecurity community will have already taken note that new releases have been created for both the v2 branch and the v3 branch, and that the content in each case is listed as addressing a security issue.

This github issue is being created to provide some interim information until a somewhat more detailed blog post is available (which is expected within the next 24 hours at https://www.trustwave.com/en-us/resources/blogs/spiderlabs-blog/ ). The main specific motivation being to suggest a few mitigation strategies.

The ModSecurity team received a responsible disclosure from @theMiddleBlue about a possible DoS issue in ModSecurity v3 when JSON parsing depth becomes very large. Assistance was later also received from @airween in confirming that the issue likewise exists in v2 and in validating the ultimate fix in that branch. Thanks to both. The DoS in this case is that an attacker could potentially prevent a web server from responding to legitimate requests in a timely manner.

At this early stage of public disclosure, I will omit further details except as relate directly to mitigation strategies. Upgrading the software is the primary recommendation, but the mitigation strategies below may be useful for various cases where upgrading immediately is not possible.

Mitigation Strategies:

If your web applications do not expect JSON request bodies, you can simply disable any rules that activate the JSON parser. The rule that does this by default in modsecurity.conf-recommended is 200001.

Most other mitigation strategies are likely to take advantage of the fact that to create large JSON parsing depth necessitates using a certain minimum number of characters. This implies that this large-depth issue can potentially be controlled by examining total request size.

If, for example, you do have some applications that accept JSON request bodies, but that in legitimate requests they should all be relatively small, you could create a phase:1 chained rule that rejects the request if the JSON parser has been activated and the value in the Content-Length header is larger than a chosen threshold (perhaps 10000 or 20000).

ModSecurity v2 users can potentially also make use of SecRequestBodyNoFilesLimit if other request types are also within your chosen threshold.

As a special note to users still on versions <2.9.3: Although the problem exists in principle in earlier versions like 2.9.2, it does appear to take a larger request body size to cause noticeable problems. Whether using SecRuleBodyNoFilesLimit, or a chained rule that looks at Content-Length, most installations on such older versions can likely mitigate the issue successfully with somewhat higher thresholds (perhaps 40000 or 50000).

@theMiddleBlue
Copy link

Thanks @martinhsv !

Is it possible to add the new SecRequestBodyJsonDepthLimit directive into the modsecurity.conf-recommended file for v3/master branch?

@airween
Copy link
Member

airween commented Nov 24, 2021

Also would be fine to place it in the Reference. This keyword affects both versions.

@airween
Copy link
Member

airween commented Nov 24, 2021

Is there any reason why is the default value so high in both versions (10000)?

I've checked for example PHP's json_encode() function, looks like the default value is 512 - I think that would be enough. (Without documentation or any other information (like default modsecurity.conf) it's hard to see it, and user can forgot to decrease it.)

@martinhsv
Copy link
Contributor Author

Hi @theMiddleBlue ,

I had given a bit of thought as to whether it would be good to make such an addition to modsecurity.conf-recommended. I was -- and still am -- of two minds about that.

Items in that file are particularly useful for things that users are likely to want to change -- either because there is no useable default set within the software itself, or there is a default but it's one that in the normal course users are likely to want to change it. My guess is that the vast majority of users will not seek to change the default value of SecRequestBodyJsonDepthLimit.

Since there is a default limit in the software, if I were to add an entry to the -recommended file, it would be likely be a commented-out example (with a descriptive comment).

There is small disadvantage to making extra additions to that file for things that most users are not going to care about. Doing so can clutter the file and make for one extra thing that a new user will spend some seconds thinking about when doing a first setup.

As I said, I'm not really either in-favour or against. I will hold off on making a change for now but will review it in a couple of weeks. In the meantime, members of community who care one way or the other can signal approval for the idea (e.g. with thumbs up) or disapproval (thumbs down).

(Incidentally: You specifically mention v3/master. Given that the issue, and fix, are relevant to both v2 and v3, if I were to make a change to the -recommended file, I think it would make sense to do so in both branches.

@martinhsv
Copy link
Contributor Author

martinhsv commented Nov 24, 2021

Hi @airween ,

Good point about the Reference manual. Thanks for suggesting it.

[Edit: Done]

@martinhsv
Copy link
Contributor Author

Hi @airween ,

Regarding the selection of the default depth limit:

Since the depth limit is configurable, the selection of the default is, to a significant extent, a secondary consideration. It should, of course, be somewhere within a reasonable range.

Potential default values in this case could be considered candidates where both of the following would be true:

  1. The value was low enough such that the great majority of cases could have reasonable protection against the issue merely by upgrading the software -- i.e. that most users would not have to both upgrade the software and make a change to a configuration file.
  2. The value should be high enough so that it would be extremely unlikely that anyone would have to make a configuration change to increase the limit. It's an undesirable property for a security fix to unnecessarily break any valid functionality. I did take note of JSON depth limits implemented elsewhere, but had to take into account the variety of backend technologies in use, as well as the possibility that some very rare applications be quite esoteric and use unusually large depth.

In the end, I considered values as low as 1000 and as high as 10000. The final selection was a judgement call.

@877509395
Copy link

877509395 commented Nov 29, 2021 via email

@martinhsv
Copy link
Contributor Author

Hi @877509395 ,

I have no plans to publish anything resembling a PoC beyond what has already been provided. (Or provide any links to the same.)

There is the risk that doing so would be of greater benefit (even if only marginal benefit) to individuals with malicious intent than it would be for users wanting to protect themselves.

This is especially true in the short term while there may be users who want to address the issue -- whether through upgrade or other measures -- but have not yet had a chance to do so.

@877509395
Copy link

@martinhsv thanks.

@martinhsv
Copy link
Contributor Author

The files for the Windows 64-bit installer are now available at https://github.com/SpiderLabs/ModSecurity/releases/tag/v2.9.5

@martinhsv
Copy link
Contributor Author

Regarding modsecurity.conf-recommended:

I have gone ahead and created pull requests for both v2 and v3 that add SecRequestBodyJsonDepthLimit with a value of 512. I will plan to merge these within the next couple of days.

While the software default seemed suitable for the initial rollout of the functionality, having the out-of-the-box setting be somewhat lower is also sound. The value chosen is the suggestion made by @airween , which is of course also the default for php.

Thanks all for the various input on this matter.

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

4 participants