diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 0c28e3c1..64b16cb1 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -58,6 +58,48 @@ 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_buf_t *b; + ngx_chain_t *cl, **ll; + + ll = ∈ + + for (cl = in; cl; cl = cl->next) { + b = cl->buf; + + /* + * Empty the buffer and drop its memory/file flags so that it + * becomes a special buffer (see ngx_buf_special()). The write + * filter rejects a zero-size non-special buffer with "zero + * size buf in writer", so a buffer that carries none of + * flush/sync/last_buf is unlinked from the chain instead. + */ + b->pos = b->last; + b->file_last = b->file_pos; + b->in_file = 0; + b->temporary = 0; + b->memory = 0; + b->mmap = 0; + + if (ngx_buf_special(b)) { + *ll = cl; + ll = &cl->next; + } + } + + *ll = NULL; + + 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..45d3ee2a 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -100,6 +100,22 @@ 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; + /* + * Set by ngx_http_modsecurity_process_intervention() when the + * intervention it just applied was a redirect, i.e. it installed a + * Location header on r->headers_out. Callers branch on this rather than + * on r->headers_out.location, which the original response may already + * have carried. + */ + unsigned intervention_redirect: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 03b8764d..d7b45318 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,24 @@ 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); + /* + * Connection and Keep-Alive are forbidden on HTTP/2 (RFC 9113 S8.2.2) + * and HTTP/3 (RFC 9114 S4.2), and nginx never emits them there. + * 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 h2/h3 response. + */ +#if (NGX_HTTP_V2) + if (r->stream) { + return 1; + } +#endif +#if defined(nginx_version) && nginx_version >= 1025000 + if (r->http_version == NGX_HTTP_VERSION_30) { + return 1; + } +#endif + if (r->headers_out.status == NGX_HTTP_SWITCHING_PROTOCOLS) { connection = "upgrade"; } else if (r->keepalive) { @@ -516,7 +534,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 +543,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); @@ -533,6 +557,32 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) return ngx_http_next_header_filter(r); } if (ret > 0) { + if (ctx->intervention_redirect) { + /* + * 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. + * (Branch on the ctx flag, not on r->headers_out.location: the + * original response may itself have carried a Location.) + * 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/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index 1e55ade0..597a4e26 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -156,6 +156,9 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re return NGX_HTTP_INTERNAL_SERVER_ERROR; } + /* Reflects only the intervention applied by this call, if any. */ + ctx->intervention_redirect = 0; + if (msc_intervention(transaction, &intervention) == 0) { dd("nothing to do"); return 0; @@ -187,14 +190,43 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re if (intervention.url != NULL) { + u_char *url; + size_t len; + dd("intervention -- redirecting to: %s with status code: %d", intervention.url, intervention.status); if (r->header_sent) { dd("Headers are already sent. Cannot perform the redirection at this point."); + free(intervention.url); return -1; } + /* + * The callers ignore the result for an error_page response (nginx + * is already serving the outcome of an earlier intervention), so + * do not rewrite that response's headers either. + */ + if (r->error_page) + { + free(intervention.url); + return intervention.status; + } + + /* + * libmodsecurity hands us a strdup()'d URL. Copy it into the + * request pool, since the header outlives this function, and + * release the original like intervention.log above. + */ + len = ngx_strlen(intervention.url); + url = ngx_pnalloc(r->pool, len); + if (url == NULL) { + free(intervention.url); + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + ngx_memcpy(url, intervention.url, len); + free(intervention.url); + /** * Not sure if it sane to do this indepent of the phase * but, here we go... @@ -207,15 +239,38 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re * */ ngx_http_clear_location(r); - ngx_str_t a = ngx_string(""); - a.data = (unsigned char *)intervention.url; - a.len = strlen(intervention.url); + /* + * 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; + r->headers_out.content_type_lowcase = NULL; + r->headers_out.content_type_hash = 0; + ngx_str_null(&r->headers_out.charset); + if (r->headers_out.content_encoding) { + r->headers_out.content_encoding->hash = 0; + r->headers_out.content_encoding = NULL; + } + if (r->headers_out.content_range) { + r->headers_out.content_range->hash = 0; + r->headers_out.content_range = NULL; + } ngx_table_elt_t *location = NULL; location = ngx_list_push(&r->headers_out.headers); + if (location == NULL) { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } ngx_str_set(&location->key, "Location"); - location->value = a; + location->value.data = url; + location->value.len = len; r->headers_out.location = location; r->headers_out.location->hash = 1; @@ -223,6 +278,10 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re ngx_http_modsecurity_store_ctx_header(r, &location->key, &location->value); #endif + /* Let the audit log / phase 5 see the status actually sent. */ + msc_update_status_code(ctx->modsec_transaction, intervention.status); + ctx->intervention_redirect = 1; + return intervention.status; } @@ -308,7 +367,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"); @@ -521,7 +580,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, 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(<