From 62ead2a12cece59f1489b3272a5bde5572d08600 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Mon, 12 May 2014 17:07:59 +0400 Subject: [PATCH 1/7] Fixed segmentation fault if http context is not defined. --- nginx/modsecurity/ngx_http_modsecurity.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index 7c139531..41f1e9b5 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -938,6 +938,12 @@ ngx_http_modsecurity_init(ngx_conf_t *cf) static ngx_int_t ngx_http_modsecurity_init_process(ngx_cycle_t *cycle) { + if (ngx_http_cycle_get_module_main_conf(cycle, ngx_http_modsecurity) + == NULL) + { + return NGX_OK; + } + /* must set log hook here cf->log maybe changed */ modsecSetLogHook(cycle->log, modsecLog); modsecInitProcess(); From f16d98bb75832d8697d5e58cee45f570e026a537 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Wed, 14 May 2014 14:45:24 +0400 Subject: [PATCH 2/7] Removed unneeded and invalid initialization. --- nginx/modsecurity/ngx_http_modsecurity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index 215f65fc..5e0df81d 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -1097,7 +1097,7 @@ ngx_http_modsecurity_init_process(ngx_cycle_t *cycle) static ngx_int_t ngx_http_modsecurity_handler(ngx_http_request_t *r) { - ngx_int_t rc = NULL; + ngx_int_t rc; ngx_http_modsecurity_ctx_t *ctx = NULL; ngx_http_modsecurity_loc_conf_t *cf = NULL; From 1baeaef7fcdf0aaee6c433d4137bfea7a96ff818 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Thu, 15 May 2014 15:56:44 +0400 Subject: [PATCH 3/7] Obtain port from r->connection->local_sockaddr. This eliminates segfaults caused by unset (NULL) r->port_start and non-NULL r->port_end. In fact, r->port_start is always NULL, so it is useless to rely on this pointer. --- nginx/modsecurity/ngx_http_modsecurity.c | 34 ++++++++++++++++++++---- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index 5e0df81d..f5058306 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -279,9 +279,13 @@ ngx_http_modsecurity_load_request(ngx_http_request_t *r) { ngx_http_modsecurity_ctx_t *ctx; request_rec *req; - ngx_str_t str; size_t root; ngx_str_t path; + ngx_uint_t port; + struct sockaddr_in *sin; +#if (NGX_HAVE_INET6) + struct sockaddr_in6 *sin6; +#endif ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity); req = ctx->req; @@ -324,10 +328,30 @@ ngx_http_modsecurity_load_request(ngx_http_request_t *r) req->parsed_uri.path = (char *)ngx_pstrdup0(r->pool, &r->uri); req->parsed_uri.is_initialized = 1; - str.data = r->port_start; - str.len = r->port_end - r->port_start; - req->parsed_uri.port = ngx_atoi(str.data, str.len); - req->parsed_uri.port_str = (char *)ngx_pstrdup0(r->pool, &str); + switch (r->connection->local_sockaddr->sa_family) { + +#if (NGX_HAVE_INET6) + case AF_INET6: + sin6 = (struct sockaddr_in6 *) r->connection->local_sockaddr; + port = ntohs(sin6->sin6_port); + break; +#endif + +#if (NGX_HAVE_UNIX_DOMAIN) + case AF_UNIX: + port = 0; + break; +#endif + + default: /* AF_INET */ + sin = (struct sockaddr_in *) r->connection->local_sockaddr; + port = ntohs(sin->sin_port); + break; + } + + req->parsed_uri.port = port; + req->parsed_uri.port_str = ngx_pnalloc(r->pool, sizeof("65535")); + (void) ngx_sprintf((u_char *)req->parsed_uri.port_str, "%ui%c", port, '\0'); req->parsed_uri.query = r->args.len ? req->args : NULL; req->parsed_uri.dns_looked_up = 0; From 77feb2b76112de3113b721778f92925600280eb1 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Mon, 19 May 2014 15:55:51 +0400 Subject: [PATCH 4/7] Fixed ngx_http_modsecurity_header_filter() to call next header filter eventually. Returning of NGX_OK there may lead to request's hang, in particular when HEAD method is used. --- nginx/modsecurity/ngx_http_modsecurity.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index f5058306..9926c71b 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -1294,7 +1294,7 @@ ngx_http_modsecurity_header_filter(ngx_http_request_t *r) { "modSecurity: header filter"); r->filter_need_in_memory = 1; - return NGX_OK; + return ngx_http_next_header_filter(r); } #endif #ifndef NGX_HTTP_MODSECURITY_PREACCESS_HANDLER_ONLY From a20c7a70917a6d2157273705bb9bd4eb546025b4 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Mon, 19 May 2014 16:12:12 +0400 Subject: [PATCH 5/7] Fixed ngx_http_modsecurity_body_filter() to properly handle filters chain. Body filter is a wrong place to call ngx_http_next_header_filter() - that function is intended to be called from header filters. In case when there were no errors, body filter should call ngx_http_next_body_filter() eventually. While here, also removed useless second return. --- nginx/modsecurity/ngx_http_modsecurity.c | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index 9926c71b..ada9a12c 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -1409,19 +1409,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) r->headers_out.content_length = NULL; /* header filter will set this */ } - r->header_sent = 0; - - rc = ngx_http_next_header_filter(r); - if (rc == NGX_ERROR || rc > NGX_OK) { - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, - "ModSecurity: ngx_http_next_header_filter not NGX_OK."); - - return ngx_http_filter_finalize_request(r, &ngx_http_modsecurity, rc); - } - return ngx_http_next_body_filter(r, out); - - return NGX_OK; } #endif From 5d9f0d85b270cfb2b12f8d1c9e7c6dc87219559e Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Mon, 2 Jun 2014 11:48:54 +0400 Subject: [PATCH 6/7] All ngx_log_debug() calls changed to ngx_log_debug0(). --- nginx/modsecurity/ngx_http_modsecurity.c | 34 ++++++++++++------------ 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index ada9a12c..ac90617d 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -563,7 +563,7 @@ ngx_http_modsecurity_load_request_body(ngx_http_request_t *r) ngx_http_modsecurity_ctx_t *ctx; ngx_chain_t *chain; - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: loading request body."); ctx = ngx_http_get_module_ctx(r, ngx_http_modsecurity); @@ -580,7 +580,7 @@ ngx_http_modsecurity_load_request_body(ngx_http_request_t *r) #ifdef MOVE_REQUEST_CHAIN_TO_MODSEC if (move_chain_to_brigade(chain, ctx->brigade, r->pool, 1) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed to move chain to brigade."); return NGX_ERROR; @@ -589,7 +589,7 @@ ngx_http_modsecurity_load_request_body(ngx_http_request_t *r) r->request_body = NULL; #else if (copy_chain_to_brigade(chain, ctx->brigade, r->pool, 1) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed to copy chain to brigade."); return NGX_ERROR; @@ -636,7 +636,7 @@ ngx_http_modsecurity_save_request_body(ngx_http_request_t *r) r->headers_in.content_length_n = content_length; - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: Content length: %O, Content length n: %O", content_length, r->headers_in.content_length_n); #else @@ -890,17 +890,17 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r) ctx = ngx_http_get_module_pool_ctx(r, ngx_http_modsecurity); - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: starting to process the request..."); if (ngx_http_modsecurity_load_request(r) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while loading the request."); return NGX_HTTP_INTERNAL_SERVER_ERROR; } if (ngx_http_modsecurity_load_headers_in(r) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while loading the headers."); return NGX_HTTP_INTERNAL_SERVER_ERROR; } @@ -908,7 +908,7 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r) rc = modsecProcessRequestHeaders(ctx->req); rc = ngx_http_modsecurity_status(r, rc); if (rc != NGX_DECLINED) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while processing the request headers"); return rc; } @@ -928,18 +928,18 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r) modsecContextState(ctx->req) != MODSEC_DISABLED) { load_request_body = 1; - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: request body will be processed..."); } if (load_request_body == 1) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: loading request body..."); rc = ngx_http_modsecurity_load_request_body(r); if (rc != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while loading the request body."); return NGX_HTTP_INTERNAL_SERVER_ERROR; } @@ -947,13 +947,13 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r) } - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: processing request body..."); rc = modsecProcessRequestBody(ctx->req); rc = ngx_http_modsecurity_status(r, rc); if (rc != NGX_DECLINED) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: finalizing the connection after process the " \ "request body."); @@ -962,13 +962,13 @@ ngx_http_modsecurity_process_request(ngx_http_request_t *r) if (load_request_body == 1) { if (ngx_http_modsecurity_save_request_body(r) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while saving the request body"); return NGX_HTTP_INTERNAL_SERVER_ERROR; } if (ngx_http_modsecurity_save_headers_in(r) != NGX_OK) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: failed while saving the headers in"); return NGX_HTTP_INTERNAL_SERVER_ERROR; } @@ -1181,7 +1181,7 @@ ngx_http_modsecurity_handler(ngx_http_request_t *r) { } if (ctx->body_requested == 0) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: asking for the request body, if any."); ctx->body_requested = 1; @@ -1377,7 +1377,7 @@ ngx_http_modsecurity_body_filter(ngx_http_request_t *r, ngx_chain_t *in) rc = move_brigade_to_chain(ctx->brigade, &out, r->pool); if (rc == NGX_ERROR) { - ngx_log_debug(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: Problems moving brigade to chain"); return ngx_http_filter_finalize_request(r, From d0ead529d997695f92cb9671026eab4cbf999600 Mon Sep 17 00:00:00 2001 From: Andrei Belov Date: Tue, 3 Jun 2014 17:30:04 +0400 Subject: [PATCH 7/7] Explicitly set log object to r->connection->log in preaccess phase handler. This change fixes a number of scenarios when ModSecurity's log entries may be written to the wrong file descriptors. In particular, there was an issue with almost any configuration using nginx cache features (proxy_cache, fastcgi_cache, etc) when garbage from ModSecurity logs has been sent to the control socket used for communication between nginx master process and auxiliary processes (workers, cache manager, cache loader). Described behavior was observed with nginx/1.7.0, modsecurity/2.8.0 and OWASP CRS v2.2.9. --- nginx/modsecurity/ngx_http_modsecurity.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/nginx/modsecurity/ngx_http_modsecurity.c b/nginx/modsecurity/ngx_http_modsecurity.c index ac90617d..9f5e80a9 100644 --- a/nginx/modsecurity/ngx_http_modsecurity.c +++ b/nginx/modsecurity/ngx_http_modsecurity.c @@ -1125,6 +1125,8 @@ ngx_http_modsecurity_handler(ngx_http_request_t *r) { ngx_http_modsecurity_ctx_t *ctx = NULL; ngx_http_modsecurity_loc_conf_t *cf = NULL; + modsecSetLogHook(r->connection->log, modsecLog); + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, r->connection->log, 0, "ModSec: Catching a new access phase handler. Count: %d", r->main->count);