Merge pull request #240 from defanator/issue238-fix

Use ngx_http_filter_finalize_request() on intervention in header filter

Author: defanator

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

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)

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;
 
 

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' */
 
@@ -525,9 +529,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);
     }
 
     /*

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",

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;
         }
     }

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)');