Fix phase 2 and 3 audit logging in case of internal redirect #255
Conversation
|
Any update on this? |
|
@kolotouros : No |
|
@kolotouros This will duplicate log entry, and no extra response header, nor response body. For
In this patch:
|
mlevogiannis
force-pushed
the
bugfix/phase-2-3-audit-log-internal-redirect
branch
from
a6b32c3
to
de8fde0
Compare
|
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. |
|
Is there any news please? |
mlevogiannis
force-pushed
the
bugfix/phase-2-3-audit-log-internal-redirect
branch
from
de8fde0
to
89c8d3b
Compare
|
+1, we need to merge the fixes for error_page redirect still with the modsec nginx code base. |
|
Hi guys, it seems like there are some conflicts that must be resolved, so @mlevogiannis please:
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! |
|
@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. |
you mean the PR #273 instead of this, right?
No, I'm not insist. But that PR must be updated too. Thanks. |
Phase 2 and 3 log entries were not logged in the audit log in case of an internal redirect. Only phase 1 and 4 ones were logged, the former because early logging was explicitly enabled and the latter because the internal redirect does not work in this phase. This commit unconditionally enables early logging in all ModSecurity phases. Since the Nginx log phase may not be executed in case of an intervention, the process intervention function is the only place which is guaranteed to call the log handler in such a case. Co-authored-by: Dimitris Kolotouros <[email protected]> Co-authored-by: Thanos Giannopoulos <[email protected]>
mlevogiannis
force-pushed
the
bugfix/phase-2-3-audit-log-internal-redirect
branch
from
89c8d3b
to
02249da
Compare
|
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.
Updated. |
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.