Modsecurity: Don't run request body completion checks when ProcessPartial and the input filter has't seen all of the body

[rainerjung]

Version: 2.9.3

Only run completion checks for request bodies, if we have
actually seen all of the body. In case ProcessPartial is configured
and our input filter only processed part of the request body, checking
for a complete request body makes no sense.

I will provide a pull request.

[dune73]

Thank you Rainer.

[tomsommer]

Relates to https://github.com/SpiderLabs/ModSecurity/issues/1600 https://github.com/SpiderLabs/ModSecurity/issues/1471

[markblackman]

Also relates to #1827

[zimmerle]

Relates to #1600 #1471

Also relates to #1827

All these three issues seem to be a consequence of ModSecurity been holding the filter data whenever the limit threshold is reached. However, in this very issue @rainerjung is not treating this specific problem. He did treat on #2092, therefore I am making a comment there mention the relation with the listed issues.

[zimmerle]

Hi @rainerjung,

Thank you for your patch.

I do have some concerns that, if you don't mind, I would like to discuss and understand your point of view.

SecRequestBodyLimitAction can assume two different values: _Reject_ and _ProcessPartial_. When it is marked to _Reject_ I understand that we may not need to inspect the request body contents. However, in the case of _ProcessPartial_ we need to go over the request data no matter what. Because the user may have configured ModSecurity to validate a JSON, XML, or multipart considering the limit values. In other words: He may be looking for a JSON error to drop a request (or any other action). If we remove that check, we won't be processing the content partially, rather ignoring the content. Therefore not generating the error which is expected by the end user.

What do you think about that?

[zimmerle]

@rainerjung: Waiting for your considerations here.

@tomsommer && @markblackman: in case you are interested, you are invited to discuss as well.

@dune73: it seems that you have agreed with @rainerjung's original idea, what is your opinion considering my comments?

[markblackman]

In my case I don't currently have the requirement to validate a full request body even in the processpartial case, so I am content with the implicit ignore, but I can imagine others will not be. You could also just print a warning to that effect ("ProcessPartial limit exceeded, validation not done") in the logs.

We are already using Rainer's patch to very positive effect and these will go into production systems tomorrow, in fact, so the question is academic for me. We desperately needed these fixes for our use case and we have no interest in validation cases yet.

[rainerjung]

SecRequestBodyLimitAction can assume two different values: _Reject_ and _ProcessPartial_. When it is marked to _Reject_ I understand that we may not need to inspect the request body contents. However, in the case of _ProcessPartial_ we need to go over the request data no matter what. Because the user may have configured ModSecurity to validate a JSON, XML, or multipart considering the limit values. In other words: He may be looking for a JSON error to drop a request (or any other action). If we remove that check, we won't be processing the content partially, rather ignoring the content. Therefore not generating the error which is expected by the end user.

What the patch adds is checking msr->if_seen_eos to decide, whether a failed completion check should be escalated. If !msr->if_seen_eos, the mod:security has not seen all othe the request body. This will be due to ProcessPartial and a request body size that is bigger than the configured limits. In that case, mod_security only buffers a part of the request and lets the remaining bytes through unchecked. Due to the fact, that the buffered request body is incomplete, the checks in ..._complete() will always fail. The buffered part of the request body will not be syntactically valid and the complete() checks will fail. But that is expected. The full request body might be complete, mod_security just can't tell, because it only buffered a part of it.

The patch does only not geneate the error, if the part of the request that mod_security has buffered is not the full request body. Otherwise we should have gotten signalled by the web server, that the end of the request is reached and msr->if_seen_eos will be set by mod_security.

Regards,
Rainer

[tomsommer]

Perhaps a SecRequestBodyLimitAction SkipProcessing to solve this?

[dune73]

@zimmerle: I do not have a qualified opinion here.

[zimmerle]

Hi, @rainerjung

What the patch adds is checking msr->if_seen_eos to decide, whether a failed completion check should be escalated. If !msr->if_seen_eos, the mod:security has not seen all othe the request body. This will be due to ProcessPartial and a request body size that is bigger than the configured limits. In that case, mod_security only buffers a part of the request and lets the remaining bytes through unchecked. Due to the fact, that the buffered request body is incomplete, the checks in ..._complete() will always fail. The buffered part of the request body will not be syntactically valid and the complete() checks will fail. But that is expected. The full request body might be complete, mod_security just can't tell, because it only buffered a part of it.

