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

Stop segfaults caused by NULL request_body #271

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

Conversation

brandonpayton
Copy link
Contributor

ModSecurity-nginx assumes ngx_http_request_t.request_body is never NULL and encounters a segfault when the request_body is in fact NULL.

We have seen this happen when ModSecurity-nginx is used in conjunction with lua-nginx-module. When a subrequest is made using this lua API, it can result in the request_body being set to NULL here.

I considered whether this was more of a bug with lua-nginx-module, but nginx's codebase appears to recognize a NULL request_body is possible (some examples: a, b, c). Also, it seems reasonable for ModSecurity-nginx to be a little more defensive in this case.

This is a patch to fix that issue by wrapping the code that process the request_body in an if-NULL check. With this patch, msc_append_request_body() will not be called when the request_body is NULL, and this seems like it will be OK because ModSecurity's Transaction's m_requestBody is a std::ostringstream that will simply not have any data.

@brandonpayton
Copy link
Contributor Author

In the case of this patch, viewing with ignoring whitespace changes emphasizes the simplicity of this change:
https://github.com/SpiderLabs/ModSecurity-nginx/pull/271/files?w=1

On another note, I just realized that nginx checks r->request_body->bufs for NULL when checking request_body, so I plan to add that the same sort of check to this PR when back at my test machine.

@brandonpayton
Copy link
Contributor Author

brandonpayton commented Mar 4, 2022

Unfortunately, reproducing the segfault appears to require building nginx with a module that sets request_body to NULL and triggering it, but perhaps it would be sufficient to observe that making sure request_body is not NULL does not break anything.

On my test machine, I've built nginx with ModSecurity-nginx and lua-nginx-module and added a location to the nginx config that uses Lua to trigger setting request_body to NULL. With that setup, I am able to reproduce the segfault and confirm that, with this patch, the segfault no longer occurs.

@brandonpayton
Copy link
Contributor Author

brandonpayton commented Mar 5, 2022

On another note, I just realized that nginx checks r->request_body->bufs for NULL when checking request_body, so I plan to add that the same sort of check to this PR when back at my test machine.

Actually, checking r->request_body->bufs does not appear to be strictly needed because r->request_body->bufs is assigned to chain here, and chain is not used without checking it first here.

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.

1 participant