fix: correctness gaps in response header/intervention handling

src/ngx_http_modsecurity_body_filter.c

@@ -58,6 +58,25 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
         return ngx_http_next_body_filter(r, in);
     }
 
+    if (ctx->response_replaced) {
+        /*
+         * The header filter already replaced this response with a
+         * WAF-triggered redirect after the original body started flowing
+         * from the content handler. Drop the original bytes -- they belong
+         * to a response we are no longer sending -- but keep forwarding the
+         * chain (last_buf/flush flags and all) so nginx still sees the
+         * response complete.
+         */
+        ngx_chain_t *cl;
+
+        for (cl = in; cl; cl = cl->next) {
+            cl->buf->pos = cl->buf->last;
+            cl->buf->in_file = 0;
+            cl->buf->file_last = cl->buf->file_pos;
+        }
+        return ngx_http_next_body_filter(r, in);
+    }
+
     if (ctx->intervention_triggered) {
         return ngx_http_next_body_filter(r, in);
     }

src/ngx_http_modsecurity_common.h

@@ -100,6 +100,14 @@ typedef struct {
     unsigned logged:1;
     unsigned intervention_triggered:1;
     unsigned request_body_processed:1;
+    /*
+     * Set when the header filter replaces an already-populated response with
+     * a WAF-triggered redirect (see ngx_http_modsecurity_header_filter()).
+     * The original response body may already be flowing through the output
+     * chain; the body filter must drop it rather than send it alongside the
+     * new redirect.
+     */
+    unsigned response_replaced:1;
 } ngx_http_modsecurity_ctx_t;
 
 

src/ngx_http_modsecurity_header_filter.c

