nginx crash observed on nginx 1.4.5 and ModSecurity 2.7.7

[akamatgi]

CentOS release 6.5 (Final) 2.6.32-431.el6.x86_64 #1 SMP

I have ModSecurity turned on with "SecRuleEngine DetectionOnly" set in modsecurity.conf
I also have enabled Caching in nginx.conf
When I am sending a request with a sql injection pattern in the URL, I am seeing that nginx crashes with Segmentation Violation with the following backtrace:

(gdb) bt
#0 ngx_http_write_filter (r=0x1f467c0, in=0x1f4a778) at src/http/ngx_http_write_filter_module.c:121
#1 0x0000000000464b8d in ngx_http_chunked_body_filter (r=0x1f467c0, in=)

at src/http/modules/ngx_http_chunked_filter_module.c:111

#2 0x0000000000476067 in ngx_http_spdy_body_filter (r=0x1f467c0, in=0x1f4a778) at src/http/ngx_http_spdy_filter_module.c:618
#3 0x000000000046a4a5 in ngx_http_gzip_body_filter (r=0x1f467c0, in=0x1f4a778) at src/http/modules/ngx_http_gzip_filter_module.c:325
#4 0x000000000046b3ea in ngx_http_postpone_filter (r=0x1f467c0, in=0x1f4a778) at src/http/ngx_http_postpone_filter_module.c:83
#5 0x000000000046bb49 in ngx_http_ssi_body_filter (r=0x1f467c0, in=)

at src/http/modules/ngx_http_ssi_filter_module.c:396

#6 0x000000000046ffbf in ngx_http_charset_body_filter (r=0x1f467c0, in=)

at src/http/modules/ngx_http_charset_filter_module.c:553

#7 0x00000000004914cb in ngx_http_modsecurity_body_filter (r=, in=)

at .../modsecurity-apache_2.7.7/nginx/modsecurity/ngx_http_modsecurity.c:1254

#8 0x000000000041de58 in ngx_output_chain (ctx=0x1f4a5c0, in=0x7fffa65158e0) at src/core/ngx_output_chain.c:66
#9 0x000000000045327f in ngx_http_copy_filter (r=0x1f467c0, in=0x7fffa65158e0) at src/http/ngx_http_copy_filter_module.c:143
#10 0x0000000000464f1c in ngx_http_range_body_filter (r=0x1f467c0, in=0x7fffa65158e0)

at src/http/modules/ngx_http_range_filter_module.c:587

#11 0x00000000004479e2 in ngx_http_output_filter (r=0x1f467c0, in=0x7fffa65158e0) at src/http/ngx_http_core_module.c:1956
#12 0x0000000000468fe4 in ngx_http_cache_send (r=0x1f467c0) at src/http/ngx_http_file_cache.c:1022
#13 0x000000000045de17 in ngx_http_upstream_cache_send (r=0x1f467c0, u=0x1f49978) at src/http/ngx_http_upstream.c:868
#14 0x00000000004607ae in ngx_http_upstream_cache (r=0x1f467c0) at src/http/ngx_http_upstream.c:774
#15 ngx_http_upstream_init_request (r=0x1f467c0) at src/http/ngx_http_upstream.c:493
#16 0x0000000000460ecf in ngx_http_upstream_init (r=0x1f467c0) at src/http/ngx_http_upstream.c:466
#17 0x00000000004567de in ngx_http_read_client_request_body (r=0x1f467c0, post_handler=0x460dba <ngx_http_upstream_init>)

at src/http/ngx_http_request_body.c:84

