Fix phase 2 and 3 audit logging in case of internal redirect

[mlevogiannis]

Pull request #241 attempted to fix the problem where the log handler was not called in case of internal redirects (e.g. when using error_page). The problem was that the log handler was registered at Nginx log phase, however processing never reached that Nginx phase in case of an internal redirect (the Nginx context of the original request was cleared and a new one was created for the internal redirect). The pull request tackled the issue by introducing the concept of “early logging”, where the process intervention function would manually call the log handler.

However, this early logging mechanism was only enabled for ModSecurity’s phase 1 (request headers). As a result, the original issue was fixed only for that phase but not for ModSecurity’s phases 2 (request body) and 3 (response headers). It did not apply to ModSecurity’s phase 4 (response body), as at this phase it not possible to perform a redirection due to Nginx limitations (see this comment for more information).

This pull request unconditionally enables early logging in the process intervention function for all ModSecurity phases, by removing the respective control flag (what it does is similar to pull request #90, on which pull request #241 is based on). The log handler should always be called in both the process intervention function and the Nginx log phase. The former is the only place in which the log handler is guaranteed to be called in case of an intervention (as demonstrated by this issue) and the latter is required so that it is called when an intervention is not triggered. The "logged" context flag ensures that logging takes place only once.

Finally, the custom error page test case has been updated to include rules of all ModSecurity phases. The tests for ModSecurity phases 2 and 3 fail with the current upstream code and succeed after the proposed fix is applied.

[kolotouros]

Any update on this?

[martinhsv]

@kolotouros : No

[liudongmiao]

@kolotouros This will duplicate log entry, and no extra response header, nor response body.

For error_page, current implementations:

1. request, no response, no log
2. second request, got response, log with response headers and body

In this patch:

1. request, no response, log without headers
2. second request, got response, log without headers

[mlevogiannis]

This pull request is more of a fix to the existing workaround ("early logging") than a proper solution to the issue caused by internal redirects.

Pull request #273 is in the correct direction, it recovers the original request's context which can then be used to log the transaction properly. However in its current state it does not properly mitigate the issue described in this pull request. Specifically, if ModSecurity is not enabled in the internal redirect's location conf, the log handler will not run at all. This is demonstrated by the updated test case included in this pull request.

If pull request #273 is merged (along with the extra changes discussed in the respective thread), this pull request can be closed.

[SWGAKamui]

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

[jeremyjpj0916]

+1, we need to merge the fixes for error_page redirect still with the modsec nginx code base.

[airween]

Hi guys,

it seems like there are some conflicts that must be resolved, so @mlevogiannis please:

• pick up and merge the modifications
• resolve the conflicts
• push the new commits

Then the new CI will start and we can see the tests results too.

It would be nice to do these in next few days, I would like to release a new version, and - perhaps - many user would be happy whit this.

Thanks!

[mlevogiannis]

@airween In my opinion, this branch (mentioned in #273) should be merged instead of this PR (the reasoning is described here. It fixes the issue described in the first post and includes the same test cases. We may create a new PR for it, if that will help.

However, if you insist on merging this PR, I will update it to resolve the conflicts.

[airween]

@airween In my opinion, this branch (mentioned in #273)

you mean the PR #273 instead of this, right?

should be merged instead of this PR (the reasoning is described here. It fixes the issue described in the first post and includes the same test cases. We may create a new PR for it, if that will help.

However, if you insist on merging this PR, I will update it to resolve the conflicts.

No, I'm not insist. But that PR must be updated too.

Thanks.

[sonarcloud[bot]]

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]

@airween In my opinion, this branch (mentioned in #273)

you mean the PR #273 instead of this, right?

No, I mean this branch, which is based on the PR you mention. I have not created a separate PR for it (yet, but I will do so should this be requested) as I hoped that the original author would include the extra commits in their branch, but they only copied part of our changes in a new commit.

should be merged instead of this PR (the reasoning is described here. It fixes the issue described in the first post and includes the same test cases. We may create a new PR for it, if that will help. However, if you insist on merging this PR, I will update it to resolve the conflicts.

No, I'm not insist. But that PR must be updated too.

Updated.