@@ -157,10 +157,10 @@ ngx_http_modsecurity_resolv_header_server(ngx_http_request_t *r, ngx_str_t name,
     if (r->headers_out.server == NULL) {
         if (clcf->server_tokens) {
             value.data = (u_char *)ngx_http_server_full_string;
-            value.len = sizeof(ngx_http_server_full_string);
+            value.len = sizeof(ngx_http_server_full_string) - 1;
         } else {
             value.data = (u_char *)ngx_http_server_string;
-            value.len = sizeof(ngx_http_server_string);
+            value.len = sizeof(ngx_http_server_string) - 1;
         }
     } else {
         ngx_table_elt_t *h = r->headers_out.server;
@@ -218,7 +218,7 @@ ngx_http_modsecurity_resolv_header_content_length(ngx_http_request_t *r, ngx_str
 
     ctx = ngx_http_modsecurity_get_module_ctx(r);
 
-    if (r->headers_out.content_length_n > 0)
+    if (r->headers_out.content_length_n >= 0)
     {
         ngx_sprintf((u_char *)buf, "%O%Z", r->headers_out.content_length_n);
         value.data = (unsigned char *)buf;
@@ -304,6 +304,19 @@ ngx_http_modsecurity_resolv_header_connection(ngx_http_request_t *r, ngx_str_t n
     clcf = ngx_http_get_module_loc_conf(r, ngx_http_core_module);
     ctx = ngx_http_modsecurity_get_module_ctx(r);
 
+#if (NGX_HTTP_V2)
+    /*
+     * Connection and Keep-Alive are forbidden on HTTP/2 (RFC 9113 S8.2.2)
+     * and nginx never emits them on an h2 stream. Synthesizing them here
+     * would make the WAF inspect a header the client never actually
+     * receives, so a RESPONSE_HEADERS:Connection rule would false-positive
+     * on every HTTP/2 response.
+     */
+    if (r->stream) {
+        return 1;
+    }
+#endif
+
     if (r->headers_out.status == NGX_HTTP_SWITCHING_PROTOCOLS) {
         connection = "upgrade";
     } else if (r->keepalive) {
@@ -516,14 +529,20 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r)
 
     /*
      * NGINX always sends HTTP response with HTTP/1.1, except cases when
-     * HTTP V2 module is enabled, and request has been posted with HTTP/2.0.
+     * HTTP V2 module is enabled, and request has been posted with HTTP/2.0,
+     * or the request came in over HTTP/3.
      */
     http_response_ver = "HTTP 1.1";
 #if (NGX_HTTP_V2)
     if (r->stream) {
         http_response_ver = "HTTP 2.0";
     }
 #endif
+#if defined(nginx_version) && nginx_version >= 1025000
+    if (r->http_version == NGX_HTTP_VERSION_30) {
+        http_response_ver = "HTTP 3.0";
+    }
+#endif
 
     old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
     msc_process_response_headers(ctx->modsec_transaction, status, http_response_ver);
@@ -533,6 +552,30 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r)
         return ngx_http_next_header_filter(r);
     }
     if (ret > 0) {
+        if (r->headers_out.location != NULL) {
+            /*
+             * A redirect: process_intervention() already built the Location
+             * header (and cleared the stale entity headers describing the
+             * response we are discarding) directly on r->headers_out.
+             * ngx_http_filter_finalize_request() calls ngx_http_clean_header(),
+             * which memzeroes the *entire* headers_out struct -- Location
+             * included -- before nginx regenerates its own error page, which
+             * would silently drop the redirect. Forward the headers we
+             * already built through the normal chain instead.
+             *
+             * The original response body may already be in flight from the
+             * content handler; content_length_n = 0 and response_replaced
+             * tell the body filter to drop it rather than send it alongside
+             * this redirect.
+             */
+            ctx->intervention_triggered = 1;
+            ctx->response_replaced = 1;
+            r->headers_out.status = ret;
+            ngx_str_null(&r->headers_out.status_line);
+            r->headers_out.content_length_n = 0;
+            r->header_only = 1;
+            return ngx_http_next_header_filter(r);
+        }
         return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret);
     }
 

src/ngx_http_modsecurity_module.c

@@ -201,6 +201,23 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re
          *
          */
         ngx_http_clear_location(r);
+
+        /*
+         * The response being replaced by this redirect may already carry
+         * entity headers describing the body we are discarding. A body-less
+         * redirect must not advertise them (RFC 9110 SS15.4 / SS8.3-8.8).
+         */
+        ngx_http_clear_content_length(r);
+        ngx_http_clear_last_modified(r);
+        ngx_http_clear_etag(r);
+        ngx_http_clear_accept_ranges(r);
+        ngx_str_null(&r->headers_out.content_type);
+        r->headers_out.content_type_len = 0;
+        if (r->headers_out.content_encoding) {
+            r->headers_out.content_encoding->hash = 0;
+            r->headers_out.content_encoding = NULL;
+        }
+
         ngx_str_t a = ngx_string("");
 
         a.data = (unsigned char *)intervention.url;
@@ -302,7 +319,7 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r)
 
     ngx_http_set_ctx(r, ctx, ngx_http_modsecurity_module);
 
-    cln = ngx_pool_cleanup_add(r->pool, sizeof(ngx_http_modsecurity_ctx_t));
+    cln = ngx_pool_cleanup_add(r->pool, 0);
     if (cln == NULL)
     {
         dd("failed to create the ModSecurity context cleanup");
@@ -515,7 +532,7 @@ static ngx_command_t ngx_http_modsecurity_commands[] =  {
   },
   {
     ngx_string("modsecurity_transaction_id"),
-    NGX_HTTP_LOC_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_MAIN_CONF|NGX_CONF_1MORE,
+    NGX_HTTP_LOC_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_MAIN_CONF|NGX_CONF_TAKE1,
     ngx_conf_set_transaction_id,
     NGX_HTTP_LOC_CONF_OFFSET,
     0,

tests/modsecurity-response-redirect.t

@@ -0,0 +1,108 @@
+#!/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 WAF-triggered redirect (a phase:3 "redirect:" action)
+# replacing an already-populated response.
+#
+# ngx_http_modsecurity_header_filter() used to report a redirect via
+# ngx_http_filter_finalize_request(), which calls nginx's own
+# ngx_http_clean_header() and memzeroes the entire headers_out struct --
+# including the Location header ModSecurity had just built -- before nginx
+# regenerated its own generic, Location-less redirect page. The client never
+# actually got redirected, and the discarded response's entity headers
+# (Content-Length in particular) leaked into that generic page instead of
+# being cleared.
+
+###############################################################################
+
+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
+SecRule REQUEST_HEADERS:X-Redirect-Me "@streq 1" "id:104,phase:3,redirect:'/other',log"
+EOF
+
+# A large body: if the discarded response leaks through anyway, it dwarfs the
+# expected empty redirect body and its stale Content-Length would give it
+# away.
+$t->write_file('index.html', ('x' x 4096) . "\n");
+
+$t->run();
+$t->plan(5);
+
+###############################################################################
+
+my $plain = http_get('/');
+like($plain, qr/^HTTP\/1\.[01] 200/, 'plain request passes through unmodified');
+
+my $redirect = http(<<EOF);
+GET / HTTP/1.0
+Host: localhost
+X-Redirect-Me: 1
+
+EOF
+
+like($redirect, qr/^HTTP\/1\.[01] 302/m, 'WAF redirect: status is 302');
+# nginx absolutizes a relative Location value with the request's scheme and
+# Host, so match on the path suffix rather than requiring the literal
+# relative form the rule specified.
+like($redirect, qr{^Location:.*/other\r?$}m, 'WAF redirect: Location header present');
+like($redirect, qr/^Content-Length:\s*0\r?$/m,
+    'WAF redirect: no stale Content-Length from the discarded response');
+unlike($redirect, qr/x{100}/, 'WAF redirect: original response body was not sent');
+
+###############################################################################