fix: fail closed on swallowed WAF return values

src/ngx_http_modsecurity_access.c

@@ -182,6 +182,10 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
             ctx->intervention_triggered = 1;
             return ret;
         }
+        else if (ret < 0) {
+            ctx->intervention_triggered = 1;
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
 
         const char *http_version;
         switch (r->http_version) {
@@ -231,6 +235,10 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
             ctx->intervention_triggered = 1;
             return ret;
         }
+        else if (ret < 0) {
+            ctx->intervention_triggered = 1;
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
 
         /**
          * Since incoming request headers are already in place, lets send it to ModSecurity
@@ -283,6 +291,10 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
             ctx->intervention_triggered = 1;
             return ret;
         }
+        else if (ret < 0) {
+            ctx->intervention_triggered = 1;
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
     }
 
 #if 1
@@ -403,6 +415,18 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
              */
             dd("request body inspection: file -- %s", file_name);
 
+            /*
+             * msc_request_body_from_file()/msc_append_request_body() return
+             * 0 not only on a genuine failure but also -- indistinguishably,
+             * from the caller's side -- whenever SecRequestBodyLimitAction
+             * is ProcessPartial and the body exceeds SecRequestBodyLimit:
+             * libmodsecurity deliberately truncates at the limit and
+             * reports it the same way (see Transaction::appendRequestBody(),
+             * which is what requestBodyFromFile() delegates to). That is
+             * normal, by-design behavior, not an inspection bypass -- the
+             * truncated content is still evaluated -- so it must not fail
+             * the request closed.
+             */
             msc_request_body_from_file(ctx->modsec_transaction, file_name);
 
             already_inspected = 1;
@@ -414,6 +438,10 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
         {
             u_char *data = chain->buf->pos;
 
+            /* See the comment on msc_request_body_from_file() above: 0 here
+             * means either a real failure or (indistinguishably) a
+             * by-design ProcessPartial truncation, so it must not fail
+             * closed. */
             msc_append_request_body(ctx->modsec_transaction, data,
                 chain->buf->last - data);
 
@@ -431,8 +459,13 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
              */
             ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
             if (ret > 0) {
+                ctx->intervention_triggered = 1;
                 return ret;
             }
+            else if (ret < 0) {
+                ctx->intervention_triggered = 1;
+                return NGX_HTTP_INTERNAL_SERVER_ERROR;
+            }
         }
 
         /**
@@ -445,17 +478,29 @@ ngx_http_modsecurity_access_handler(ngx_http_request_t *r)
 /* XXX: once more -- is body can be modified ?  content-length need to be adjusted ? */
 
         old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
-        msc_process_request_body(ctx->modsec_transaction);
-        ctx->request_body_processed = 1;
+        ret = msc_process_request_body(ctx->modsec_transaction);
         ngx_http_modsecurity_pcre_malloc_done(old_pool);
+        ctx->request_body_processed = 1;
+
+        if (ret != 1) {
+            ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+                "ModSecurity: request body phase processing failed");
+            ctx->intervention_triggered = 1;
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
 
         ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
         if (r->error_page) {
             return NGX_DECLINED;
             }
         if (ret > 0) {
+            ctx->intervention_triggered = 1;
             return ret;
         }
+        else if (ret < 0) {
+            ctx->intervention_triggered = 1;
+            return NGX_HTTP_INTERNAL_SERVER_ERROR;
+        }
     }
 
     dd("Nothing to add on the body inspection, reclaiming a NGX_DECLINED");

src/ngx_http_modsecurity_body_filter.c

@@ -146,12 +146,44 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
         u_char *data = chain->buf->pos;
         int ret;
 
-        msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
+        /*
+         * msc_append_response_body() returns 0 not only on a genuine
+         * failure but also -- indistinguishably, from the caller's side --
+         * whenever SecResponseBodyLimitAction is ProcessPartial and the
+         * body exceeds SecResponseBodyLimit: libmodsecurity deliberately
+         * truncates at the limit and reports it the same way (see
+         * Transaction::appendResponseBody(), whose own docstring is
+         * "@retval false Operation failed, process partial demanded"). That
+         * is normal, by-design behavior, not an inspection bypass -- the
+         * truncated content is still evaluated -- so it must not fail the
+         * request closed.
+         */
+        msc_append_response_body(ctx->modsec_transaction, data,
+            chain->buf->last - data);
+
         ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
         if (ret > 0) {
+            ctx->intervention_triggered = 1;
             return ngx_http_filter_finalize_request(r,
                 &ngx_http_modsecurity_module, ret);
         }
+        else if (ret < 0) {
+            /*
+             * process_intervention() returns -1 specifically when
+             * r->header_sent is already true (it wanted to intervene but
+             * can't safely rewrite headers). By the time the body filter
+             * runs, header_sent is *always* true -- header_filter() already
+             * ran. ngx_http_filter_finalize_request() would call
+             * ngx_http_clean_header() and try to send a fresh status line
+             * and headers anyway, producing nginx's own "header already
+             * sent" alert and a truncated response instead of a clean
+             * abort. NGX_ERROR is the correct signal here: it triggers
+             * ngx_http_terminate_request(), which closes the connection
+             * without attempting to resend anything.
+             */
+            ctx->intervention_triggered = 1;
+            return NGX_ERROR;
+        }
 
 /* XXX: chain->buf->last_buf || chain->buf->last_in_chain */
         is_request_processed = chain->buf->last_buf;
@@ -160,19 +192,28 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
             ngx_pool_t *old_pool;
 
             old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
-            msc_process_response_body(ctx->modsec_transaction);
+            ret = msc_process_response_body(ctx->modsec_transaction);
             ngx_http_modsecurity_pcre_malloc_done(old_pool);
 
+            if (ret != 1) {
+                ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+                    "ModSecurity: response body phase processing failed");
+                ctx->intervention_triggered = 1;
+                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, 0);
             if (ret > 0) {
+                ctx->intervention_triggered = 1;
                 return ret;
             }
             else if (ret < 0) {
-                return ngx_http_filter_finalize_request(r,
-                    &ngx_http_modsecurity_module, NGX_HTTP_INTERNAL_SERVER_ERROR);
-
+                /* See the comment on the identical check above. */
+                ctx->intervention_triggered = 1;
+                return NGX_ERROR;
             }
         }
     }

