recovery context after internal redirect

[liudongmiao]

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.

[liudongmiao]

@martinhsv could you help to review for this?

[liudongmiao]

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

#241 doesn't resolve the problem too.

[AirisX]

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.

[liudongmiao]

@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

[mlevogiannis]

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.

[SWGAKamui]

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

[liudongmiao]

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

[lmq1999]

This commit should be merge, I having same prob

[jessp01]

Hi all,

Just wondering where things stand with this?

Thanks,

[airween]

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.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[mlevogiannis]

I have rebased onto the latest master and updated the branch in our repository so that the newly introduced CI workflow tests pass. As a recap, our branch:

• Includes the original commit of this PR, which properly recovers the module's context in case of an internal redirect.
• Uses the recovered context to decide whether ModSecurity is enabled or not, which properly fixes Fix phase 2 and 3 audit logging in case of internal redirect #255.
• Adds test cases that cover the issues described in Fix phase 2 and 3 audit logging in case of internal redirect #255.
• Removes the partial "early logging" workaround, which is no longer required.

@liudongmiao Can you please update this PR's branch with the changes in our branch? You may either cherry-pick the 3 extra commits or "git reset" your branch with our branch and force push it.

[liudongmiao]

@mlevogiannis The original patch should be very easy to understand, as it will get the context from clean handler if the context is reset. And there is reference from realip module.

[jeremyjpj0916]

Definitely need to see the error_page issues with ModSecurity nginx resolved after 2 years of it being in limbo and all of us running patch files to achieve functionality that should be baked into the main repo for all to benefit.

[lmq1999]

should this problem need to solve to main branches ?

[airween]

Please read this comment. I need some help to reproduce the original issue.

[liudongmiao]

merged in #326