Skip to content

Stop segfaults caused by NULL request_body #271

New issue

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

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

Already on GitHub? # to your account

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.

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

Successfully merging this pull request may close these issues.

1 participant