From 6ec8fd9aeaa7533b5601f1cafd1d00538d7b68d4 Mon Sep 17 00:00:00 2001 From: AirisX Date: Tue, 23 Jan 2018 10:35:39 +0400 Subject: [PATCH 1/3] Fixed auditlog in case of internal redirect --- src/ngx_http_modsecurity_common.h | 1 + src/ngx_http_modsecurity_log.c | 5 +++++ src/ngx_http_modsecurity_module.c | 10 ++++++++++ 3 files changed, 16 insertions(+) diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index 7d8ebcd..05e8b0f 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -80,6 +80,7 @@ typedef struct { unsigned waiting_more_body:1; unsigned body_requested:1; unsigned processed:1; + unsigned logged:1; } ngx_http_modsecurity_ctx_t; diff --git a/src/ngx_http_modsecurity_log.c b/src/ngx_http_modsecurity_log.c index f6454b5..2897097 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 6d6ee7a..396f1f6 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -137,9 +137,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; @@ -194,6 +201,9 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re if (intervention.status != 200) { + 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."); From 798396fb48cf0e46bd1226bda71c62fb8b2bbc6b Mon Sep 17 00:00:00 2001 From: AirisX Date: Fri, 16 Feb 2018 15:26:08 +0400 Subject: [PATCH 2/3] Changed SecAuditEngine and added missing rules in modsecurity-config-auditlog.t --- tests/modsecurity-config-auditlog.t | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) 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%%/ From a36fc1487bdddba0dee4d450ce98554eb5234815 Mon Sep 17 00:00:00 2001 From: AirisX Date: Fri, 16 Feb 2018 15:27:36 +0400 Subject: [PATCH 3/3] Added test for custom error page --- tests/modsecurity-config-custom-error-page.t | 241 +++++++++++++++++++ 1 file changed, 241 insertions(+) create mode 100644 tests/modsecurity-config-custom-error-page.t diff --git a/tests/modsecurity-config-custom-error-page.t b/tests/modsecurity-config-custom-error-page.t new file mode 100644 index 0000000..e2363a6 --- /dev/null +++ b/tests/modsecurity-config-custom-error-page.t @@ -0,0 +1,241 @@ +#!/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 localhost; + + 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 AB + SecAuditLog %%TESTDIR%%/auditlog-local.txt + SecAuditLogType Serial + SecAuditLogStorageDir %%TESTDIR%%/ + '; + } + } + + server { + listen 127.0.0.1:8081; + server_name localhost; + + 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 AB + 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(8); + +############################################################################### + +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('/index.html?what=root'); +$t2 = http_get('/index.html?what=other'); + +# Performing requests to a server with ModSecurity enabled at server context +$t3 = http_get2('/index.html?what=root'); +$t4 = http_get2('/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'); + +############################################################################### + +sub http_get2($;%) { + my ($url, %extra) = @_; + return http2(<new( + Proto => 'tcp', + PeerAddr => '127.0.0.1:' . port(8081) + ) + or die "Can't connect to nginx: $!\n"; + + log_out($request); + $s->print($request); + + select undef, undef, undef, $extra{sleep} if $extra{sleep}; + return '' if $extra{aborted}; + + if ($extra{body}) { + log_out($extra{body}); + $s->print($extra{body}); + } + + alarm(0); + }; + alarm(0); + if ($@) { + log_in("died: $@"); + return undef; + } + + return $s; +} + +sub http_end2($;%) { + my ($s) = @_; + my $reply; + + eval { + local $SIG{ALRM} = sub { die "timeout\n" }; + local $SIG{PIPE} = sub { die "sigpipe\n" }; + alarm(8); + + local $/; + $reply = $s->getline(); + + alarm(0); + }; + alarm(0); + if ($@) { + log_in("died: $@"); + return undef; + } + + log_in($reply); + return $reply; +} + +###############################################################################