From d90eed54f3877c6b0f90d62ca03f65eded46be64 Mon Sep 17 00:00:00 2001 From: Marios Levogiannis Date: Tue, 23 Nov 2021 11:39:28 +0200 Subject: [PATCH 1/2] Tests: Add test cases for all phases in custom error page test --- tests/modsecurity-config-custom-error-page.t | 82 ++++++++++++-------- 1 file changed, 51 insertions(+), 31 deletions(-) diff --git a/tests/modsecurity-config-custom-error-page.t b/tests/modsecurity-config-custom-error-page.t index a8f5862..b8f6214 100644 --- a/tests/modsecurity-config-custom-error-page.t +++ b/tests/modsecurity-config-custom-error-page.t @@ -55,7 +55,7 @@ http { error_page 403 /403.html; location /403.html { - root %%TESTDIR%%/http; + alias %%TESTDIR%%/403.html; internal; } @@ -63,7 +63,11 @@ http { modsecurity on; modsecurity_rules ' SecRuleEngine On - SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny" + SecResponseBodyAccess On + SecRule ARGS:phase1 "@streq BAD" "id:10,phase:1,auditlog,status:403,deny" + SecRule ARGS:phase2 "@streq BAD" "id:11,phase:2,auditlog,status:403,deny" + SecRule ARGS:phase3 "@streq BAD" "id:12,phase:3,auditlog,status:403,deny" + SecRule ARGS:phase4 "@streq BAD" "id:13,phase:4,auditlog,status:403,drop" SecDebugLog %%TESTDIR%%/auditlog-debug-local.txt SecDebugLogLevel 9 SecAuditEngine RelevantOnly @@ -82,7 +86,11 @@ http { modsecurity on; modsecurity_rules ' SecRuleEngine On - SecRule ARGS "@streq root" "id:10,phase:1,auditlog,status:403,deny" + SecResponseBodyAccess On + SecRule ARGS:phase1 "@streq BAD" "id:10,phase:1,auditlog,status:403,deny" + SecRule ARGS:phase2 "@streq BAD" "id:11,phase:2,auditlog,status:403,deny" + SecRule ARGS:phase3 "@streq BAD" "id:12,phase:3,auditlog,status:403,deny" + SecRule ARGS:phase4 "@streq BAD" "id:13,phase:4,auditlog,status:403,drop" SecDebugLog %%TESTDIR%%/auditlog-debug-global.txt SecDebugLogLevel 9 SecAuditEngine RelevantOnly @@ -96,7 +104,7 @@ http { location /403.html { modsecurity off; - root %%TESTDIR%%/http; + alias %%TESTDIR%%/403.html; internal; } @@ -107,31 +115,28 @@ http { EOF my $index_txt = "This is the index page."; -my $custom_txt = "This is a custom error page."; +my $error_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->write_file("/403.html", $error_txt); +$t->todo_alerts(); $t->run(); -$t->plan(10); +$t->plan(32); ############################################################################### 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'); +like(http_get_host('s1', '/?phase1=BAD'), qr/$error_txt/, 'location context, phase 1, error page'); +like(http_get_host('s1', '/?phase1=GOOD'), qr/$index_txt/, 'location context, phase 1, index page'); +like(http_get_host('s1', '/?phase2=BAD'), qr/$error_txt/, 'location context, phase 2, error page'); +like(http_get_host('s1', '/?phase2=GOOD'), qr/$index_txt/, 'location context, phase 2, index page'); +like(http_get_host('s1', '/?phase3=BAD'), qr/$error_txt/, 'location context, phase 3, error page'); +like(http_get_host('s1', '/?phase3=GOOD'), qr/$index_txt/, 'location context, phase 3, index page'); +is(http_get_host('s1', '/?phase4=BAD'), '', 'location context, phase 4, drop'); +like(http_get_host('s1', '/?phase4=GOOD'), qr/$index_txt/, 'location context, phase 4, index page'); my $local = do { local $/ = undef; @@ -140,6 +145,25 @@ my $local = do { <$fh>; }; +like($local, qr/phase1=BAD/, 'location context, phase 1, BAD in auditlog'); +unlike($local, qr/phase1=GOOD/, 'location context, phase 1, GOOD not in auditlog'); +like($local, qr/phase2=BAD/, 'location context, phase 2, BAD in auditlog'); +unlike($local, qr/phase2=GOOD/, 'location context, phase 2, GOOD not in auditlog'); +like($local, qr/phase3=BAD/, 'location context, phase 3, BAD in auditlog'); +unlike($local, qr/phase3=GOOD/, 'location context, phase 3, GOOD not in auditlog'); +like($local, qr/phase4=BAD/, 'location context, phase 4, BAD in auditlog'); +unlike($local, qr/phase4=GOOD/, 'location context, phase 4, GOOD not in auditlog'); + +# Performing requests to a server with ModSecurity enabled at server context +like(http_get_host('s2', '/?phase1=BAD'), qr/$error_txt/, 'server context, phase 1, error page'); +like(http_get_host('s2', '/?phase1=GOOD'), qr/$index_txt/, 'server context, phase 1, index page'); +like(http_get_host('s2', '/?phase2=BAD'), qr/$error_txt/, 'server context, phase 2, error page'); +like(http_get_host('s2', '/?phase2=GOOD'), qr/$index_txt/, 'server context, phase 2, index page'); +like(http_get_host('s2', '/?phase3=BAD'), qr/$error_txt/, 'server context, phase 3, error page'); +like(http_get_host('s2', '/?phase3=GOOD'), qr/$index_txt/, 'server context, phase 3, index page'); +is(http_get_host('s2', '/?phase4=BAD'), '', 'server context, phase 4, drop'); +like(http_get_host('s2', '/?phase4=GOOD'), qr/$index_txt/, 'server context, phase 4, index page'); + my $global = do { local $/ = undef; open my $fh, "<", "$d/auditlog-global.txt" @@ -147,18 +171,14 @@ my $global = do { <$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'); +like($global, qr/phase1=BAD/, 'server context, phase 1, BAD in auditlog'); +unlike($global, qr/phase1=GOOD/, 'server context, phase 1, GOOD not in auditlog'); +like($global, qr/phase2=BAD/, 'server context, phase 2, BAD in auditlog'); +unlike($global, qr/phase2=GOOD/, 'server context, phase 2, GOOD not in auditlog'); +like($global, qr/phase3=BAD/, 'server context, phase 3, BAD in auditlog'); +unlike($global, qr/phase3=GOOD/, 'server context, phase 3, GOOD not in auditlog'); +like($global, qr/phase4=BAD/, 'server context, phase 4, BAD in auditlog'); +unlike($global, qr/phase4=GOOD/, 'server context, phase 4, GOOD not in auditlog'); ############################################################################### From 02249daa30ecc7036ed424f55341c42a28d323fc Mon Sep 17 00:00:00 2001 From: Marios Levogiannis Date: Wed, 24 Nov 2021 12:36:28 +0200 Subject: [PATCH 2/2] Fix phase 2 and 3 audit logging in case of internal redirect Phase 2 and 3 log entries were not logged in the audit log in case of an internal redirect. Only phase 1 and 4 ones were logged, the former because early logging was explicitly enabled and the latter because the internal redirect does not work in this phase. This commit unconditionally enables early logging in all ModSecurity phases. Since the Nginx log phase may not be executed in case of an intervention, the process intervention function is the only place which is guaranteed to call the log handler in such a case. Co-authored-by: Dimitris Kolotouros Co-authored-by: Thanos Giannopoulos --- src/ngx_http_modsecurity_body_filter.c | 4 ++-- src/ngx_http_modsecurity_common.h | 2 +- src/ngx_http_modsecurity_header_filter.c | 2 +- src/ngx_http_modsecurity_log.c | 1 + src/ngx_http_modsecurity_module.c | 9 +++------ src/ngx_http_modsecurity_pre_access.c | 4 ++-- src/ngx_http_modsecurity_rewrite.c | 6 +++--- 7 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/ngx_http_modsecurity_body_filter.c b/src/ngx_http_modsecurity_body_filter.c index 86bccc7..3e5fdd1 100644 --- a/src/ngx_http_modsecurity_body_filter.c +++ b/src/ngx_http_modsecurity_body_filter.c @@ -147,7 +147,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, 0); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity_module, ret); @@ -165,7 +165,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, 0); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { return ret; } diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index 11fdc2d..3b91cca 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -137,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, ngx_int_t early_log); +int ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r); 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); #if (NGX_PCRE2) diff --git a/src/ngx_http_modsecurity_header_filter.c b/src/ngx_http_modsecurity_header_filter.c index 257e7fd..7f9b6ad 100644 --- a/src/ngx_http_modsecurity_header_filter.c +++ b/src/ngx_http_modsecurity_header_filter.c @@ -528,7 +528,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, 0); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); 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 167b2d3..e1e0e4c 100644 --- a/src/ngx_http_modsecurity_log.c +++ b/src/ngx_http_modsecurity_log.c @@ -78,6 +78,7 @@ ngx_http_modsecurity_log_handler(ngx_http_request_t *r) old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); msc_process_logging(ctx->modsec_transaction); ngx_http_modsecurity_pcre_malloc_done(old_pool); + ctx->logged = 1; return NGX_OK; } diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index c90b3f6..e6f4be3 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -137,7 +137,7 @@ ngx_inline char *ngx_str_to_char(ngx_str_t a, ngx_pool_t *p) int -ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r, ngx_int_t early_log) +ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_request_t *r) { char *log = NULL; ModSecurityIntervention intervention; @@ -222,11 +222,8 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re */ 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; - } + dd("intervention -- calling log handler manually with code: %d", intervention.status); + ngx_http_modsecurity_log_handler(r); if (r->header_sent) { diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c index ea5c021..7805ee6 100644 --- a/src/ngx_http_modsecurity_pre_access.c +++ b/src/ngx_http_modsecurity_pre_access.c @@ -195,7 +195,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, 0); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { return ret; } @@ -214,7 +214,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, 0); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (r->error_page) { return NGX_DECLINED; } diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_rewrite.c index 926cf70..f8c3ae4 100644 --- a/src/ngx_http_modsecurity_rewrite.c +++ b/src/ngx_http_modsecurity_rewrite.c @@ -117,7 +117,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, 1); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { ctx->intervention_triggered = 1; return ret; @@ -166,7 +166,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, 1); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (ret > 0) { ctx->intervention_triggered = 1; return ret; @@ -215,7 +215,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, 1); + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r); if (r->error_page) { return NGX_DECLINED; }