ModSecurity-nginx recovery context after internal redirect

In error_page, nginx would reset context. Then modsecurity-nginx cannot recovery from previous context.

It will act like this:

phase 0 (connection)
phase 1 (request headers)
phase 2 (request body)

Then, like a new request: (request method set to GET)

phase 0 (connection)
phase 1 (request headers)
phase 2 (request body)
... other ... phases

In previous, r->error_page was introduced to solve the log problem. However, it doesn't solve the root cause.

This PR should close #270, #255 and other error_page (internal redirect) issues.

Mar 11 '22 19:03 liudongmiao

@martinhsv could you help to review for this?

Mar 19 '22 00:03 liudongmiao

@zimmerle @AirisX @defanator Could you help to check for this?

#241 doesn't resolve the problem too.

Mar 20 '22 05:03 liudongmiao

Hello @liudongmiao, Could you provide some test cases (e.g. with ModSecurity defined on different contexts: server, location with analysing the transaction log content)? I believe that this approach (context recovering) is something right way.

Apr 12 '22 13:04 AirisX

@AirisX I think https://github.com/SpiderLabs/ModSecurity-nginx/blob/master/tests/modsecurity-config-custom-error-page.t should be ok. However, the test case doesn't check the created transaction in log. And if the request method is non-HEAD, it would be changed to GET.

In my test case, without this patch, there would be two transaction. If you don't use custom erorr_page, you can ignore this.

For erorr_page, nginx would call ngx_http_send_error_page in src/http/ngx_http_special_response.c, then nginx would call ngx_memzero in ngx_http_internal_redirect and ngx_http_named_location (for named location).

You can debug with IDE like Clion, or read http://nginx.org/en/docs/dev/development_guide.html#http_request_redirection

ngx_http_internal_redirect would clear context, then start from phase NGX_HTTP_SERVER_REWRITE_PHASE
ngx_http_named_location would clear context, then start from phase NGX_HTTP_REWRITE_PHASE

All nginx phases:

NGX_HTTP_POST_READ_PHASE = 0,
NGX_HTTP_SERVER_REWRITE_PHASE,
NGX_HTTP_FIND_CONFIG_PHASE,
NGX_HTTP_REWRITE_PHASE, <-- modsecurity hook, create new transaction if cannot get from context
NGX_HTTP_POST_REWRITE_PHASE,
NGX_HTTP_PREACCESS_PHASE, <-- modsecurity hook for request body
NGX_HTTP_ACCESS_PHASE,
NGX_HTTP_POST_ACCESS_PHASE,
NGX_HTTP_PRECONTENT_PHASE,
NGX_HTTP_CONTENT_PHASE,
NGX_HTTP_LOG_PHASE <-- modsecurity hook for log

Apr 13 '22 05:04 liudongmiao

We also agree that this pull request is in the right direction, however it does not directly close pull request #255, contrary to what is stated in the first comment. Specifically, the log handler will only run if ModSecurity is also enabled in the internal redirect's location conf, but not if it is not (with the exception of phase 1 rules where the log handler runs because of the "early logging" workaround). This is demonstrated by the updated test case included in pull request #255 (the current test case does not cover the aforementioned problematic cases).

The problem is that the connector decides whether ModSecurity is enabled or not based on the configuration directive in the request's location conf. At the time the log handler runs, the original request has already been replaced by a new one with the internal redirect's location conf. As a result, if ModSecurity is not enabled in the internal redirect's location conf, the check at line 47 will fail and the original transaction will not be logged.

What is required is to make this decision based on the original request, in which case the recovered context should be used instead of the location conf. There are two possible options here:

Create a new context in the rewrite handler regardless of the configuration directive in the location conf and use an extra flag in the context to mark whether ModSecurity is enabled or not. This flag will then be checked by the pre access handler, the header filter, the body filter and the log handler after the original context has been recovered.
Extend the current behavior of the header and body filters, which assume that a null context means that ModSecurity is disabled, to the pre access and log handlers. This way the log handler will run regardless of ModSecurity being enabled or not in the internal redirect's location conf.

As a PoC, the second option was implemented in this branch. These changes can either be included in this pull request, or we can open a new one.

May 19 '22 13:05 mlevogiannis

Is there any news please? It seems that those commits helps my need. Thanks to the author for your contribution. :)

Aug 16 '22 08:08 SWGAKamui

@mlevogiannis I have checked your commit, how to merge it into this PR?

Aug 18 '22 03:08 liudongmiao

This commit should be merge, I having same prob

Oct 25 '22 04:10 lmq1999

Hi all,

Just wondering where things stand with this?

Thanks,

Mar 08 '24 11:03 jessp01

To all: sorry for the delay, the new team takes care with sources since end of January, 2024. We weren't able to review all PR's, but now I try to take a look here too.

Thanks and sorry again.