Hi @JakubOnderka,

thanks for this PR!

I'm sure this patch can be useful for many users, but please consider the following:

log the whole message in case request is blocked into nginx error log. But the same information is also logged into modsecurity audit logs

this depends on some circumstances. Eg. by default audit log contains the transaction related information only if the status code is 4XX or 5XX except 404 (see SecAuditLogRelevantStatus).

If someone uses Core Rule Set in anomaly scoring mode, and the transaction's score value does not reach the threshold, then those information will be lost (I mean the triggered rules).

Moreover consider if someone uses some IPS/IDS (eg. fail2ban) which uses only the error.log (as I know there is no any plugin for fail2ban which can use audit.log) - then this configuration could be unusable.

I support any new feature, but we must notice users what do they do.

so logs can grow pretty fast in case of DDoS or scanning attacks.

If the log level is lower than info in Nginx's configuration (eg. no level), then the result is almost the same (like this PR's result).

This patch adds new option modsecurity_error_log that accepts on or off option. on is default that logs the whole message to error log, but it can be turned off.

A side note, but hope others will be check this PR too and write their opinions: modsecurity_error_log refers to me that where is the log, I mean the path. May be some more informative name would be better - eg. modsecurity_use_error_log, or similar.

It also adds new variable $modsecurity_status that contains status code from modsecurity.

It would be nice to see a real example of its use. While you want to add a new configuration keyword and a new variable, please add their documentation into README.md, below the Usage section (you can do that within this PR - not a separated one). If you make the documentation for modsecurity_error_log (or the other name - we will see it), then please add the side effects too what I explained above.

And thanks again!

If the log level is lower than info in Nginx's configuration (eg. no level), then the result is almost the same (like this PR's result).

Hi @airween
I tested on nginx with error log level. But I still saw logs in error_log. Correct level is lower then error. I did'nt see any logs in error_log when I tested with critical log level like you said.

I want to keep nginx log level error but I don't want to generate duplicate ModSecurity log when I using audit log. So this feature would be useful for this case.

Thanks

Hi @airween

I just modified disable error log part of pull request as below. This protect current state. Is this ok?

diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h
index cde48a5..3fa8a70 100644
--- a/src/ngx_http_modsecurity_common.h
+++ b/src/ngx_http_modsecurity_common.h
@@ -118,6 +118,7 @@ typedef struct {
 void *rules_set;
 
 ngx_flag_t enable;
+ ngx_flag_t disable_error_log;
 #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
 ngx_flag_t sanity_checks_enabled;
 #endif
diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c
index e8a5f4b..28c7228 100644
--- a/src/ngx_http_modsecurity_module.c
+++ b/src/ngx_http_modsecurity_module.c
@@ -146,6 +146,7 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re
 intervention.log = NULL;
 intervention.disruptive = 0;
 ngx_http_modsecurity_ctx_t *ctx = NULL;
+ ngx_http_modsecurity_conf_t *mcf;
 
 dd("processing intervention");
 
@@ -160,12 +161,19 @@ ngx_http_modsecurity_process_intervention (Transaction *transaction, ngx_http_re
 return 0;
 }
 
- log = intervention.log;
- if (intervention.log == NULL) {
- log = "(no log message was specified)";
+ mcf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module);
+ if (mcf == NULL) {
+ return NGX_HTTP_INTERNAL_SERVER_ERROR;
 }
 
- ngx_log_error(NGX_LOG_ERR, (ngx_log_t *)r->connection->log, 0, "%s", log);
+ // logging to nginx error log can be disable by setting `modsecurity_disable_error_log` to on
+ if (!mcf->disable_error_log) {
+ log = intervention.log;
+ if (intervention.log == NULL) {
+ log = "(no log message was specified)";
+ }
+ ngx_log_error(NGX_LOG_ERR, (ngx_log_t *)r->connection->log, 0, "%s", log);
+ }
 
 if (intervention.log != NULL) {
 free(intervention.log);
@@ -513,6 +521,14 @@ static ngx_command_t ngx_http_modsecurity_commands[] = {
 0,
 NULL
 },
+ {
+ ngx_string("modsecurity_disable_error_log"),
+ NGX_HTTP_LOC_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_MAIN_CONF|NGX_CONF_FLAG,
+ ngx_conf_set_flag_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_modsecurity_conf_t, disable_error_log),
+ NULL
+ },
 ngx_null_command
 };
 
@@ -724,6 +740,7 @@ ngx_http_modsecurity_create_conf(ngx_conf_t *cf)
 conf->rules_set = msc_create_rules_set();
 conf->pool = cf->pool;
 conf->transaction_id = NGX_CONF_UNSET_PTR;
+ conf->disable_error_log = NGX_CONF_UNSET;
 #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
 conf->sanity_checks_enabled = NGX_CONF_UNSET;
 #endif
@@ -763,6 +780,7 @@ ngx_http_modsecurity_merge_conf(ngx_conf_t *cf, void *parent, void *child)
 
 ngx_conf_merge_value(c->enable, p->enable, 0);
 ngx_conf_merge_ptr_value(c->transaction_id, p->transaction_id, NULL);
+ ngx_conf_merge_value(c->disable_error_log, p->disable_error_log, 0);
 #if defined(MODSECURITY_SANITY_CHECKS) && (MODSECURITY_SANITY_CHECKS)
 ngx_conf_merge_value(c->sanity_checks_enabled, p->sanity_checks_enabled, 0);
 #endif

+1 - the hardcoded logging to error is very problematic as it cannot be controlled.
IMO it should just be changed to info, or even debug (or an option to define) - or even removed, as logging is done by modsec itself in the auditlog. AFAIK the httpd module doesn't do this either?

@JakubOnderka do you mind updating the patch so it works with 1.0.4? or @fatihusta do you mind making a new PR with your patch?

Closing as opened a new version in #360.