diff --git a/src/ngx_http_modsecurity_access.c b/src/ngx_http_modsecurity_access.c index effa8a91..8528d8e3 100644 --- a/src/ngx_http_modsecurity_access.c +++ b/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"); diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 0c28e3c1..2210a960 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/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; } } } diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 03b8764d..5b98355e 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/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 diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index d3d9624d..fa4502ab 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -290,7 +290,7 @@ 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); @@ -298,6 +298,17 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r) 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,7 +317,7 @@ 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; @@ -314,7 +325,7 @@ ngx_http_modsecurity_create_ctx(ngx_http_request_t *r) #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 diff --git a/tests/modsecurity-response-body-deny.t b/tests/modsecurity-response-body-deny.t new file mode 100644 index 00000000..ece2370a --- /dev/null +++ b/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'); + +###############################################################################