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

Clear original response code in send_error_bucket function #2850

Open
wants to merge 1 commit into
base: v2/master
Choose a base branch
from

Conversation

TomasKorbar
Copy link

If this is left intact, then apache thinks that this code was generated during processing of ErrorDocument and does not handle it properly

Fix #2849

If this is left intact, then apache thinks that this code
was generated during processing of ErrorDocument and does not
handle it properly

Fix owasp-modsecurity#2849
@TomasKorbar
Copy link
Author

Could this be merged? I can provide further help if needed.

@marcstern
Copy link

FYI: We're using it on prod for 2 months on several huge sites

@martinhsv
Copy link
Contributor

I haven't had opportunity to do a detailed analysis of this.

It looks simple, but this is the type of change that could potentially have unintended consequences -- perhaps only for some configurations and/or use cases.

Some questions that spring to mind immediately:

  • why is the new line of code setting status to a hard-coded 200, when the assignment to status_line (just above the new code) has been set using the status value passed in to the function?
  • is 200 really always the correct setting here? What if the original response was some other non-error response code like 301?

@TomasKorbar
Copy link
Author

I haven't had opportunity to do a detailed analysis of this.

It looks simple, but this is the type of change that could potentially have unintended consequences -- perhaps only for some configurations and/or use cases.

Some questions that spring to mind immediately:

* why is the new line of code setting status to a hard-coded 200, when the assignment to status_line (just above the new code) has been set using the status value passed in to the function?

I admit it seemed strange to me as well but fortunately i described the breaking point in #2849, so let me clarify.
There is a code in Apache httpd which assumes that when ap_die function is called and request record contains status code other than 200, then this error code was encountered while trying to access custom response URL. Setting response code to 200 here has in fact no effect on the final response because just after the check for recursive error, original response code gets overwritten by the output of filter.

r->status = type;
* is 200 really always the correct setting here? What if the original response was some other non-error response code like 301?

I think so, because it looks to me that send_error_bucket is used only when mod_security wants to change status code of the response and if the original status code, so it does not matter what the old response code was.

Please correct me if i am wrong.

@martinhsv martinhsv added the 2.x Related to ModSecurity version 2.x label Apr 20, 2023
@TomasKorbar
Copy link
Author

Hi @martinhsv ,
Just wanted to check in on my pull request - any updates or plans to merge it? Thanks!
Please let me know if there are further actions required from my side.

@martinhsv
Copy link
Contributor

Hi @TomasKorbar ,

No imminent plans. As I suggested previously, I'd need to gain higher confidence of the correctness of this change before merging. In the meantime, the PR is available for anyone who wishes to merge in their own environment to make use of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Related to ModSecurity version 2.x Platform - Apache
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants