Finish the SecRequestBodyNoFilesLimit configuration parameter

[airween]

It looks like the SecRequestBodyNoFilesLimit configuration parameter is only half done, not much was missing.

Is there any reason it's just almost done? I mean any kind of security risk/issue (you just ignored it during the developing...). As you can see, there isn't so much addition code.

May be it's not what you expected (or planned), so if you have any idea, please let me know.

I think this missing feature would be at least as good as #2060 and #2234.

[zimmerle]

Thank you for sharing your opinion. Do you mind to share why do you need such a feature?

[dune73]

It is useful against some forms of DoS attacks, it used to be there in 2.9 and it is documented to be present in 3.0, is not it. It's troubling to learn it is no longer supported.

[zimmerle]

It is useful against some forms of DoS attacks, it used to be there in 2.9 and it is documented to be present in 3.0, is not it. It's troubling to learn it is no longer supported.

Wiki - Supported on libModSecurity: No

[airween]

Thank you for sharing your opinion. Do you mind to share why do you need such a feature?

(Well, I have to tell you that your question was a little surprised me :))

I think the best answer is a quote from the reference:

This directive is useful to reduce susceptibility to DoS attacks when someone is sending request bodies of very large sizes. Web applications that require file uploads must configure SecRequestBodyLimit to a high value, but because large files are streamed to disk, file uploads will not increase memory consumption. However, it’s still possible for someone to take advantage of a large request body limit and send non-upload requests with large body sizes. This directive eliminates that loophole.

But let me ask you something: why did you started to implement it? That's why I asked above, did you see some risk to finish this option?

[airween]

It is useful against some forms of DoS attacks, it used to be there in 2.9 and it is documented to be present in 3.0, is not it. It's troubling to learn it is no longer supported.

Wiki - Supported on libModSecurity: No

Sorry to say, but this state in reference was modified about 2 weeks ago. Before the supported state of this feature was "Yes". And if you see the attached modsecurity.conf.recommended for v3 (which is 16 months old), then you can see also that looks like as the implemented feature:

https://github.com/SpiderLabs/ModSecurity/blob/v3/master/modsecurity.conf-recommended#L39

# Maximum request body size we will accept for buffering. If you support
# file uploads then the value given on the first line has to be as large
# as the largest file you are willing to accept. The second value refers
# to the size of data, with files excluded. You want to keep that value as
# low as practical.
#
SecRequestBodyLimit 13107200
SecRequestBodyNoFilesLimit 131072

[zimmerle]

(Well, I have to tell you that your question was a little surprised me :))

I think the best answer is a quote from the reference:

This directive is useful to reduce susceptibility to DoS attacks when someone is sending request bodies of very large sizes. Web applications that require file uploads must configure SecRequestBodyLimit to a high value, but because large files are streamed to disk, file uploads will not increase memory consumption. However, it’s still possible for someone to take advantage of a large request body limit and send non-upload requests with large body sizes. This directive eliminates that loophole.

But let me ask you something: why did you started to implement it? That's why I asked above, did you see some risk to finish this option?

The manual (for 2x) state something that was a problem back on 2.5 (575e863). I understand why it was implemented back them. Now that 13 years have passed, why such a feature is important for you?

[airween]

Because the SecRequestBodyLimit is usually higher, and it contains the whole request body. Usually the users must to keep as "high" (for eg. check the uploaded files...), but if there is no file upload, everyone can tune the body limit for the other part of request body (excluded the files).

You wrote that you understand why was that implemented, and 13 years passed - yes, but it's still available in v2... so I don't understand why do you ask this. :(

[zimmerle]

Giving your use case scenario I may point you towards the right manner to achieve the level of protection that you want with ModSecurity v3 or even consider to support this. Without understanding what you aiming to do it will be very hard to help you. I am closing this until hear a solid use case for this feature.

[airween]

Right, thanks.

Just one question (again): could you tell me why did you started (and almost finished) this feature, if you saw that unnecessary?

[dune73]

@zimmerle : I think the loophole and the reason this was implemented 13 years ago still stands.

You describe the use case as not solid. But I see it as solid and the attack vector as very real: This directive helps to prevent DoS via large urlencoded POSTs while still allowing bigger file uploads. I do not see how you could prevent this with other means. But instead of explaining your reasoning, you declare the use case as unsolid and promptly reject @airween's pull request. I perceive this as unfriendly behavior.

You are free to run the ModSecurity project like this, but it harms future 3rd party contributions. And I think it harms ModSecurity as a whole.

[martinhsv]

Hi @dune73 ,

Just so I better understand the substantive technical concern, is it fair to say that you see the DoS risk as primarily computational (e.g. large volume of input being processed against many rules)? ... And not so much about things like memory consumption, for example?

If so, would your concern be alleviated if were to merge the pull request for SecArgumentsLimit as described here?:
#2060
#2234

[dune73]

Hey @martinhsv,

Thank you for this interesting alternative. I think SecArgumentsLimit is a welcome addition to the arsenal and I support its merging.

However, it does not address the specific use case SecRequestBodyNoFilesLimit was created for. The risk I see primarily affects bandwidth, probably cpu and possibly also memory, but I am unsure to what extent without examining the behavior of the binary.

Suppose we have a file payload.txt starting with "x=1234..." and a size of 10MB. Setting SecArgumentsLimit to 1 will allow the submission of this file if we do it as follows:

$> curl http://localhost -d "@payload.txt"

If I block the upload via SecRequestBodyLimit, this request has a total duration of 8000+ microseconds on Apache (ModSec 2.9.3 here)

However, if I set SecRequestBodyNoFilesLimit to 1024, then the duration is only 500+ microseconds. And file uploads up to 10MB are still possible, which is a necessity for certain setups.

I can try and block the request in phase 1 via a limit on the Content-Length header. This is even faster (380+ microseconds) since the request is blocked before the body reception even starts.

However, the attacker can circumvent this rule by using Transfer-Encoding as follows:

$> curl http://localhost -d "@payload.txt" -H "Content-Length:" -H "Transfer-Encoding: chunked"

With this we are back at 8K microseconds and I can't see how I could reduce this without SecRequestBodyNoFilesLimit.

So the directive saves me 7 milliseconds on an application DoS request. This is quite substantial. That's why I think this directive is useful. The use case is limited, but DoS against ModSec is really hard to fight and this is one of the weapons that we can use. This is aggrevated by the uncomfortable fact that CRS still has a few rules that come with Regular Expression DoS problems. We are weeding them out one after the other, but we are not yet there and using SecRequestBodyNoFilesLimit is a mitigation strategy.

Now there is more to this from a policy viewpoint. ModSec 3 was announced as a feature complete rewrite of ModSec 2. In this particular case ModSec 3 accepts the directive, but ignores it silently. I'm glad @airween discovered this shortcoming. Before this, I was not aware that the advice I was giving to people was in fact futile.

On top, the ModSec Handbook advertises the directive as a useful tool and many, many people use it, completely unaware that an upgrade to ModSec 3 opened their risk vs. App Level DoS - or the risk of an App Level DoS is now an additional reason to not upgrade to ModSec 3.

Now whatever is documented for ModSec 2 is not set in stone. Declaring certain directives and constructs legacy and asking people to move away from them is a necessity from time to time. Maybe SecRequestBodyNoFilesLimit is such a directive and there are good reasons to axe it. But if that is the case, I miss the reasoning and the considerate message that explains this decision to the community.

Kind regards,

Christian

[mmelo-yottaa]

Hi Christian -

Just to completely understand MS2 - how does SecRequestBodyNoFilesLimit determine the block in .5 msec vs SecRequestBodyLimit requiring 8 msec (both processing a 1024byte payload)? Is this a function of how MS2 implemented each of these? Does SecRequestBodyNoFilesLimit happen earlier in request processing? Thanks for any insight!

Mike M.

[dune73]

Hey @mmelo-yottaa,

No, it's a 10MB payload, but the SecRequestBodyNoFilesLimit stops the processing earlier. I do not really know the implementation - @airween and his PR above would know - but I presume it somehow stops receiving the data when the limit is reached. So accepting 1K vs 10,000K.

[airween]

Hí @mmelo-yottaa, that's the point, what @dune73 described.

Consider you know that your application needs maximum 1-2k of payload (JSON/XML/form-data) BUT sometimes you want to upload a file with size 1M.

If the request without file is greather than this value, then you can drop it, but in case of file upload, you can inspect it.

[zimmerle]

Just so I better understand the substantive technical concern, is it fair to say that you see the DoS risk as primarily computational (e.g. large volume of input being processed against many rules)? ... And not so much about things like memory consumption, for example?

If so, would your concern be alleviated if were to merge the pull request for SecArgumentsLimit as described here?:
#2060
#2234

Well mentioned @martinhsv. I think that seems to be o good alternative. The pull request #2234 is already reviewed and will be merged as soon as possible. Is there any use case that #2234 is not covering?

[martinhsv]

Hey @zimmerle , #2234 addresses the case where there is a large number of arguments (where each one may be a small number of bytes). He is suggesting the inverse scenario, where the number of arguments is small, but at least one of which is very long (possibly multiple megabytes). I did a bit of testing with that scenario in v3 and it's a case at least worth thinking about.

@dune73 : Thanks for the detailed explanation.

I don't believe the code in this pull request will do anything special like short-circuit the loading-into-memory of the body if the limit is exceeded.

What it can do, if the limit is exceeded, is prevent evaluation of any of the phase:2 rules that come after the rule that tests REQBODY_ERROR. That's not without value, of course, since running CRS phase:2 rules against a very large input can be quite expensive.

However, given that that is all it is doing, I suppose I'd wonder aloud whether other length-limiting options would be workable alternatives for the case you outline. For example this way to test the length of each argument:

SecRule ARGS "@gt 1024" "id:200007,phase:2,t:length,deny,status:400"

Or perhaps using ARGS_COMBINED_SIZE to test the total.

I guess I just want to make sure we understand what the functional requirement is, and that a particular implementation both satisfies that and is the best tool to do so.

Cheers,
Martin

[dune73]

Hi @martinhsv, glad you see some value in the use case.

And unfortunate, the bandwidth is consumed and all that we save is the body processor resources thanks to SecRequestBodyNoFilesLimit. The idea of dropping of the connection while it's being received was probably wishful thinking on my behalf.

But anyways, if the BodyProcessor eats 7 milliseconds and I can save that, then it's useful on my server.

Now your proposals make a lot of sense and running rules working on &ARGS and ARGS_COMBINED_SIZE all have merit; hence the rules 920360-920410 in CRS. And I'm sure there is more that can be done in this regard - those rules have not seen any review since a long time AFAICS.

The problem with all this is of course that it's running in phase 2 and you need to make sure it's running early in phase 2 and deny immediately - not the scoring approach of CRS.

So all things considered, I believe SecRequestBodyNoFilesLimit is still the superior approach and I can't see it having any disadvantage other than adding a directive to the code. What is the reason it no longer being supported.

[airween]

Hi @martinhsv,

I don't believe the code in this pull request will do anything special like short-circuit the loading-into-memory of the body if the limit is exceeded.

yes, you're right. It's follow the behavior of ModSec2.

I think there are two possible places where we can compare the length of the body with variable NoFilesLimit: when the engine got it (https://github.com/SpiderLabs/ModSecurity/blob/4e9ba44d034c7afdf23bacea948b3b9961f424dc/src/transaction.cc#L990), and when the engine processes the body.

In first case, we have to check the CT header, I don't like this solution.

In my solution I check it when the body processor finished - may be I can move this comparison before it starts - what do you think about it?

Anyway, the sent code is slower than this proposal, but - if I understand correctly - the problem was that it's unusable. If you see this could be helpful, I can rewrite this feature. In other case, I won't work on this anymore.

For example this way to test the length of each argument:

Thanks - anyway, I think the SecArgumentsLimit also can replaceable by
SecRule &ARGS "@gt 1" "id:200007,phase:2,t:length,deny,status:400"

but of course, this can only be verified, when all arguments processed. Your solution is faster.

Further insight, that the used configuration variables SecRequestBodyNoFilesLimit or SecArgumentsLimit can be used independently of the rule set. Checking these values in rule set is a bit complicated for a user.

And just one more note: none of the &ARGS or SecArgumentsLimit can check the related attribute of XML content.

[BasLangenberg]

We ran into this issue and wrote a custom rule to enforce a lower NoFiles Limit for our API endpoints.

    SecRule REQUEST_URI "/api/" \
      "phase:1,\
      id:400900,\
      log,\
      deny,\
      chain"
    SecRule REQUEST_HEADERS:Content-Length "@gt 1048576" "chain"
    SecRule REQUEST_HEADERS:Content-Type "!@rx ^(multipart/form-data|application/x-www-form-urlencoded)$"

This might help out others running into the same issue.

@airween / @martinhsv We still see merit in implementing the SecRequestBodyNoFilesLimit as it would simplify our configuration, and it is less error prone than a custom rule. If you have feedback on our rule, that would be very much appreciated as well!

[airween]

@BasLangenberg after a quick view I think your solution can help to solve the original issue - but it works only if the engine (SecruleEngine) is On. If it is in DetectionOnly, then the payload can overload the engine.

I'm not professional in HTTP/2, but as I know the Content-Length header is not mandatory in that case, so possible this solution can't work then too. May be the checking of &REQUEST_BODY in phase:2 would be better in that case.

Btw: yes, I still think that the implementation of SecRequestBodyNoFilesLimit would be fine.

[martinhsv]

Hi @BasLangenberg ,

I'm fine with revisiting the possibility of implementing SecRequestBodyNoFilesLimit in ModSecurity v3.

In your deployment, what use case(s) would this be helpful with? I.e. what are you using the substitute custom rule to protect against?

(It's quite useful to have particular cases in mind. This is partly to make sure reasonable alternatives don't already exist, but also to ensure that the chosen implementation actual addresses the use case(s) of concern.)