Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mlevogiannis
Copy link

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
Copy link

Any update on this?

@martinhsv
Copy link
Contributor

@kolotouros : No

@liudongmiao
Copy link

@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 mlevogiannis force-pushed the bugfix/phase-2-3-audit-log-internal-redirect branch from a6b32c3 to de8fde0 Compare May 19, 2022 13:08
@mlevogiannis
Copy link
Author

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
Copy link

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

@mlevogiannis mlevogiannis force-pushed the bugfix/phase-2-3-audit-log-internal-redirect branch from de8fde0 to 89c8d3b Compare September 1, 2022 14:51
@jeremyjpj0916
Copy link

jeremyjpj0916 commented May 30, 2024

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

@airween
Copy link
Member

airween commented May 31, 2024

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
Copy link
Author

@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
Copy link
Member

airween commented Jun 1, 2024

@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.

mlevogiannis and others added 2 commits June 3, 2024 09:58
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 mlevogiannis force-pushed the bugfix/phase-2-3-audit-log-internal-redirect branch from 89c8d3b to 02249da Compare June 3, 2024 07:01
Copy link

sonarqubecloud bot commented Jun 3, 2024

Quality Gate Passed 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
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants