ModSecurity header phase runs before nginx rate limiting

[brandonpayton]

As a user, we would like to skip the cost of ModSecurity rule processing for requests that are rate-limited by nginx. Today, the ModSecurity header phase is processed before the ngx_http_limit_req_module considers rate limiting.

This is due to how each module hooks into the nginx request phases. The ngx_http_limit_req_module enforces rate limiting in the NGX_HTTP_PREACCESS_PHASE, but ModSecurity-nginx processes the ModSecurity header phase earlier, during the NGX_HTTP_REWRITE_PHASE.

Some workarounds are:

• Move all rules to the ModSecurity request body, but this is not a great option when using large, shared rulesets like the OWASP ModSecurity Core Rule Set.
• Make a custom build that runs the ModSecurity request header phase in the PREACCESS phase. This is the same nginx phase used to run the ModSecurity request body phase, but unlike the request body phase, the header phase wouldn't have to wait to receive the request body.

Even with these workarounds, it seems like it might make sense to be able to rate-limit prior to ModSec processing. Is this something we could consider changing or making configurable?

[martinhsv]

Hello @brandonpayton ,

Interesting question.

The nginx phase to which REQUEST_HEADERS is attached has been that way since the inception of ModSecurity-nginx, so I would want to tread cautiously here. It would be interesting to understand more about the logic of the current nginx phase assignments and the potential tradeoffs of any change.

For example, why is the ngx_http_limit_req_module running so late? The documentation states that realip assignments are already done in the NGX_HTTP_POST_READ_PHASE (which is before NGX_HTTP_REWRITE_PHASE), so why does it wait until the NGX_HTTP_PREACCESS_PHASE? And: Are there things we would lose by shifting REQUEST_HEADERS to be later than it currently runs?

It's not clear to me that ngx_http_limit_req_module ought to run before any ModSecurity processing. Both that module and ModSecurity's REQUEST_HEADERS need to have received the request headers (the former presumably because it may need to adjust ip based on headers like X-Forwarded-For). But some ModSecurity rules might (again, conceptually) need less information, (e.g. the URI) to act.

Nevertheless, it may be that REQUEST_HEADERS is currently running earlier than ideal, and it should be changed universally. Or maybe not.

I agree that the workaround suggested in your first bullet point is not ideal. On the surface a quick find-and-replace would do the trick but:
a) you'd have to be careful not to change any rules that must run in phase:1 (for example body parser activations) and
b) you'd lose the advantage of some phase:1 rules being able to short-circuit prior to the cost of processing the request body

[brandonpayton]

Hi @martinhsv,

Thank you for your thoughtful response. I appreciate your time and am sorry for the delay in responding here.

The nginx phase to which REQUEST_HEADERS is attached has been that way since the inception of ModSecurity-nginx, so I would want to tread cautiously here. It would be interesting to understand more about the logic of the current nginx phase assignments and the potential tradeoffs of any change.

Agreed.

For example, why is the ngx_http_limit_req_module running so late? The documentation states that realip assignments are already done in the NGX_HTTP_POST_READ_PHASE (which is before NGX_HTTP_REWRITE_PHASE), so why does it wait until the NGX_HTTP_PREACCESS_PHASE?

Since the limit_conn and limit_req directives can be configured per nginx location, it seems important that actions in the REWRITE phase take place first to influence which location is ultimately the effective location for the request. Only then can ngx_http_limit_conn_module and ngx_http_limit_req_module know what their limit-related settings are.

And: Are there things we would lose by shifting REQUEST_HEADERS to be later than it currently runs?

Not sure, but I agree this is a possibility.

It's not clear to me that ngx_http_limit_req_module ought to run before any ModSecurity processing. Both that module and ModSecurity's REQUEST_HEADERS need to have received the request headers (the former presumably because it may need to adjust ip based on headers like X-Forwarded-For). But some ModSecurity rules might (again, conceptually) need less information, (e.g. the URI) to act.

It's possible ModSecurity users' preferences may vary in this case. For us, it is natural to prefer that ModSecurity only examine requests that aren't already rejected by nginx because, in those cases, we spend less CPU per rejected request.

Would this be a place where it would be good to support another directive or a build-time option that will allow users to express their preference?

Currently, we are simply running a patched build that moves the ModSecurity REQUEST_HEADERS and REQUEST_BODY phases to the nginx ACCESS phase (it turned out that PREACCESS was too early because ModSecurity was still running before the nginx limit-related modules). In addition, even with these custom changes, there are times where ModSecurity continues to run for limited requests so we currently address that with nginx maps and another patch (which I plan to submit soon) that allows ModSecurity-nginx to enabled/disabled dynamically per request.

If ModSecurity-nginx would support a way to say "give nginx request limiting priority", then the relevant logic could also explicitly check relevant nginx request flags and skip processing when nginx-limited.

What do you think?

[EthraZa]

I spent a full day trying to create a ModSec rule that use the rate-limit info and now I find out it will never work because ModSec wants to be the first one to do its stuff.
Come on, if it's not necessary to run in the PREACCESS phase and could be running in the ACCESS phase, why it's still there preventing us to advance with things?

[airween]

Come on, if it's not necessary to run in the PREACCESS phase and could be running in the ACCESS phase, why it's still there preventing us to advance with things?

Any PR which changes this behavior is welcome :).

[tomsommer]

@brandonpayton maybe you can share your patch? or take a look at #361