From 2dc1f7d9e7a1ec33d11a74aebf0e51dcd00b536d Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 19 Dec 2017 11:21:16 +0400 Subject: [PATCH 01/14] fix response body filtering --- src/ngx_http_modsecurity_body_filter.c | 108 ++++++++++++++--------- src/ngx_http_modsecurity_common.h | 5 +- src/ngx_http_modsecurity_header_filter.c | 18 ++-- src/ngx_http_modsecurity_module.c | 3 +- 4 files changed, 86 insertions(+), 48 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index f8b3c71..04d84de 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -31,13 +31,15 @@ ngx_http_modsecurity_body_filter_init(void) return NGX_OK; } - 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; + ngx_chain_t *chain = in; + ngx_int_t ret; + ngx_pool_t *old_pool; + ngx_int_t is_request_processed = 0; #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS) ngx_http_modsecurity_conf_t *loc_cf = NULL; ngx_list_part_t *part = &r->headers_out.headers.part; @@ -49,11 +51,11 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) return ngx_http_next_body_filter(r, in); } + /* get context for request */ ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module); - dd("body filter, recovering ctx: %p", ctx); - - if (ctx == NULL) { + + if (ctx == NULL || r->filter_finalize) { return ngx_http_next_body_filter(r, in); } @@ -130,56 +132,82 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) { dd("%d header(s) were not inspected by ModSecurity, so exiting", worth_to_fail); return ngx_http_filter_finalize_request(r, - &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); + &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); } } #endif - 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; + for (chain = in; chain != NULL; chain = chain->next) { + + ngx_buf_t *copy_buf; + ngx_chain_t* copy_chain; + is_request_processed = chain->buf->last_buf; + ngx_int_t data_size = chain->buf->end - chain->buf->start; + ngx_int_t data_offset = chain->buf->pos - chain->buf->start; + u_char *data = chain->buf->start; + msc_append_response_body(ctx->modsec_transaction, data, + chain->buf->end - 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; - - for (chain = in; chain != NULL; chain = chain->next) - { - u_char *data = chain->buf->start; + copy_chain = ngx_alloc_chain_link(r->pool); + if (copy_chain == NULL) { + return NGX_ERROR; + } - msc_append_response_body(ctx->modsec_transaction, data, chain->buf->end - 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); + copy_buf = ngx_calloc_buf(r->pool); + if (copy_buf == NULL) { + return NGX_ERROR; + } + copy_buf->start = ngx_pcalloc(r->pool, data_size); + if (copy_buf->start == NULL) { + return NGX_ERROR; + } + ngx_memcpy(copy_buf->start, chain->buf->start, data_size); + copy_buf->pos = copy_buf->start + data_offset; + copy_buf->end = copy_buf->start + data_size; + copy_buf->last = copy_buf->pos + ngx_buf_size(chain->buf); + copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0; + copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0; + copy_chain->buf = copy_buf; + copy_chain->buf->last_buf = 1; + copy_chain->next = NULL; + chain->buf->pos = chain->buf->last; + + if (ctx->temp_chain == NULL) { + ctx->temp_chain = copy_chain; + } else { + if (ctx->current_chain == NULL) { + ctx->temp_chain->next = copy_chain; + ctx->temp_chain->buf->last_buf = 0; + } else { + ctx->current_chain->next = copy_chain; + ctx->current_chain->buf->last_buf = 0; } + ctx->current_chain = copy_chain; } + } + if (is_request_processed) { 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); - -/* 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); + &ngx_http_modsecurity_module, ret); + } else if (ret < 0) { + return ngx_http_filter_finalize_request(r, + &ngx_http_modsecurity_module, ret); } + ctx->response_body_filtered = 1; + ctx->header_pt(r); + return ngx_http_next_body_filter(r, ctx->temp_chain); + } else { + return NGX_AGAIN; } - else - { - dd("buffer was not fully loaded! ctx: %p", ctx); - } - -/* XXX: xflt_filter() -- return NGX_OK here */ - return ngx_http_next_body_filter(r, in); } diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index f2a4aa8..c3888e1 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -31,7 +31,6 @@ typedef struct { ngx_str_t value; } ngx_http_modsecurity_header_t; - typedef struct { ngx_http_request_t *r; Transaction *modsec_transaction; @@ -52,6 +51,10 @@ typedef struct { unsigned waiting_more_body:1; unsigned body_requested:1; unsigned processed:1; + ngx_http_output_header_filter_pt header_pt; + ngx_chain_t* temp_chain; + ngx_chain_t* current_chain; + unsigned response_body_filtered:1; } ngx_http_modsecurity_ctx_t; diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 7728d82..400a9d6 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -542,10 +542,16 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) * */ - /* - * The line below is commented to make the spdy test to work - */ - //r->headers_out.content_length_n = -1; - - return ngx_http_next_header_filter(r); + /* + * The line below is commented to make the spdy test to work + */ + //r->headers_out.content_length_n = -1; + + if (ctx->response_body_filtered){ + return ngx_http_next_header_filter(r); + } + else { + ctx->header_pt = ngx_http_next_header_filter; + return NGX_AGAIN; + } } diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index 6d6ee7a..c034439 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -247,7 +247,8 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r) ctx->modsec_transaction = msc_new_transaction(cf->modsec, loc_cf->rules_set, r->connection->log); dd("transaction created"); - + + ctx->response_body_filtered = 0; ngx_http_set_ctx(r, ctx, ngx_http_modsecurity_module); cln = ngx_pool_cleanup_add(r->pool, sizeof(ngx_http_modsecurity_ctx_t)); From 59f25736bd5fb5f7d2f1081b58fafce4686e0531 Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 19 Dec 2017 16:50:07 +0400 Subject: [PATCH 02/14] fix empty chunk sending in response chunks --- src/ngx_http_modsecurity_body_filter.c | 48 ++++++++++++++------------ 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 04d84de..e0e4e7b 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -153,31 +153,33 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); } + if (data_size > 0){ + copy_chain = ngx_alloc_chain_link(r->pool); + if (copy_chain == NULL) { + return NGX_ERROR; + } - copy_chain = ngx_alloc_chain_link(r->pool); - if (copy_chain == NULL) { - return NGX_ERROR; - } - - copy_buf = ngx_calloc_buf(r->pool); - if (copy_buf == NULL) { - return NGX_ERROR; - } - copy_buf->start = ngx_pcalloc(r->pool, data_size); - if (copy_buf->start == NULL) { - return NGX_ERROR; + copy_buf = ngx_calloc_buf(r->pool); + if (copy_buf == NULL) { + return NGX_ERROR; + } + copy_buf->start = ngx_pcalloc(r->pool, data_size); + if (copy_buf->start == NULL) { + return NGX_ERROR; + } + ngx_memcpy(copy_buf->start, chain->buf->start, data_size); + copy_buf->pos = copy_buf->start + data_offset; + copy_buf->end = copy_buf->start + data_size; + copy_buf->last = copy_buf->pos + ngx_buf_size(chain->buf); + copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0; + copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0; + copy_chain->buf = copy_buf; + copy_chain->buf->last_buf = 1; + copy_chain->next = NULL; + chain->buf->pos = chain->buf->last; } - ngx_memcpy(copy_buf->start, chain->buf->start, data_size); - copy_buf->pos = copy_buf->start + data_offset; - copy_buf->end = copy_buf->start + data_size; - copy_buf->last = copy_buf->pos + ngx_buf_size(chain->buf); - copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0; - copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0; - copy_chain->buf = copy_buf; - copy_chain->buf->last_buf = 1; - copy_chain->next = NULL; - chain->buf->pos = chain->buf->last; - + else + copy_chain = chain; if (ctx->temp_chain == NULL) { ctx->temp_chain = copy_chain; } else { From 40958336a64e4529081feedd9dbf5b0abb4e1328 Mon Sep 17 00:00:00 2001 From: dennus Date: Wed, 20 Dec 2017 14:58:55 +0400 Subject: [PATCH 03/14] fix tests --- src/ngx_http_modsecurity_body_filter.c | 8 ++++++-- tests/modsecurity.t | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index e0e4e7b..aa52e76 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -59,6 +59,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) return ngx_http_next_body_filter(r, in); } + #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS) loc_cf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module); if (loc_cf != NULL && loc_cf->sanity_checks_enabled != NGX_CONF_UNSET) @@ -200,14 +201,17 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) ngx_http_modsecurity_pcre_malloc_done(old_pool); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { + if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL) + ctx->header_pt(r); return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); } else if (ret < 0) { return ngx_http_filter_finalize_request(r, - &ngx_http_modsecurity_module, ret); + &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); } ctx->response_body_filtered = 1; - ctx->header_pt(r); + if (ctx->header_pt != NULL) + ctx->header_pt(r); return ngx_http_next_body_filter(r, ctx->temp_chain); } else { return NGX_AGAIN; diff --git a/tests/modsecurity.t b/tests/modsecurity.t index 7079447..e570810 100644 --- a/tests/modsecurity.t +++ b/tests/modsecurity.t @@ -135,13 +135,13 @@ is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4'); like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); like(http_get('/phase2?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 2'); like(http_get('/phase3?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 3'); -is(http_get('/phase4?what=block401'), '', 'block 401 - phase 4'); +like(http_get('/phase4?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 4'); # Block (403) like(http_get('/phase1?what=block403'), qr/403 Forbidden/, 'block 403 - phase 1'); like(http_get('/phase2?what=block403'), qr/403 Forbidden/, 'block 403 - phase 2'); like(http_get('/phase3?what=block403'), qr/403 Forbidden/, 'block 403 - phase 3'); -is(http_get('/phase4?what=block403'), '', 'block 403 - phase 4'); +like(http_get('/phase4?what=block403'), qr/403 Forbidden/, 'block 403 - phase 4'); # Nothing to detect like(http_get('/phase1?what=nothing'), qr/should be moved\/blocked before this./, 'nothing phase 1'); From 7c44a6b3f3ef6b042ae93b90ef432e3622a66d06 Mon Sep 17 00:00:00 2001 From: dennus Date: Wed, 20 Dec 2017 15:39:14 +0400 Subject: [PATCH 04/14] added 2 tests modification for proxy --- tests/modsecurity-proxy.t | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/modsecurity-proxy.t b/tests/modsecurity-proxy.t index 9bd0102..82d11dc 100644 --- a/tests/modsecurity-proxy.t +++ b/tests/modsecurity-proxy.t @@ -128,13 +128,13 @@ is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4'); like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); like(http_get('/phase2?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 2'); like(http_get('/phase3?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 3'); -is(http_get('/phase4?what=block401'), '', 'block 401 - phase 4'); +like(http_get('/phase4?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 4'); # Block (403) like(http_get('/phase1?what=block403'), qr/403 Forbidden/, 'block 403 - phase 1'); like(http_get('/phase2?what=block403'), qr/403 Forbidden/, 'block 403 - phase 2'); like(http_get('/phase3?what=block403'), qr/403 Forbidden/, 'block 403 - phase 3'); -is(http_get('/phase4?what=block403'), '', 'block 403 - phase 4'); +like(http_get('/phase4?what=block403'), qr/403 Forbidden/, 'block 403 - phase 4'); # Nothing to detect like(http_get('/phase1?what=nothing'), qr/phase1\?what=nothing\' not found/, 'nothing phase 1'); From b520ac9b6f4a610360050b58813af9b6d05ad30d Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 3 Apr 2018 15:22:14 +0400 Subject: [PATCH 05/14] don't do redirect on phase 4 --- src/ngx_http_modsecurity_body_filter.c | 4 ++-- tests/modsecurity-proxy.t | 4 ++-- tests/modsecurity.t | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index aa52e76..0e26754 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -201,9 +201,9 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) ngx_http_modsecurity_pcre_malloc_done(old_pool); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { - if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL) + if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL) ctx->header_pt(r); - return ngx_http_filter_finalize_request(r, + else return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); } else if (ret < 0) { return ngx_http_filter_finalize_request(r, diff --git a/tests/modsecurity-proxy.t b/tests/modsecurity-proxy.t index 82d11dc..25d5433 100644 --- a/tests/modsecurity-proxy.t +++ b/tests/modsecurity-proxy.t @@ -116,13 +116,13 @@ unlike(http_head('/'), qr/SEE-THIS/, 'proxy head request'); like(http_get('/phase1?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 1'); like(http_get('/phase2?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 2'); like(http_get('/phase3?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 3'); -is(http_get('/phase4?what=redirect302'), '', 'redirect 302 - phase 4'); +like(http_get('/phase4?what=redirect302'), qr/404/, 'redirect 302 - phase 4'); # Redirect (301) like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1'); like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2'); like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3'); -is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4'); +like(http_get('/phase4?what=redirect301'), qr/404, 'redirect 301 - phase 4'); # Block (401) like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); diff --git a/tests/modsecurity.t b/tests/modsecurity.t index e570810..2d31076 100644 --- a/tests/modsecurity.t +++ b/tests/modsecurity.t @@ -123,13 +123,13 @@ $t->plan(20); like(http_get('/phase1?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 1'); like(http_get('/phase2?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 2'); like(http_get('/phase3?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 3'); -is(http_get('/phase4?what=redirect302'), '', 'redirect 302 - phase 4'); +like(http_get('/phase4?what=redirect302'), qr/404/, 'redirect 302 - phase 4'); # Redirect (301) like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1'); like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2'); like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3'); -is(http_get('/phase4?what=redirect301'), '', 'redirect 301 - phase 4'); +like(http_get('/phase4?what=redirect301'), qr/404/, 'redirect 301 - phase 4'); # Block (401) like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); From cd967cb5ad6316665925e8043b7b77de4b233755 Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 3 Apr 2018 15:48:17 +0400 Subject: [PATCH 06/14] fix status codes in test --- tests/modsecurity-proxy.t | 2 +- tests/modsecurity.t | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/modsecurity-proxy.t b/tests/modsecurity-proxy.t index 25d5433..ae94f15 100644 --- a/tests/modsecurity-proxy.t +++ b/tests/modsecurity-proxy.t @@ -122,7 +122,7 @@ like(http_get('/phase4?what=redirect302'), qr/404/, 'redirect 302 - phase 4'); like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1'); like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2'); like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3'); -like(http_get('/phase4?what=redirect301'), qr/404, 'redirect 301 - phase 4'); +like(http_get('/phase4?what=redirect301'), qr/404/, 'redirect 301 - phase 4'); # Block (401) like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); diff --git a/tests/modsecurity.t b/tests/modsecurity.t index 2d31076..0dd982f 100644 --- a/tests/modsecurity.t +++ b/tests/modsecurity.t @@ -123,13 +123,13 @@ $t->plan(20); like(http_get('/phase1?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 1'); like(http_get('/phase2?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 2'); like(http_get('/phase3?what=redirect302'), qr/302 Moved Temporarily/, 'redirect 302 - phase 3'); -like(http_get('/phase4?what=redirect302'), qr/404/, 'redirect 302 - phase 4'); +like(http_get('/phase4?what=redirect302'), qr/200/, 'redirect 302 - phase 4'); # Redirect (301) like(http_get('/phase1?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 1'); like(http_get('/phase2?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 2'); like(http_get('/phase3?what=redirect301'), qr/301 Moved Permanently/, 'redirect 301 - phase 3'); -like(http_get('/phase4?what=redirect301'), qr/404/, 'redirect 301 - phase 4'); +like(http_get('/phase4?what=redirect301'), qr/200/, 'redirect 301 - phase 4'); # Block (401) like(http_get('/phase1?what=block401'), qr/401 Unauthorized/, 'block 401 - phase 1'); From c99bd17c13eecf02d659698d78ae9a2022e46c2a Mon Sep 17 00:00:00 2001 From: dennus Date: Mon, 23 Apr 2018 13:06:19 +0400 Subject: [PATCH 07/14] fix response body chunks processing --- src/ngx_http_modsecurity_body_filter.c | 41 +++++++++++++----------- src/ngx_http_modsecurity_header_filter.c | 3 +- src/ngx_http_modsecurity_pre_access.c | 4 +-- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 0e26754..802fe1e 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -34,7 +34,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) { - ngx_http_modsecurity_ctx_t *ctx = NULL; ngx_chain_t *chain = in; ngx_int_t ret; @@ -47,15 +46,17 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) ngx_uint_t i = 0; #endif - if (in == NULL) { +if (in == NULL) { + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "MDS input chain is null"); + return ngx_http_next_body_filter(r, in); - } + } /* get context for request */ ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module); dd("body filter, recovering ctx: %p", ctx); - if (ctx == NULL || r->filter_finalize) { + if (ctx == NULL || r->filter_finalize || ctx->response_body_filtered) { return ngx_http_next_body_filter(r, in); } @@ -143,18 +144,16 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) ngx_buf_t *copy_buf; ngx_chain_t* copy_chain; is_request_processed = chain->buf->last_buf; - ngx_int_t data_size = chain->buf->end - chain->buf->start; - ngx_int_t data_offset = chain->buf->pos - chain->buf->start; - u_char *data = chain->buf->start; + ngx_int_t data_size = chain->buf->last - chain->buf->pos; + u_char *data = chain->buf->pos; msc_append_response_body(ctx->modsec_transaction, data, - chain->buf->end - 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 (data_size > 0){ copy_chain = ngx_alloc_chain_link(r->pool); if (copy_chain == NULL) { return NGX_ERROR; @@ -168,9 +167,9 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) if (copy_buf->start == NULL) { return NGX_ERROR; } - ngx_memcpy(copy_buf->start, chain->buf->start, data_size); - copy_buf->pos = copy_buf->start + data_offset; - copy_buf->end = copy_buf->start + data_size; + ngx_memcpy(copy_buf->start, chain->buf->pos, data_size); + copy_buf->pos = copy_buf->start ; + copy_buf->end = copy_buf->pos + data_size ; copy_buf->last = copy_buf->pos + ngx_buf_size(chain->buf); copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0; copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0; @@ -178,9 +177,6 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) copy_chain->buf->last_buf = 1; copy_chain->next = NULL; chain->buf->pos = chain->buf->last; - } - else - copy_chain = chain; if (ctx->temp_chain == NULL) { ctx->temp_chain = copy_chain; } else { @@ -196,15 +192,23 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) } if (is_request_processed) { + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING"); 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); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { - if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL) + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING RET = %d", ret); + if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){ + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO HEADER FILTERS = %d", ret); ctx->header_pt(r); - else return ngx_http_filter_finalize_request(r, - &ngx_http_modsecurity_module, ret); + } + else { + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO FINALIZE = %d", ret); + ngx_http_filter_finalize_request(r, + &ngx_http_modsecurity_module + , ret); + } } else if (ret < 0) { return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); @@ -214,6 +218,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) ctx->header_pt(r); return ngx_http_next_body_filter(r, ctx->temp_chain); } else { + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS WAITING FOR NEXT CHUNK"); return NGX_AGAIN; } } diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 400a9d6..e4d646c 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -552,6 +552,7 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) } else { ctx->header_pt = ngx_http_next_header_filter; + r->headers_out.content_length_n = -1; return NGX_AGAIN; - } + } } 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 08c3fbab854483ad87d8be25be78bbae344c64ef Mon Sep 17 00:00:00 2001 From: dennus Date: Mon, 23 Apr 2018 14:09:58 +0400 Subject: [PATCH 08/14] fix log output --- src/ngx_http_modsecurity_body_filter.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 802fe1e..c55c255 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -192,19 +192,15 @@ if (in == NULL) { } if (is_request_processed) { - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING"); 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); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING RET = %d", ret); if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){ - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO HEADER FILTERS = %d", ret); ctx->header_pt(r); } else { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO FINALIZE = %d", ret); ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module , ret); @@ -218,7 +214,6 @@ if (in == NULL) { ctx->header_pt(r); return ngx_http_next_body_filter(r, ctx->temp_chain); } else { - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS WAITING FOR NEXT CHUNK"); return NGX_AGAIN; } } From e59954f6015d162422ef24752b9913045677f0b3 Mon Sep 17 00:00:00 2001 From: dennus Date: Wed, 16 May 2018 18:01:22 +0400 Subject: [PATCH 09/14] Switch ResponseBodyAccess on Phase 4 --- tests/modsecurity-proxy.t | 3 ++- tests/modsecurity.t | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/modsecurity-proxy.t b/tests/modsecurity-proxy.t index ae94f15..50f613c 100644 --- a/tests/modsecurity-proxy.t +++ b/tests/modsecurity-proxy.t @@ -86,7 +86,8 @@ http { modsecurity on; modsecurity_rules ' SecRuleEngine On - SecDefaultAction "phase:4,log,deny,status:403" + SecResponseBodyAccess On + SecDefaultAction "phase:4,log,deny,status:403" SecRule ARGS "@streq redirect301" "id:1,phase:4,status:301,redirect:http://www.modsecurity.org" SecRule ARGS "@streq redirect302" "id:2,phase:4,status:302,redirect:http://www.modsecurity.org" SecRule ARGS "@streq block401" "id:3,phase:4,status:401,block" diff --git a/tests/modsecurity.t b/tests/modsecurity.t index 0dd982f..ca341ad 100644 --- a/tests/modsecurity.t +++ b/tests/modsecurity.t @@ -97,7 +97,8 @@ http { modsecurity on; modsecurity_rules ' SecRuleEngine On - SecDefaultAction "phase:4,log,deny,status:403" + SecResponseBodyAccess On + SecDefaultAction "phase:4,log,deny,status:403" SecRule ARGS "@streq redirect301" "id:1,phase:4,status:301,redirect:http://www.modsecurity.org" SecRule ARGS "@streq redirect302" "id:2,phase:4,status:302,redirect:http://www.modsecurity.org" SecRule ARGS "@streq block401" "id:3,phase:4,status:401,block" From 17cb5b42a563d7f77cb09fdf92ad07ddbd1852f6 Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 26 Jun 2018 09:30:29 +0400 Subject: [PATCH 10/14] fix return --- src/ngx_http_modsecurity_body_filter.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 2a8931f..be088f9 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -201,7 +201,7 @@ if (in == NULL) { ctx->header_pt(r); } else { - ngx_http_filter_finalize_request(r, + return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module , ret); } From 42c6a981427d6d110d6c1f39ba557f024dbae79c Mon Sep 17 00:00:00 2001 From: dennus Date: Tue, 26 Jun 2018 12:33:03 +0400 Subject: [PATCH 11/14] case sentivity var name? --- tests/modsecurity-scoring.t | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/modsecurity-scoring.t b/tests/modsecurity-scoring.t index ddd3b61..33a3612 100644 --- a/tests/modsecurity-scoring.t +++ b/tests/modsecurity-scoring.t @@ -46,7 +46,7 @@ http { SecRuleEngine On SecRule ARGS "@streq badarg1" "id:11,phase:2,setvar:tx.score=1" SecRule ARGS "@streq badarg2" "id:12,phase:2,setvar:tx.score=2" - SecRule TX:SCORE "@ge 2" "id:199,phase:request,deny,log,status:403" + SecRule tx:score "@ge 2" "id:199,phase:request,deny,log,status:403" '; } @@ -56,7 +56,7 @@ http { SecRule ARGS "@streq badarg1" "id:21,phase:2,setvar:tx.score=+1" SecRule ARGS "@streq badarg2" "id:22,phase:2,setvar:tx.score=+1" SecRule ARGS "@streq badarg3" "id:23,phase:2,setvar:tx.score=+1" - SecRule TX:SCORE "@ge 3" "id:299,phase:request,deny,log,status:403" + SecRule tx:score "@ge 3" "id:299,phase:request,deny,log,status:403" '; } } From 5f5a32c4a5d78182ac9e557d08b2da2924bb0a8d Mon Sep 17 00:00:00 2001 From: Dennus Date: Tue, 30 Oct 2018 13:30:55 +0400 Subject: [PATCH 12/14] Update ngx_http_modsecurity_body_filter.c --- src/ngx_http_modsecurity_body_filter.c | 39 ++++++++++++++++---------- 1 file changed, 24 insertions(+), 15 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index be088f9..f6fa2f2 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -57,6 +57,8 @@ if (in == NULL) { dd("body filter, recovering ctx: %p", ctx); if (ctx == NULL || r->filter_finalize || ctx->response_body_filtered) { + if (ctx && ctx->response_body_filtered) + r->filter_finalize = 1; return ngx_http_next_body_filter(r, in); } @@ -138,8 +140,8 @@ if (in == NULL) { } } #endif - - for (chain = in; chain != NULL; chain = chain->next) { + + for (chain = in; chain != NULL; chain = chain->next) { ngx_buf_t *copy_buf; ngx_chain_t* copy_chain; @@ -154,29 +156,28 @@ if (in == NULL) { return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); } + if (!chain->buf->last_buf){ copy_chain = ngx_alloc_chain_link(r->pool); if (copy_chain == NULL) { return NGX_ERROR; } - - copy_buf = ngx_calloc_buf(r->pool); + copy_buf = ngx_calloc_buf(r->pool); if (copy_buf == NULL) { return NGX_ERROR; } - copy_buf->start = ngx_pcalloc(r->pool, data_size); - if (copy_buf->start == NULL) { - return NGX_ERROR; - } - ngx_memcpy(copy_buf->start, chain->buf->pos, data_size); - copy_buf->pos = copy_buf->start ; - copy_buf->end = copy_buf->pos + data_size ; - copy_buf->last = copy_buf->pos + ngx_buf_size(chain->buf); + //copy_buf->last=ngx_cpymem(copy_buf->pos, chain->buf->pos, data_size); + copy_buf->pos = chain->buf->pos ; + copy_buf->end = chain->buf->end; + copy_buf->last = chain->buf->last; copy_buf->temporary = (chain->buf->temporary == 1) ? 1 : 0; copy_buf->memory = (chain->buf->memory == 1) ? 1 : 0; copy_chain->buf = copy_buf; - copy_chain->buf->last_buf = 1; + copy_chain->buf->last_buf = chain->buf->last_buf; copy_chain->next = NULL; chain->buf->pos = chain->buf->last; + } + else + copy_chain = chain; if (ctx->temp_chain == NULL) { ctx->temp_chain = copy_chain; } else { @@ -189,23 +190,30 @@ if (in == NULL) { } ctx->current_chain = copy_chain; } + } if (is_request_processed) { + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING"); 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); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING RET = %d", ret); if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){ + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO HEADER FILTERS = %d", ret); ctx->header_pt(r); } else { - return ngx_http_filter_finalize_request(r, + ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO FINALIZE = %d", ret); + ctx->response_body_filtered = 1; + return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module , ret); } } else if (ret < 0) { + ctx->response_body_filtered = 1; return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR); } @@ -214,6 +222,7 @@ if (in == NULL) { ctx->header_pt(r); return ngx_http_next_body_filter(r, ctx->temp_chain); } else { + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS WAITING FOR NEXT CHUNK"); return NGX_AGAIN; } -} \ No newline at end of file +} From 9b5bd5335a2a6c75418874e68146696e49c0780c Mon Sep 17 00:00:00 2001 From: Dennus Date: Tue, 30 Oct 2018 13:34:42 +0400 Subject: [PATCH 13/14] bypass buffer data copying --- src/ngx_http_modsecurity_body_filter.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index f6fa2f2..7d5b06c 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -165,7 +165,6 @@ if (in == NULL) { if (copy_buf == NULL) { return NGX_ERROR; } - //copy_buf->last=ngx_cpymem(copy_buf->pos, chain->buf->pos, data_size); copy_buf->pos = chain->buf->pos ; copy_buf->end = chain->buf->end; copy_buf->last = chain->buf->last; @@ -192,21 +191,17 @@ if (in == NULL) { } } - + if (is_request_processed) { - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING"); 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); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH PROCESSING RET = %d", ret); - if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){ - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO HEADER FILTERS = %d", ret); + if (ret < NGX_HTTP_BAD_REQUEST && ctx->header_pt != NULL){ ctx->header_pt(r); } - else { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS FINISH DO FINALIZE = %d", ret); + else { ctx->response_body_filtered = 1; return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module @@ -222,7 +217,6 @@ if (in == NULL) { ctx->header_pt(r); return ngx_http_next_body_filter(r, ctx->temp_chain); } else { - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0,"MDS WAITING FOR NEXT CHUNK"); return NGX_AGAIN; } } From e315f5b91044dde08b106a85fc9fe739120b301f Mon Sep 17 00:00:00 2001 From: Dennus Date: Tue, 30 Oct 2018 14:31:26 +0400 Subject: [PATCH 14/14] remove unused variables --- src/ngx_http_modsecurity_body_filter.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 7d5b06c..05fb571 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -146,7 +146,6 @@ if (in == NULL) { ngx_buf_t *copy_buf; ngx_chain_t* copy_chain; is_request_processed = chain->buf->last_buf; - ngx_int_t data_size = chain->buf->last - chain->buf->pos; u_char *data = chain->buf->pos; msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);