#18 0x0000000000482bf0 in ngx_http_proxy_handler (r=0x1f467c0) at src/http/modules/ngx_http_proxy_module.c:725
#19 0x0000000000448e5c in ngx_http_core_content_phase (r=0x1f467c0, ph=0x27daf98) at src/http/ngx_http_core_module.c:1408
#20 0x0000000000443723 in ngx_http_core_run_phases (r=0x1f467c0) at src/http/ngx_http_core_module.c:888
#21 0x0000000000443833 in ngx_http_handler (r=) at src/http/ngx_http_core_module.c:871
#22 0x000000000044e841 in ngx_http_process_request (r=0x1f467c0) at src/http/ngx_http_request.c:1828

After digging around the code, I found a potential issue in the function move_brigade_to_chain() in the following piece of code:
if (APR_BUCKET_IS_EOS(e)) {
if (cl == NULL) {
cl = ngx_alloc_chain_link(pool);
if (cl == NULL) {
break;
}

            cl->buf = ngx_calloc_buf(pool);
            if (cl->buf == NULL) {
                break;
            }

            cl->buf->last_buf = 1;
            *ll = cl;
        } else {
            cl->buf->last_buf = 1;
        }
        apr_brigade_cleanup(bb);
        return NGX_OK;
    }

Adding 'cl->next = NULL;' between the lines 'cl->buf->last_buf = 1;' and '*ll = cl;' fixed the issue for me.
Is this a valid fix?

[akamatgi]

A brief note on why I think the issue is in move_brigade_to_chain():
The crash was triggered because the ngx_chain passed to ngx_http_write_filter() was having a garbage ngx_buf in the chain.
With no other modules enabled in nginx, the ngx_chain is the output of ngx_http_modsecurity_body_filter, which in turn is created by move_brigade_to_chain().
As mentioned in the comment above, the next pointer of the "last" ngx_chain created inside move_brigade_to_chain() was not explicitly setting the next pointer to NULL, which could have led to the crash.

[p0pr0ck5]

akamatgi, I was having a very similar issue and have been troubleshooting this (on and off) for several months. 'cl->next = NULL;' does indeed seem to have resolved my segfault issue (I had also experienced on debian based systems). I'll be testing this over the next few days and will update this after testing the full CRS spectrum, but this seems promising! Let me know when I can send you a beer! ;)

[akamatgi]

Robert, glad to know it fixed your issue( atleast for now ).

On Wed, Apr 16, 2014 at 5:22 AM, Robert notifications@github.com wrote:

akamatgi, I was having a very similar issue and have been troubleshooting
this (on and off) for several months. 'cl->next = NULL;' does indeed seem
to have resolved my segfault issue (I had also experienced on debian based
systems). I'll be testing this over the next few days and will update this
after testing the full CRS spectrum, but this seems promising! Let me know
when I can send you a beer! ;)

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/681#issuecomment-40548520
.

[zimmerle]

Hi @akamatgi,

Did you managed to run the regression tests with your patch applied? Do you want to share it in a format of a pull request?
https://help.github.com/articles/creating-a-pull-request

Thanks,
F.

[kyprizel]

As I can see - it fixed the problem for me.
#722

[zimmerle]

Hi @kyprizel thank you for the patch. We already have 'cl->next = NULL' in the branch:

https://github.com/SpiderLabs/ModSecurity/tree/nginx_refactoring

nginx_refactoring branch is currently under testing. I will merge the b3db7d3 under this testing branch by the end of the day.

Thanks,
F.

[kyprizel]

Thanks, please, do not merge b3db7d3 - it's not a good solution and can crash process in some cases.

[akamatgi]

Hi Felipe,
Sorry for the late response.
no, I did not run the regression tests. This was detected by our QA and it
intended as a point fix.
I dont think the QA ran the regression tests either.
Thanks,
-anirudh

On Wed, Apr 16, 2014 at 7:14 PM, Felipe Zimmerle
notifications@github.comwrote:

Hi @akamatgi https://github.com/akamatgi,

Did you managed to run the regression tests with your patch applied? Do
you want to share it in a format of a pull request?
https://help.github.com/articles/creating-a-pull-request

Thanks,
F.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/681#issuecomment-40599929
.