diff --git a/CHANGES b/CHANGES index cbc6a37..3af6980 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,8 @@ v1.0.x - YYYY-MMM-DD (To be released) ------------------------------------- + - Fix auditlog in case of internal redirect + [Issue #90 - @AirisX, @defanator] - Fix nginx sends response without headers [Issue #238 - @airween, @defanator] - Fix nginx not clearing body cache (caused by incomplete fix for #187) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 1b85df5..725f986 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -145,7 +145,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) int ret; msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); if (ret > 0) { return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); @@ -163,7 +163,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) /* 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); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); if (ret > 0) { return ret; } diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index 79958b5..727e18a 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -97,6 +97,7 @@ typedef struct { unsigned waiting_more_body:1; unsigned body_requested:1; unsigned processed:1; + unsigned logged:1; unsigned intervention_triggered:1; } ngx_http_modsecurity_ctx_t; @@ -136,7 +137,7 @@ typedef struct { extern ngx_module_t ngx_http_modsecurity_module; /* ngx_http_modsecurity_module.c */ -int ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r); +int ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r, ngx_int_t early_log); ngx_http_modsecurity_ctx_t *ngx_http_modsecurity_create_ctx(ngx_http_request_t *r); char *ngx_str_to_char(ngx_str_t a, ngx_pool_t *p); ngx_pool_t *ngx_http_modsecurity_pcre_malloc_init(ngx_pool_t *pool); diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 51990c7..c36be19 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -526,7 +526,7 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); msc_process_response_headers(ctx->modsec_transaction, status, http_response_ver); ngx_http_modsecurity_pcre_malloc_done(old_pool); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); if (r->error_page) { return ngx_http_next_header_filter(r); } diff --git a/src/ngx_http_modsecurity_log.c b/src/ngx_http_modsecurity_log.c index 5546596..d713a65 100644 --- a/src/ngx_http_modsecurity_log.c +++ b/src/ngx_http_modsecurity_log.c @@ -67,6 +67,11 @@ ngx_http_modsecurity_log_handler(ngx_http_request_t *r) return NGX_ERROR; } + if (ctx->logged) { + dd("already logged earlier"); + return NGX_OK; + } + dd("calling msc_process_logging for %p", ctx); old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); msc_process_logging(ctx->modsec_transaction); diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index cc3f02a..b6f33f5 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -130,7 +130,7 @@ ngx_inline char *ngx_str_to_char(ngx_str_t a, ngx_pool_t *p) ngx_inline int -ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r) +ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r, ngx_int_t early_log) { char *log = NULL; ModSecurityIntervention intervention; @@ -138,9 +138,16 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re intervention.url = NULL; intervention.log = NULL; intervention.disruptive = 0; + ngx_http_modsecurity_ctx_t *ctx = NULL; dd("processing intervention"); + ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module); + if (ctx == NULL) + { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + if (msc_intervention(transaction, &intervention) == 0) { dd("nothing to do"); return 0; @@ -200,6 +207,20 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re if (intervention.status != 200) { + /** + * FIXME: this will bring proper response code to audit log in case + * when e.g. error_page redirect was triggered, but there still won't be another + * required pieces like response headers etc. + * + */ + msc_update_status_code(ctx->modsec_transaction, intervention.status); + + if (early_log) { + dd("intervention -- calling log handler manually with code: %d", intervention.status); + ngx_http_modsecurity_log_handler(r); + ctx->logged = 1; + } + if (r->header_sent) { dd("Headers are already sent. Cannot perform the redirection at this point."); diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c index 779b71e..abb7d3e 100644 --- a/src/ngx_http_modsecurity_pre_access.c +++ b/src/ngx_http_modsecurity_pre_access.c @@ -193,7 +193,7 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) * it may ask for a intervention in consequence of that. * */ - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); if (ret > 0) { return ret; } @@ -212,7 +212,7 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) msc_process_request_body(ctx->modsec_transaction); ngx_http_modsecurity_pcre_malloc_done(old_pool); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); if (r->error_page) { return NGX_DECLINED; } diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_rewrite.c index 4316db8..f6f8d41 100644 --- a/src/ngx_http_modsecurity_rewrite.c +++ b/src/ngx_http_modsecurity_rewrite.c @@ -115,7 +115,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) * */ dd("Processing intervention with the connection information filled in"); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1); if (ret > 0) { ctx->intervention_triggered = 1; return ret; @@ -156,7 +156,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) ngx_http_modsecurity_pcre_malloc_done(old_pool); dd("Processing intervention with the transaction information filled in (uri, method and version)"); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1); if (ret > 0) { ctx->intervention_triggered = 1; return ret; @@ -205,7 +205,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) msc_process_request_headers(ctx->modsec_transaction); ngx_http_modsecurity_pcre_malloc_done(old_pool); dd("Processing intervention with the request headers information filled in"); - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1); if (r->error_page) { return NGX_DECLINED; } diff --git a/tests/modsecurity-config-auditlog.t b/tests/modsecurity-config-auditlog.t index 92a8291..0094b3c 100644 --- a/tests/modsecurity-config-auditlog.t +++ b/tests/modsecurity-config-auditlog.t @@ -65,7 +65,7 @@ http { SecRule ARGS "@streq root" "id:21,phase:1,auditlog,status:302,redirect:http://www.modsecurity.org" SecDebugLog %%TESTDIR%%/auditlog-debug-root.txt SecDebugLogLevel 9 - SecAuditEngine On + SecAuditEngine RelevantOnly SecAuditLogParts AB SecAuditLog %%TESTDIR%%/auditlog-root.txt SecAuditLogType Serial @@ -76,9 +76,10 @@ http { location /subfolder1/subfolder2 { modsecurity_rules ' SecRule ARGS "@streq subfolder2" "id:41,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org" + SecRule ARGS "@streq subfolder1" "id:42,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org" SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder2.txt SecDebugLogLevel 9 - SecAuditEngine On + SecAuditEngine RelevantOnly SecAuditLogParts AB SecResponseBodyAccess On SecAuditLog %%TESTDIR%%/auditlog-subfolder2.txt @@ -93,7 +94,7 @@ http { SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder1.txt SecDebugLogLevel 9 SecAuditLogParts AB - SecAuditEngine On + SecAuditEngine RelevantOnly SecAuditLog %%TESTDIR%%/auditlog-subfolder1.txt SecAuditLogType Serial SecAuditLogStorageDir %%TESTDIR%%/ @@ -104,10 +105,11 @@ http { modsecurity_rules ' SecResponseBodyAccess On SecRule ARGS "@streq subfolder4" "id:61,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org" - SecRule ARGS "@streq subfolder4withE" "id:2,phase:1,ctl:auditLogParts=+E,auditlog" + SecRule ARGS "@streq subfolder3" "id:62,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org" + SecRule ARGS "@streq subfolder4withE" "id:63,phase:1,ctl:auditLogParts=+E,auditlog" SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder4.txt SecDebugLogLevel 9 - SecAuditEngine On + SecAuditEngine RelevantOnly SecAuditLogParts AB SecAuditLog %%TESTDIR%%/auditlog-subfolder4.txt SecAuditLogType Serial @@ -121,7 +123,7 @@ http { SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder3.txt SecDebugLogLevel 9 SecAuditLogParts AB - SecAuditEngine On + SecAuditEngine RelevantOnly SecAuditLog %%TESTDIR%%/auditlog-subfolder3.txt SecAuditLogType Serial SecAuditLogStorageDir %%TESTDIR%%/ diff --git a/tests/modsecurity-config-custom-error-page.t b/tests/modsecurity-config-custom-error-page.t new file mode 100644 index 0000000..a8f5862 --- /dev/null +++ b/tests/modsecurity-config-custom-error-page.t @@ -0,0 +1,174 @@ +#!/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. +# + + +# Tests for ModSecurity module. + +############################################################################### + +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 s1; + + error_page 403 /403.html; + + location /403.html { + root %%TESTDIR%%/http; + internal; + } + + location / { + modsecurity on; + modsecurity_rules ' + SecRuleEngine On + SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny" + SecDebugLog %%TESTDIR%%/auditlog-debug-local.txt + SecDebugLogLevel 9 + SecAuditEngine RelevantOnly + SecAuditLogParts ABIJDEFHZ + SecAuditLog %%TESTDIR%%/auditlog-local.txt + SecAuditLogType Serial + SecAuditLogStorageDir %%TESTDIR%%/ + '; + } + } + + server { + listen 127.0.0.1:8080; + server_name s2; + + modsecurity on; + modsecurity_rules ' + SecRuleEngine On + SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny" + SecDebugLog %%TESTDIR%%/auditlog-debug-global.txt + SecDebugLogLevel 9 + SecAuditEngine RelevantOnly + SecAuditLogParts ABIJDEFHZ + SecAuditLog %%TESTDIR%%/auditlog-global.txt + SecAuditLogType Serial + SecAuditLogStorageDir %%TESTDIR%%/ + '; + + error_page 403 /403.html; + + location /403.html { + modsecurity off; + root %%TESTDIR%%/http; + internal; + } + + location / { + } + } +} +EOF + +my $index_txt = "This is the index page."; +my $custom_txt = "This is a custom error page."; + +$t->write_file("/index.html", $index_txt); +mkdir($t->testdir() . '/http'); +$t->write_file("/http/403.html", $custom_txt); + +$t->run(); +$t->plan(10); + +############################################################################### + +my $d = $t->testdir(); + +my $t1; +my $t2; +my $t3; +my $t4; + +# Performing requests to a server with ModSecurity enabled at location context +$t1 = http_get_host('s1', '/index.html?what=root'); +$t2 = http_get_host('s1', '/index.html?what=other'); + +# Performing requests to a server with ModSecurity enabled at server context +$t3 = http_get_host('s2', '/index.html?what=root'); +$t4 = http_get_host('s2', '/index.html?what=other'); + +my $local = do { + local $/ = undef; + open my $fh, "<", "$d/auditlog-local.txt" + or die "could not open: $!"; + <$fh>; +}; + +my $global = do { + local $/ = undef; + open my $fh, "<", "$d/auditlog-global.txt" + or die "could not open: $!"; + <$fh>; +}; + +like($t1, qr/$custom_txt/, 'ModSecurity at location / root'); +like($t2, qr/$index_txt/, 'ModSecurity at location / other'); +like($local, qr/what=root/, 'ModSecurity at location / root present in auditlog'); +unlike($local, qr/what=other/, 'ModSecurity at location / other not present in auditlog'); + +like($t3, qr/$custom_txt/, 'ModSecurity at server / root'); +like($t4, qr/$index_txt/, 'ModSecurity at server / other'); +like($global, qr/what=root/, 'ModSecurity at server / root present in auditlog'); +unlike($global, qr/what=other/, 'ModSecurity at server / other not present in auditlog'); + +like($local, qr/Access denied with code 403/, 'ModSecurity at location / 403 in auditlog'); +like($global, qr/Access denied with code 403/, 'ModSecurity at server / 403 in auditlog'); + +############################################################################### + +sub http_get_host { + my ($host, $url) = @_; + return http(<