SecRequestBodyAccess off skips the phase:2 rules

[airween]

Describe the bug

If the SecRequestBodyAccess is off, then the engine skips all rules which has phase:2 action.

To Reproduce

Here you can find a regression test case.

In the debug log you can see this line:

[nnnnnn] [/?q=evil] [4] Request body processing is disabled

Expected behavior

As you can see, the request contains an XML payload (CT doesn't count...), and only one (relevant) rule with id:1. The rule has two variable: REQUEST_URI and REQUEST_BODY. First one available in phase 1, but the second one only in phase 2, so the rule set to phase 2. The request was constructed that both variable matches, but the test returns with 200.

The expected behavior is 403, because the config disables only the processing of request body, not the whole phase 2.

If you replace the phase:2 by phase:1 in rule 1, then the request will blocked as it expected.

Server (please complete the following information):

• ModSecurity version (and connector): libmodsecurity3.0.4, current master afefda

Additional context

There are two another issues related to this wrong behavior:
#1940 - CRS issue - this helped me to describe this behavior
#2273 - ModSecurity - SecRequestBodyAccess Off doesn't fully disable request body processing

I checked the source, and found a very strange solution to handle the SecRequestBodyAccess - looks like the appendBody() allocates a buffer even thought the variable set it to off. In addition, the chosen body processor also woke up, and the payload will parsed. Checking the body access is comes only at this point, that's why the #2273 exists. I assume that users disable the body access to save CPU/memory (as reporter also wrote there), but in this isn't fulfilled.

The another strange behavior is if the SecRequestBodyAccess is off, then whole phase 2 will ignored (in line 895-911), because the evaluation called later, in L938.

Note: I didn't checked, but I suppose the ctl:requestBodyAccess=off would give same result.

[airween]

Sorry to say, but same behavior in case of SecResponseBodyAccess - if it's off, then whole phase:4 ignored.

Test case here. Problem is similar: the function returns by true if the body access is off, the evaluation won't called.

[martinhsv]

Hmm. I guess I can understand why it was coded that way (and some might even challenge whether it's a bug at all).

Given that phase:2 occurs at 'request body has been received', and one does not want to access the request body, it's not immediately obvious why one would write a rule to occur at phase:2. I.e. if your rule is checking request headers, why not make it a phase:1 rule? For example, this would seem to be the case with the cited rule 913100.

On the other hand, I can see at least some cases where this would be a problem -- particularly with generic rule sets.

For example, suppose a generic rule is intended to perform some detection against ARGS (both query args and post args), and so it is set up as a phase:2 rule. But a particular administrator is not interested in examining the request body and so sets SecResponseBodyAccess to off. In this case use of the rule will have been inadvertently turned off for query args as well.

[dune73]

You hit the nail on the head with your reasoning why most rules are written in phase 2. A quick peek at the Trustwave rule set tells me that it's over 99% of the rules. The ratio is better with CRS, but this is a serious problem and I hope you can fix this soon.

[airween]

Thanks for quick response - I just followed the mod_security2 behavior.

While it works as I wrote that expected, I assume the v3 should follow that way - or make a separated documentation, how the new version works.

Anyway, using of phase action is just one thing - the another is the #2273: if the SecRequestBodyAccess is off, why does engine start to parse that?

[marcstern]

if your rule is checking request headers, why not make it a phase:1 rule?
Several reasons:

• with "configure --enable-request-early", phase 1 rules cannot be embedded in Location
• some header rules may depend on request body (for instance the body size)
• etc.

[zimmerle]

Not executing the rules seems to be a little too radical for my taste. On the other hand, I understand the value of not running it for performance reasons and other points cited below. Furthermore, it is easy to maintain the rules by having the execute-ever approach, given that it will allow cross-phases variables in a single Rule.

The cross-phase variables is controversial. Although it facilitates the development of the rules, oftentimes it leads to circumstances where:

• Too late to block: Blocking content from the request_headers during the request_body phase. Modern web servers deliver the headers to the end application before receiving the request body.
• Too late to take Action: Response headers are already delivered to the client, giving no chance to ModSecurity except to drop the request. Despite treating the request gracefully.

Those points aren't issues on Apache, at least last time I checked, but they are issues on Nginx. Owing to it treats the request on this streaming fashion.

The too late to block is the factor that drives me to think towards a situation that ModSecurity should be warning the user that the rule may not be doing what the user hopes for, at least, not in the time that he expects. Together with the effort to support intermediate phases, this could give us a good ground for new features.

Version 2 has some counterpoints on when to execute the phase:1 via the --enable-request-early but I don't think it is related to that particular discussion.

[airween]

Another use case: when the variable is a collection name, eg. TX.

#
# -=[ Anomaly Mode: Overall Transaction Anomaly Score ]=-
#
SecRule TX:ANOMALY_SCORE "@ge %{tx.inbound_anomaly_score_threshold}" \
    "id:949110,\
    phase:2,\
    deny,\
    t:none,\
    msg:'Inbound Anomaly Score Exceeded (Total Score: %{TX.ANOMALY_SCORE})',\
    tag:'application-multi',\
    tag:'language-multi',\
    tag:'platform-multi',\
    tag:'attack-generic',\
    ver:'OWASP_CRS/3.3.0',\
    severity:'CRITICAL',\
    setvar:'tx.inbound_anomaly_score=%{tx.anomaly_score}'"

https://github.com/coreruleset/coreruleset/blob/af619744cbf25bca1d25c3bb27749951a4a4fb4f/rules/REQUEST-949-BLOCKING-EVALUATION.conf#L76-L91

I think this is justified, because the rule counts scores in the REQUEST phases, and it must to wait the end of these phases.

[dune73]

@zimmerle: I think there is nothing wrong with giving the user a way to skip phases completely. It just should not be implicit and undocumented. If the option exists, it should be via a dedicated directive. I also like the idea of ModSecurity warning about rules not doing what people expect. The traditional phase:1 rule with ARGS_POST is silently ignored and that is very taxing on newbies. Personally, I would not mind a 500 in such a situation as the rule is simply broken, but that's a question of taste.

What is not clear to me following your response: How do you intend to handle this? With SecRequestBodyAccess off skipping phase:2, ModSec3 is effectively disabling almost the entire SLR and CRS rule sets.

[zimmerle]

justified, because the rule counts scores in the REQUEST phases, and it must to wait the end of these phases.

The last rule in a phase does not necessarily be the first rule of the subsequent phase. It could be the last in the current phase.

I. SecRule "..." "phase:1,..." 
II. SecRule "..." "phase:1,..." 
III. SecRule "..." "phase:1,actionB..." 

Order-wise this is equivalent -

I. SecRule "..." "phase:1,..." 
II. SecRule "..." "phase:1,..." 
III. SecRule "..." "phase:2,actionA..." 

Notice that in the former you are taking an action when the headers may be already delivered to the end application. In other words: actionB happens a phase before actionA.

[zimmerle]

Some facts were somewhat related to that issue -

The recommended configuration file, with the description of why we think it is quintessential to have Request Body Inspection On:

https://github.com/SpiderLabs/ModSecurity/blob/afefda53c69bb17e2ec16d24dd6fbc2c8fd7d063/modsecurity.conf-recommended#L12-L16

One that may be writing the rules is reading the logs that state -
https://github.com/SpiderLabs/ModSecurity/blob/afefda53c69bb17e2ec16d24dd6fbc2c8fd7d063/src/transaction.cc#L897

The commit which makes the behavior different from v2: 379f370

The issue that propelled the change in question #1531

Reverting 379f370 will make the behavior works is a similar fashion of v2. Not yet recommended.

As a long term plan, we aim to remove those configurations. We could leave for the engine to fill the variables based on the utilization. If not used, there will be automatically disabled.

[dune73]

I do not think you should remove configuration options. But that's for a long term discussion.

Near term: Will you revert 379f370 or how do you plan to fix this until the long term solution pans out?

[dune73]

Any news on this front or a plan to share?

Infos from CRS: We merged a PR (-> 1941) that shifts all the CRS rules that can run in phase:1 into phase:1. This allows ModSecurity v3 users to mitigate this security vulnerability by running the development version of CRS (v3.4/dev) and edit the rule 949110 to run in phase 1. We are now preparing a minor release (likely 3.3.1) that will allow blocking in phase 1 as a configuration option.