Skip to content

Commit 21bc821

Browse files
defanatordefanator
authored
Merge pull request #241 from defanator/pr90-revisited
Fixed auditlog in case of internal redirect
2 parents 2a13e33 + ea89849 commit 21bc821

10 files changed

+221
-16
lines changed

Diff for: ‎CHANGES

+2
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
v1.0.x - YYYY-MMM-DD (To be released)
22
-------------------------------------
33

4+
- Fix auditlog in case of internal redirect
5+
[Issue #90 - @AirisX, @defanator]
46
- Fix nginx sends response without headers
57
[Issue #238 - @airween, @defanator]
68
- Fix nginx not clearing body cache (caused by incomplete fix for #187)

Diff for: ‎src/ngx_http_modsecurity_body_filter.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
145145
int ret;
146146

147147
msc_append_response_body(ctx->modsec_transaction, data, chain->buf->last - data);
148-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
148+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
149149
if (ret > 0) {
150150
return ngx_http_filter_finalize_request(r,
151151
&ngx_http_modsecurity_module, ret);
@@ -163,7 +163,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in)
163163

164164
/* XXX: I don't get how body from modsec being transferred to nginx's buffer. If so - after adjusting of nginx's
165165
XXX: body we can proceed to adjust body size (content-length). see xslt_body_filter() for example */
166-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
166+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
167167
if (ret > 0) {
168168
return ret;
169169
}

Diff for: ‎src/ngx_http_modsecurity_common.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,7 @@ typedef struct {
9797
unsigned waiting_more_body:1;
9898
unsigned body_requested:1;
9999
unsigned processed:1;
100+
unsigned logged:1;
100101
unsigned intervention_triggered:1;
101102
} ngx_http_modsecurity_ctx_t;
102103

@@ -136,7 +137,7 @@ typedef struct {
136137
extern ngx_module_t ngx_http_modsecurity_module;
137138

138139
/* ngx_http_modsecurity_module.c */
139-
int ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r);
140+
int ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r, ngx_int_t early_log);
140141
ngx_http_modsecurity_ctx_t *ngx_http_modsecurity_create_ctx(ngx_http_request_t *r);
141142
char *ngx_str_to_char(ngx_str_t a, ngx_pool_t *p);
142143
ngx_pool_t *ngx_http_modsecurity_pcre_malloc_init(ngx_pool_t *pool);

Diff for: ‎src/ngx_http_modsecurity_header_filter.c

+1-1
Original file line numberDiff line numberDiff line change
@@ -526,7 +526,7 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r)
526526
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
527527
msc_process_response_headers(ctx->modsec_transaction, status, http_response_ver);
528528
ngx_http_modsecurity_pcre_malloc_done(old_pool);
529-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
529+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
530530
if (r->error_page) {
531531
return ngx_http_next_header_filter(r);
532532
}

Diff for: ‎src/ngx_http_modsecurity_log.c

+5
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,11 @@ ngx_http_modsecurity_log_handler(ngx_http_request_t *r)
6767
return NGX_ERROR;
6868
}
6969

70+
if (ctx->logged) {
71+
dd("already logged earlier");
72+
return NGX_OK;
73+
}
74+
7075
dd("calling msc_process_logging for %p", ctx);
7176
old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool);
7277
msc_process_logging(ctx->modsec_transaction);

Diff for: ‎src/ngx_http_modsecurity_module.c

+22-1
Original file line numberDiff line numberDiff line change
@@ -130,17 +130,24 @@ ngx_inline char *ngx_str_to_char(ngx_str_t a, ngx_pool_t *p)
130130

131131

