owasp-modsecurity · defanator · Feb 15, 2021 · Feb 11, 2021
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -130,17 +130,24 @@ 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;
     intervention.status = 200;
     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
@@ -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
@@ -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
@@ -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
@@ -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 [email protected].
+#
+
+
+# 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(<<EOF);
+GET $url HTTP/1.0
+Host: $host
+
+EOF
+}
+
+###############################################################################