I understand what the code is doing, I am more concern in the practical outcoming in regarding the semantic of the ProcessPartial. Like @tomsommer suggest I think that is the case for SkipProcessing instead of ProcessPartial. After all, from the user perspective, the validation will be skipped (or ignored). What do you think about changing this patch to actually add the SkipProcessing?

[zimmerle]

Hi, @rainerjung any position on it?

[muradmomani]

@zimmerle
Hi, any update to this ?

[zimmerle]

@zimmerle
Hi, any update to this ?

Waiting for @rainerjung comments

[rainerjung]

Hi, @rainerjung

What the patch adds is checking msr->if_seen_eos to decide, whether a failed completion check should be escalated. If !msr->if_seen_eos, the mod:security has not seen all othe the request body. This will be due to ProcessPartial and a request body size that is bigger than the configured limits. In that case, mod_security only buffers a part of the request and lets the remaining bytes through unchecked. Due to the fact, that the buffered request body is incomplete, the checks in ..._complete() will always fail. The buffered part of the request body will not be syntactically valid and the complete() checks will fail. But that is expected. The full request body might be complete, mod_security just can't tell, because it only buffered a part of it.

I understand what the code is doing, I am more concern in the practical outcoming in regarding the semantic of the ProcessPartial. Like @tomsommer suggest I think that is the case for SkipProcessing instead of ProcessPartial. After all, from the user perspective, the validation will be skipped (or ignored). What do you think about changing this patch to actually add the SkipProcessing?

This issue was opened only for one purpose as the title says "Don't run request body completion checks when ProcessPartial and the input filter has't seen all of the body". The only behavioral change that I am looking for, is that mod_security does not claim a syntactically incorrect end of the request body if it didn't actually look at it.
Do you agree that this is a useful goal?
If so, do you see a reason, why the proposed patch does not exactly do that?
In this ticket I am not looking for any other behavioral changes.
Regards,
Rainer

[zimmerle]

Hi, @rainerjung

What the patch adds is checking msr->if_seen_eos to decide, whether a failed completion check should be escalated. If !msr->if_seen_eos, the mod:security has not seen all othe the request body. This will be due to ProcessPartial and a request body size that is bigger than the configured limits. In that case, mod_security only buffers a part of the request and lets the remaining bytes through unchecked. Due to the fact, that the buffered request body is incomplete, the checks in ..._complete() will always fail. The buffered part of the request body will not be syntactically valid and the complete() checks will fail. But that is expected. The full request body might be complete, mod_security just can't tell, because it only buffered a part of it.

I understand what the code is doing, I am more concern in the practical outcoming in regarding the semantic of the ProcessPartial. Like @tomsommer suggest I think that is the case for SkipProcessing instead of ProcessPartial. After all, from the user perspective, the validation will be skipped (or ignored). What do you think about changing this patch to actually add the SkipProcessing?

This issue was opened only for one purpose as the title says "Don't run request body completion checks when ProcessPartial and the input filter has't seen all of the body". The only behavioral change that I am looking for, is that mod_security does not claim a syntactically incorrect end of the request body if it didn't actually look at it.
Do you agree that this is a useful goal?
If so, do you see a reason, why the proposed patch does not exactly do that?
In this ticket I am not looking for any other behavioral changes.
Regards,
Rainer

what is your thinking about having SkipProcessing, as pointed here: https://github.com/SpiderLabs/ModSecurity/issues/2093#issuecomment-502791569 ?

[muradmomani]

@zimmerle
Hi,
Why introduce the SkipProcessing while we can make a rule for that specific case to be skipped?
I'm not sure but I think the best solution is to edit the behavior as outlined in this issue header

[zimmerle]

@zimmerle
Hi,
Why introduce the SkipProcessing while we can make a rule for that specific case to be skipped?
I'm not sure but I think the best solution is to edit the behavior as outlined in this issue header

The name ProcessPartial, as already stated, suggests that the content will be partially processed and therefore flagged as such. I understand that there is a use case to process partially and not flag as not inspected; that would be SkipProcessing or ProcessSkip. Don't you agree?