From e028ca43c83fd114344c34df6a8dd09eb5f34cc2 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Wed, 3 Feb 2021 13:22:02 +0300 Subject: [PATCH 1/2] Use ngx_http_filter_finalize_request() on intervention in header filter Closes #238. --- CHANGES | 2 ++ src/ngx_http_modsecurity_header_filter.c | 4 ++-- tests/modsecurity.t | 22 +++++++++++++++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/CHANGES b/CHANGES index b7716ef..cbc6a37 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v1.0.x - YYYY-MMM-DD (To be released) ------------------------------------- + - Fix nginx sends response without headers + [Issue #238 - @airween, @defanator] - Fix nginx not clearing body cache (caused by incomplete fix for #187) [Issue #216 - @krewi1, @martinhsv] - Fix config setting not respected: client_body_in_file_only on diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 84ce858..8e2bc69 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -525,9 +525,9 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (r->error_page) { return ngx_http_next_header_filter(r); - } + } if (ret > 0) { - return ret; + return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); } /* diff --git a/tests/modsecurity.t b/tests/modsecurity.t index 78c51fe..fcb26f4 100644 --- a/tests/modsecurity.t +++ b/tests/modsecurity.t @@ -105,6 +105,23 @@ http { SecRule ARGS "@streq block403" "id:4,phase:4,status:403,block" '; } + location /early-block { + modsecurity on; + modsecurity_rules ' + SecRuleEngine On + SecResponseBodyAccess On + SecDefaultAction "phase:1,log,auditlog,pass" + SecDefaultAction "phase:2,log,auditlog,pass" + SecAction "id:900101,phase:1,nolog,pass,t:none,setvar:tx.trigger_phase1=1" + SecAction "id:900103,phase:1,nolog,pass,t:none,setvar:tx.trigger_phase3=1" + SecAction "id:900105,phase:1,nolog,pass,t:none,setvar:tx.trigger_phase5=1" + SecRule TX:TRIGGER_PHASE1 "@eq 1" "id:901111,phase:1,t:none,deny,log" + SecRule REQUEST_BODY "@rx attack" "id:901121,phase:2,t:none,deny,log" + SecRule TX:TRIGGER_PHASE3 "@eq 1" "id:901131,phase:3,t:none,deny,log" + SecRule RESPONSE_BODY "@rx ok" "id:901141,phase:4,t:none,deny,log" + SecRule TX:TRIGGER_PHASE5 "@eq 1" "id:901151,phase:5,t:none,pass,log,msg:\'This is the phase 5.\'" + '; + } } } EOF @@ -113,9 +130,10 @@ $t->write_file("/phase1", "should be moved/blocked before this."); $t->write_file("/phase2", "should be moved/blocked before this."); $t->write_file("/phase3", "should be moved/blocked before this."); $t->write_file("/phase4", "should not be moved/blocked, headers delivered before phase 4."); +$t->write_file("/early-block", "should be moved/blocked before this."); $t->run(); $t->todo_alerts(); -$t->plan(20); +$t->plan(21); ############################################################################### @@ -150,3 +168,5 @@ like(http_get('/phase2?what=nothing'), qr/should be moved\/blocked before this./ like(http_get('/phase3?what=nothing'), qr/should be moved\/blocked before this./, 'nothing phase 3'); like(http_get('/phase4?what=nothing'), qr/should not be moved\/blocked, headers delivered before phase 4./, 'nothing phase 4'); +# early block (https://github.com/SpiderLabs/ModSecurity-nginx/issues/238) +like(http_get('/early-block'), qr/^HTTP.*403/, 'early block 403 (https://github.com/SpiderLabs/ModSecurity-nginx/issues/238)'); From 4f26b48998db5119ca818b0909b6b14e08ebb544 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Wed, 3 Feb 2021 18:13:38 +0300 Subject: [PATCH 2/2] Avoid extra processing of subsequent interventions if one was already triggered --- src/ngx_http_modsecurity_body_filter.c | 4 ++++ src/ngx_http_modsecurity_common.h | 1 + src/ngx_http_modsecurity_header_filter.c | 4 ++++ src/ngx_http_modsecurity_pre_access.c | 4 ++++ src/ngx_http_modsecurity_rewrite.c | 3 +++ 5 files changed, 16 insertions(+) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 6118a94..1b85df5 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -56,6 +56,10 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) return ngx_http_next_body_filter(r, in); } + if (ctx->intervention_triggered) { + return ngx_http_next_body_filter(r, in); + } + #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS) mcf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module); if (mcf != NULL && mcf->sanity_checks_enabled != NGX_CONF_UNSET) diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index 900443e..79958b5 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -97,6 +97,7 @@ typedef struct { unsigned waiting_more_body:1; unsigned body_requested:1; unsigned processed:1; + unsigned intervention_triggered: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 8e2bc69..51990c7 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -430,6 +430,10 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) return ngx_http_next_header_filter(r); } + if (ctx->intervention_triggered) { + return ngx_http_next_header_filter(r); + } + /* XXX: can it happen ? already processed i mean */ /* XXX: check behaviour on 'ModSecurity off' */ diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c index 91ef1e0..779b71e 100644 --- a/src/ngx_http_modsecurity_pre_access.c +++ b/src/ngx_http_modsecurity_pre_access.c @@ -78,6 +78,10 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) return NGX_HTTP_INTERNAL_SERVER_ERROR; } + if (ctx->intervention_triggered) { + return NGX_DECLINED; + } + if (ctx->waiting_more_body == 1) { dd("waiting for more data before proceed. / count: %d", diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_rewrite.c index b6a6d6c..4316db8 100644 --- a/src/ngx_http_modsecurity_rewrite.c +++ b/src/ngx_http_modsecurity_rewrite.c @@ -117,6 +117,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) dd("Processing intervention with the connection information filled in"); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { + ctx->intervention_triggered = 1; return ret; } @@ -157,6 +158,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) dd("Processing intervention with the transaction information filled in (uri, method and version)"); ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { + ctx->intervention_triggered = 1; return ret; } @@ -208,6 +210,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) return NGX_DECLINED; } if (ret > 0) { + ctx->intervention_triggered = 1; return ret; } }