From 657b1afdf675f68d96611ed1cf1ee0a8006c9a52 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Mon, 27 Jul 2026 19:56:46 -0300 Subject: [PATCH 1/2] fix: correctness gaps in response header/intervention handling Content-Length: 0 was never forwarded to the WAF (the check was content_length_n > 0, dropping the legitimate zero-length case), so a RESPONSE_HEADERS:Content-Length rule could never match an empty body. The synthesized Server header used sizeof() on a string literal, which includes the trailing NUL, embedding a literal \0 byte into the header value handed to the WAF -- breaking exact-match rules on RESPONSE_HEADERS:Server. The synthesized Connection (and Keep-Alive) response header was fed to the WAF unconditionally, including on HTTP/2, where nginx never actually sends either header (RFC 9113 SS8.2.2). A RESPONSE_HEADERS:Connection rule would false-positive on every HTTP/2 response. The response protocol string passed to msc_process_response_headers() only distinguished HTTP/1.1 and HTTP/2.0, defaulting to HTTP/1.1 for HTTP/3 requests -- unlike the request side, which already reports the real protocol generically. When a phase:3 "redirect:" action turns an already-populated 200 response into a 3xx, only the Location header was synthesized; the discarded response's Content-Length/Content-Type/Last-Modified/ETag/ Content-Encoding/Accept-Ranges were left in place, describing a body that is no longer sent (RFC 9110 SS15.4 / SS8.3-8.8). modsecurity_transaction_id was declared NGX_CONF_1MORE but its handler only ever reads the first argument, so a config with extra arguments was silently accepted with the extras discarded instead of rejected. ngx_http_modsecurity_create_ctx() allocated an unused sizeof(ctx) data buffer per request via ngx_pool_cleanup_add()'s size argument; nothing reads it since cln->data is set to the already-existing ctx pointer separately. Matches the ngx_pool_cleanup_add(cf->pool, 0) pattern already used elsewhere in this file. Co-Authored-By: Claude Sonnet 5 --- src/ngx_http_modsecurity_header_filter.c | 27 ++++++++++++++++++++---- src/ngx_http_modsecurity_module.c | 21 ++++++++++++++++-- 2 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 03b8764d..b85dc048 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/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,7 +529,8 @@ 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) @@ -524,6 +538,11 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) 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); diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index d3d9624d..d7d784d9 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/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, From 471a2a54843bb8f560758a7e75b146db2243ab29 Mon Sep 17 00:00:00 2001 From: Felipe Zipitria Date: Mon, 27 Jul 2026 21:25:18 -0300 Subject: [PATCH 2/2] fix: WAF-triggered redirects were silently dropped ngx_http_modsecurity_header_filter() reported a phase-3/4 "redirect:" intervention via ngx_http_filter_finalize_request(), which calls nginx's own ngx_http_clean_header() and memzeroes the entire headers_out struct -- Location included -- before nginx regenerated its own generic redirect page. The client never actually got redirected: it received a Location-less "302 Found" boilerplate page, and the discarded response's Content-Length leaked into it instead of being cleared (which made the entity-header-clearing fix from the previous commit a no-op on the one path it was meant to cover). Confirmed empirically: a redirect: action fired from access.c (phase 1/2, returns straight from the phase handler) worked correctly; the identical rule at phase:3 (via header_filter.c) did not -- the response was byte-for-byte identical whether or not the entity-header clearing fix was present, because ngx_http_clean_header() wipes everything anyway. nginx's own fix for this same problem (ngx_http_send_error_page(), used by the `error_page` directive) isn't usable here: it calls the static ngx_http_send_special_response(), which isn't part of the public module API. Fix: when process_intervention() has built a Location header, skip ngx_http_filter_finalize_request() and forward the already-correct headers_out through the normal chain instead. The original response body may already be flowing from the content handler by this point, so body_filter() now drops it (ctx->response_replaced) rather than sending it alongside the redirect. Co-Authored-By: Claude Sonnet 5 --- src/ngx_http_modsecurity_body_filter.c | 19 ++++ src/ngx_http_modsecurity_common.h | 8 ++ src/ngx_http_modsecurity_header_filter.c | 24 +++++ tests/modsecurity-response-redirect.t | 108 +++++++++++++++++++++++ 4 files changed, 159 insertions(+) create mode 100644 tests/modsecurity-response-redirect.t diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 0c28e3c1..bbbe11fe 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/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); } diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index 9f79b5ba..b3f0f278 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/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; diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index b85dc048..cdae4ea2 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -552,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); } diff --git a/tests/modsecurity-response-redirect.t b/tests/modsecurity-response-redirect.t new file mode 100644 index 00000000..4b34de9f --- /dev/null +++ b/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(<