From 5b02ab073e82fd2d7d7341c034c033242eb5c2e0 Mon Sep 17 00:00:00 2001 From: Sergei Turchanov Date: Wed, 18 Apr 2018 23:27:33 +1000 Subject: [PATCH 1/2] Fix incorrect handling of request/response body data chain of ngx_buf_t buffers. The documentation [http://nginx.org/en/docs/dev/development_guide.html#buffer] clearly states that .pos, .last must be used to reference actual data contained by the buffer. Whereas .start, .end denote the boundaries of the memory block allocated for the buffer (in case of dynamically allocated data) or just NULL (when .pos, .last reference a static memory location - one can see that kind of usage in ngx_http_gzip_filter_module.c:ngx_http_gzip_filter_gzheader()). To back up my words I invite to examine ngx_http_charset_filter_module.c:ngx_http_charset_recode() as an example of iteration over data contained in data buffer. Without this fix ngx_http_modsecurity_body_filter feeds random bytes from memory pointed by .start, .end range to msc_append_response_body. In my case is was 8KB of data instead of 10 bytes when referenced by (.pos, .last). That is this vulnerability may disclose sensitive data like passwords or whatever from nginx heap. The fix for ngx_http_modsecurity_pre_access_handler is to use .pos not .start to reference data as they may differ in general case. --- src/ngx_http_modsecurity_body_filter.c | 4 ++-- src/ngx_http_modsecurity_pre_access.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index f8b3c71..edf44f6 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -150,9 +150,9 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) for (chain = in; chain != NULL; chain = chain->next) { - u_char *data = chain->buf->start; + u_char *data = chain->buf->pos; - msc_append_response_body(ctx->modsec_transaction, data, chain->buf->end - data); + msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { return ngx_http_filter_finalize_request(r, diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c index 70f9feb..6f4cbcb 100644 --- a/src/ngx_http_modsecurity_pre_access.c +++ b/src/ngx_http_modsecurity_pre_access.c @@ -163,10 +163,10 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) while (chain && !already_inspected) { - u_char *data = chain->buf->start; + u_char *data = chain->buf->pos; msc_append_request_body(ctx->modsec_transaction, data, - chain->buf->last - chain->buf->pos); + chain->buf->last - data); if (chain->buf->last_buf) { break; From f238283f4a1356b53ebc9f1ff0157a2dc518bcfe Mon Sep 17 00:00:00 2001 From: Sergei Turchanov Date: Thu, 19 Apr 2018 21:20:23 +1000 Subject: [PATCH 2/2] A body filter function (ngx_http_modsecurity_body_filter in our case) can be called by Nginx several times during request processing. And each time with it own unique set of chained buf pointers. For example, suppose a complete response consists of this chain of data: A->B->C->D->E Ngix may (and actually does, as verified by me in gdb) call body filter two times like this: handler(r, in = A->B->C) handler(r, in = D->E), E has last_buf set Current implementation delays feeding chain->buf to msc_append_response_body until it comes upon a chain with buf->last_buf set. So we loose chain containing A->B->C sequence. We must process body bufs as soon as we see them in body handler otherwise we will not see them again. N.B. You have PR #84 pending. It goes further and fixes the problem when a blocking decision is made after headers were sent. I intentionally retained current (buggy) behavior to make my patch less intrusive and easier to review. Besides #84 impose an excessive memory usage due to a complete copy of all bufs passed through body filter (we have sometimes 500K and more replies in our applications) - I will elaborate on this in code review for #84. --- src/ngx_http_modsecurity_body_filter.c | 55 ++++++++++++-------------- 1 file changed, 25 insertions(+), 30 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index edf44f6..fd286d0 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -35,7 +35,6 @@ ngx_http_modsecurity_body_filter_init(void) ngx_int_t ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) { - int buffer_fully_loadead = 0; ngx_chain_t *chain = in; ngx_http_modsecurity_ctx_t *ctx = NULL; #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS) @@ -135,47 +134,43 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) } #endif + int is_request_processed = 0; for (; chain != NULL; chain = chain->next) { -/* XXX: chain->buf->last_buf || chain->buf->last_in_chain */ - if (chain->buf->last_buf) { - buffer_fully_loadead = 1; + u_char *data = chain->buf->pos; + int ret; + + msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + if (ret > 0) { + return ngx_http_filter_finalize_request(r, + &ngx_http_modsecurity_module, ret); } - } - if (buffer_fully_loadead == 1) - { - int ret; - ngx_pool_t *old_pool; +/* XXX: chain->buf->last_buf || chain->buf->last_in_chain */ + is_request_processed = chain->buf->last_buf; - for (chain = in; chain != NULL; chain = chain->next) - { - u_char *data = chain->buf->pos; + if (is_request_processed) { + ngx_pool_t *old_pool; + + old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); + msc_process_response_body(ctx->modsec_transaction); + ngx_http_modsecurity_pcre_malloc_done(old_pool); - msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data); +/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's + XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */ ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { - return ngx_http_filter_finalize_request(r, - &ngx_http_modsecurity_module, ret); + return ret; } - } - - old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); - msc_process_response_body(ctx->modsec_transaction); - ngx_http_modsecurity_pcre_malloc_done(old_pool); + else if (ret < 0) { + return ngx_http_filter_finalize_request(r, + &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); -/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's - XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */ - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); - if (ret > 0) { - return ret; - } - else if (ret < 0) { - return ngx_http_filter_finalize_request(r, - &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); + } } } - else + if (!is_request_processed) { dd("buffer was not fully loaded! ctx: %p", ctx); }