src/ngx_http_modsecurity_header_filter.c

@@ -533,8 +533,21 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r)
         return ngx_http_next_header_filter(r);
     }
     if (ret > 0) {
+        ctx->intervention_triggered = 1;
         return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret);
     }
+    else if (ret < 0) {
+        /*
+         * process_intervention() returns -1 specifically when
+         * r->header_sent is already true. ngx_http_filter_finalize_request()
+         * would try to send a fresh status line and headers anyway via
+         * ngx_http_clean_header(), producing nginx's own "header already
+         * sent" alert. NGX_ERROR triggers ngx_http_terminate_request()
+         * instead, which just closes the connection.
+         */
+        ctx->intervention_triggered = 1;
+        return NGX_ERROR;
+    }
 
     /*
      * Proxies will not like this... but it is necessary to unset

src/ngx_http_modsecurity_module.c

@@ -290,14 +290,25 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r)
 
     if (mcf->transaction_id) {
         if (ngx_http_complex_value(r, mcf->transaction_id, &s) != NGX_OK) {
-            return NGX_CONF_ERROR;
+            return NULL;
         }
         ctx->modsec_transaction = msc_new_transaction_with_id(mmcf->modsec, mcf->rules_set, (char *) s.data, r->connection->log);
 
     } else {
         ctx->modsec_transaction = msc_new_transaction(mmcf->modsec, mcf->rules_set, r->connection->log);
     }
 
+    /*
+     * A NULL transaction here would make every later msc_* call inspect
+     * nothing (fail-open) while the request/response is still forwarded.
+     * Fail closed instead: the caller turns a NULL ctx into a 500.
+     */
+    if (ctx->modsec_transaction == NULL) {
+        ngx_log_error(NGX_LOG_ERR, r->connection->log, 0,
+            "ModSecurity: failed to create transaction");
+        return NULL;
+    }
+
     dd("transaction created");
 
     ngx_http_set_ctx(r, ctx, ngx_http_modsecurity_module);
