search

owasp-modsecurity / ModSecurity-nginx

ModSecurity v3 Nginx Connector
Apache License 2.0
1.49k stars 277 forks source link

Stop segfaults caused by NULL request_body #271

Open brandonpayton opened 2 years ago

brandonpayton commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.