132132
ngx_inline int
133-
ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r)
133+
ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r, ngx_int_t early_log)
134134
{
135135
char *log = NULL;
136136
ModSecurityIntervention intervention;
137137
intervention.status = 200;
138138
intervention.url = NULL;
139139
intervention.log = NULL;
140140
intervention.disruptive = 0;
141+
ngx_http_modsecurity_ctx_t *ctx = NULL;
141142

142143
dd("processing intervention");
143144

145+
ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity_module);
146+
if (ctx == NULL)
147+
{
148+
return NGX_HTTP_INTERNAL_SERVER_ERROR;
149+
}
150+
144151
if (msc_intervention(transaction, &intervention) == 0) {
145152
dd("nothing to do");
146153
return 0;
@@ -200,6 +207,20 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re
200207

201208
if (intervention.status != 200)
202209
{
210+
/**
211+
* FIXME: this will bring proper response code to audit log in case
212+
* when e.g. error_page redirect was triggered, but there still won't be another
213+
* required pieces like response headers etc.
214+
*
215+
*/
216+
msc_update_status_code(ctx->modsec_transaction, intervention.status);
217+
218+
if (early_log) {
219+
dd("intervention -- calling log handler manually with code: %d", intervention.status);
220+
ngx_http_modsecurity_log_handler(r);
221+
ctx->logged = 1;
222+
}
223+
203224
if (r->header_sent)
204225
{
205226
dd("Headers are already sent. Cannot perform the redirection at this point.");

Diff for: ‎src/ngx_http_modsecurity_pre_access.c

+2-2
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r)
193193
* it may ask for a intervention in consequence of that.
194194
*
195195
*/
196-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
196+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
197197
if (ret > 0) {
198198
return ret;
199199
}
@@ -212,7 +212,7 @@ ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r)
212212
msc_process_request_body(ctx->modsec_transaction);
213213
ngx_http_modsecurity_pcre_malloc_done(old_pool);
214214

215-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
215+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0);
216216
if (r->error_page) {
217217
return NGX_DECLINED;
218218
}

Diff for: ‎src/ngx_http_modsecurity_rewrite.c

+3-3
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
115115
*
116116
*/
117117
dd("Processing intervention with the connection information filled in");
118-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
118+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1);
119119
if (ret > 0) {
120120
ctx->intervention_triggered = 1;
121121
return ret;
@@ -156,7 +156,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
156156
ngx_http_modsecurity_pcre_malloc_done(old_pool);
157157

158158
dd("Processing intervention with the transaction information filled in (uri, method and version)");
159-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
159+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1);
160160
if (ret > 0) {
161161
ctx->intervention_triggered = 1;
162162
return ret;
@@ -205,7 +205,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r)
205205
msc_process_request_headers(ctx->modsec_transaction);
206206
ngx_http_modsecurity_pcre_malloc_done(old_pool);
207207
dd("Processing intervention with the request headers information filled in");
208-
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r);
208+
ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 1);
209209
if (r->error_page) {
210210
return NGX_DECLINED;
211211
}

Diff for: ‎tests/modsecurity-config-auditlog.t

+8-6
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ http {
6565
SecRule ARGS "@streq root" "id:21,phase:1,auditlog,status:302,redirect:http://www.modsecurity.org"
6666
SecDebugLog %%TESTDIR%%/auditlog-debug-root.txt
6767
SecDebugLogLevel 9
68-
SecAuditEngine On
68+
SecAuditEngine RelevantOnly
6969
SecAuditLogParts AB
7070
SecAuditLog %%TESTDIR%%/auditlog-root.txt
7171
SecAuditLogType Serial
@@ -76,9 +76,10 @@ http {
7676
location /subfolder1/subfolder2 {
7777
modsecurity_rules '
7878
SecRule ARGS "@streq subfolder2" "id:41,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org"
79+
SecRule ARGS "@streq subfolder1" "id:42,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org"
7980
SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder2.txt
8081
SecDebugLogLevel 9
81-
SecAuditEngine On
82+
SecAuditEngine RelevantOnly
8283
SecAuditLogParts AB
8384
SecResponseBodyAccess On
8485
SecAuditLog %%TESTDIR%%/auditlog-subfolder2.txt
@@ -93,7 +94,7 @@ http {
9394
SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder1.txt
9495
SecDebugLogLevel 9
9596
SecAuditLogParts AB
96-
SecAuditEngine On
97+
SecAuditEngine RelevantOnly
9798
SecAuditLog %%TESTDIR%%/auditlog-subfolder1.txt
9899
SecAuditLogType Serial
99100
SecAuditLogStorageDir %%TESTDIR%%/
@@ -104,10 +105,11 @@ http {
104105
modsecurity_rules '
105106
SecResponseBodyAccess On
106107
SecRule ARGS "@streq subfolder4" "id:61,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org"
107-
SecRule ARGS "@streq subfolder4withE" "id:2,phase:1,ctl:auditLogParts=+E,auditlog"
108+
SecRule ARGS "@streq subfolder3" "id:62,phase:1,status:302,auditlog,redirect:http://www.modsecurity.org"
109+
SecRule ARGS "@streq subfolder4withE" "id:63,phase:1,ctl:auditLogParts=+E,auditlog"
108110
SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder4.txt
109111
SecDebugLogLevel 9
110-
SecAuditEngine On
112+
SecAuditEngine RelevantOnly
111113
SecAuditLogParts AB
112114
SecAuditLog %%TESTDIR%%/auditlog-subfolder4.txt
113115
SecAuditLogType Serial
@@ -121,7 +123,7 @@ http {
121123
SecDebugLog %%TESTDIR%%/auditlog-debug-subfolder3.txt
122124
SecDebugLogLevel 9
123125
SecAuditLogParts AB
124-
SecAuditEngine On
126+
SecAuditEngine RelevantOnly
125127
SecAuditLog %%TESTDIR%%/auditlog-subfolder3.txt
126128
SecAuditLogType Serial
127129
SecAuditLogStorageDir %%TESTDIR%%/

Diff for: ‎tests/modsecurity-config-custom-error-page.t

+174
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,174 @@
1+
#!/usr/bin/perl
2+
3+
#
4+
# ModSecurity, http://www.modsecurity.org/
5+
# Copyright (c) 2015 Trustwave Holdings, Inc. (http://www.trustwave.com/)
6+
#
7+
# You may not use this file except in compliance with
8+
# the License. You may obtain a copy of the License at
9+
#
10+
# http://www.apache.org/licenses/LICENSE-2.0
11+
#
12+
# If any of the files related to licensing are missing or if you have any
13+
# other questions related to licensing please contact Trustwave Holdings, Inc.
14+
# directly using the email address [email protected].
15+
#
16+
17+
18+
# Tests for ModSecurity module.
19+
20+
###############################################################################
21+
22+
use warnings;
23+
use strict;
24+
25+
use Test::More;
26+
27+
BEGIN { use FindBin; chdir($FindBin::Bin); }
28+
29+
use lib 'lib';
30+
use Test::Nginx;
31+
32+
###############################################################################
33+
34+
select STDERR; $| = 1;
35+
select STDOUT; $| = 1;
36+
37+
my $t = Test::Nginx->new()->has(qw/http/);
38+
39+
$t->write_file_expand('nginx.conf', <<'EOF');
40+
41+
%%TEST_GLOBALS%%
42+
43+
daemon off;
44+
45+
events {
46+
}
47+
48+
http {
49+
%%TEST_GLOBALS_HTTP%%
50+
51+
server {
52+
listen 127.0.0.1:8080;
53+
server_name s1;
54+
55+
error_page 403 /403.html;
56+
57+
location /403.html {
58+
root %%TESTDIR%%/http;
59+
internal;
60+
}
61+
62+
location / {
63+
modsecurity on;
64+
modsecurity_rules '
65+
SecRuleEngine On
66+
SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny"
67+
SecDebugLog %%TESTDIR%%/auditlog-debug-local.txt
68+
SecDebugLogLevel 9
69+
SecAuditEngine RelevantOnly
70+
SecAuditLogParts ABIJDEFHZ
71+
SecAuditLog %%TESTDIR%%/auditlog-local.txt
72+
SecAuditLogType Serial
73+
SecAuditLogStorageDir %%TESTDIR%%/
74+
';
75+
}
76+
}
77+
78+
server {
79+
listen 127.0.0.1:8080;
80+
server_name s2;
81+
82+
modsecurity on;
83+
modsecurity_rules '
84+
SecRuleEngine On
85+
SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny"
86+
SecDebugLog %%TESTDIR%%/auditlog-debug-global.txt
87+
SecDebugLogLevel 9
88+
SecAuditEngine RelevantOnly
89+
SecAuditLogParts ABIJDEFHZ
90+
SecAuditLog %%TESTDIR%%/auditlog-global.txt
91+
SecAuditLogType Serial
92+
SecAuditLogStorageDir %%TESTDIR%%/
93+
';
94+
95+
error_page 403 /403.html;
96+
97+
location /403.html {
98+
modsecurity off;
99+
root %%TESTDIR%%/http;
100+
internal;
101+
}
102+
103+
location / {
104+
}
105+
}
106+
}
107+
EOF
108+
109+
my $index_txt = "This is the index page.";
110+
my $custom_txt = "This is a custom error page.";
111+
112+
$t->write_file("/index.html", $index_txt);
113+
mkdir($t->testdir() . '/http');
114+
$t->write_file("/http/403.html", $custom_txt);
115+
116+
$t->run();
117+
$t->plan(10);
118+
119+
###############################################################################
120+
121+
my $d = $t->testdir();
122+
123+
my $t1;
124+
my $t2;
125+
my $t3;
126+
my $t4;
127+
128+
# Performing requests to a server with ModSecurity enabled at location context
129+
$t1 = http_get_host('s1', '/index.html?what=root');
130+
$t2 = http_get_host('s1', '/index.html?what=other');
131+
132+
# Performing requests to a server with ModSecurity enabled at server context
133+
$t3 = http_get_host('s2', '/index.html?what=root');
134+
$t4 = http_get_host('s2', '/index.html?what=other');
135+
136+
my $local = do {
137+
local $/ = undef;
138+
open my $fh, "<", "$d/auditlog-local.txt"
139+
or die "could not open: $!";
140+
<$fh>;
141+
};
142+
143+
my $global = do {
144+
local $/ = undef;
145+
open my $fh, "<", "$d/auditlog-global.txt"
146+
or die "could not open: $!";
147+
<$fh>;
148+
};
149+
150+
like($t1, qr/$custom_txt/, 'ModSecurity at location / root');
151+
like($t2, qr/$index_txt/, 'ModSecurity at location / other');
152+
like($local, qr/what=root/, 'ModSecurity at location / root present in auditlog');
153+
unlike($local, qr/what=other/, 'ModSecurity at location / other not present in auditlog');
154+
155+
like($t3, qr/$custom_txt/, 'ModSecurity at server / root');
156+
like($t4, qr/$index_txt/, 'ModSecurity at server / other');
157+
like($global, qr/what=root/, 'ModSecurity at server / root present in auditlog');
158+
unlike($global, qr/what=other/, 'ModSecurity at server / other not present in auditlog');
159+
160+
like($local, qr/Access denied with code 403/, 'ModSecurity at location / 403 in auditlog');
161+
like($global, qr/Access denied with code 403/, 'ModSecurity at server / 403 in auditlog');
162+
163+
###############################################################################
164+
165+
sub http_get_host {
166+
my ($host, $url) = @_;
167+
return http(<<EOF);
168+
GET $url HTTP/1.0
169+
Host: $host
170+
171+
EOF
172+
}
173+
174+
###############################################################################

0 commit comments

Comments
 (0)