@@ -306,15 +317,15 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r)
     if (cln == NULL)
     {
         dd("failed to create the ModSecurity context cleanup");
-        return NGX_CONF_ERROR;
+        return NULL;
     }
     cln->handler = ngx_http_modsecurity_cleanup;
     cln->data = ctx;
 
 #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
     ctx->sanity_headers_out = ngx_array_create(r->pool, 12, sizeof(ngx_http_modsecurity_header_t));
     if (ctx->sanity_headers_out == NULL) {
-        return NGX_CONF_ERROR;
+        return NULL;
     }
 #endif
 

tests/modsecurity-response-body-deny.t

@@ -0,0 +1,109 @@
+#!/usr/bin/perl
+
+#
+# ModSecurity, http://www.modsecurity.org/
+# Copyright (c) 2015 Trustwave Holdings, Inc. (http://www.trustwave.com/)
+#
+# You may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# If any of the files related to licensing are missing or if you have any
+# other questions related to licensing please contact Trustwave Holdings, Inc.
+# directly using the email address security@modsecurity.org.
+#
+
+
+# Regression test for a phase-4 RESPONSE_BODY "deny" action.
+#
+# By the time msc_process_response_body() detects a violation, response
+# headers (status, Content-Length) have already been sent to the client --
+# this connector does not delay headers until body inspection completes.
+# ngx_http_modsecurity_body_filter() used to report that via
+# ngx_http_filter_finalize_request(), which tries to send a *fresh* status
+# line and headers regardless (via nginx's own ngx_http_clean_header()).
+# Since headers were already flushed, this produced nginx's own "header
+# already sent" [alert] and a corrupted response instead of a clean abort.
+
+###############################################################################
+
+use warnings;
+use strict;
+
+use Test::More;
+
+BEGIN { use FindBin; chdir($FindBin::Bin); }
+
+use lib 'lib';
+use Test::Nginx;
+
+###############################################################################
+
+select STDERR; $| = 1;
+select STDOUT; $| = 1;
+
+my $t = Test::Nginx->new()->has(qw/http/);
+
+$t->write_file_expand('nginx.conf', <<'EOF');
+
+%%TEST_GLOBALS%%
+
+daemon off;
+
+events {
+}
+
+http {
+    %%TEST_GLOBALS_HTTP%%
+
+    server {
+        listen       127.0.0.1:8080;
+        server_name  localhost;
+
+        modsecurity on;
+        modsecurity_rules_file %%TESTDIR%%/rules.conf;
+
+        location / {
+        }
+    }
+}
+EOF
+
+$t->write_file('rules.conf', <<'EOF');
+SecRuleEngine On
+SecResponseBodyAccess On
+SecResponseBodyMimeType text/plain
+SecRule RESPONSE_BODY "@rx leakmarker" "id:103,phase:4,deny,status:403"
+EOF
+
+# The marker sits at the end of a large body, so the deny only fires once
+# the whole response is buffered -- well after headers went out.
+$t->write_file('leak.txt', ('x' x 8192) . "leakmarker\n");
+$t->write_file('clean.txt', ('x' x 8192) . "\n");
+
+$t->run();
+$t->plan(3);
+
+###############################################################################
+
+my $clean = http_get('/clean.txt');
+like($clean, qr/^HTTP\/1\.[01] 200/, 'benign response body passes');
+
+my $leak = http_get('/leak.txt');
+# Whether the connection drops before any bytes go out (a response that fits
+# in one buffer) or mid-transfer after a partial prefix (a larger,
+# multi-buffer response), the leak marker itself is never delivered: it only
+# triggers the deny once msc_process_response_body() has read the full
+# buffered body, which happens strictly before that data is forwarded.
+unlike($leak, qr/leakmarker/, 'leak marker body was not delivered to the client');
+
+my $d = $t->testdir();
+my $errorlog = do {
+    local $/ = undef;
+    open my $fh, "<", "$d/error.log" or die "could not open: $!";
+    <$fh>;
+};
+unlike($errorlog, qr/header already sent/, 'no "header already sent" alert from the deny path');
+
+###############################################################################