diff --git a/config b/config index 0f05253e..0643da9b 100644 --- a/config +++ b/config @@ -117,11 +117,10 @@ if test -n "$ngx_module_link"; then ngx_module_type=HTTP_FILTER ngx_module_name="$ngx_addon_name" ngx_module_srcs="$ngx_addon_dir/src/ngx_http_modsecurity_module.c \ - $ngx_addon_dir/src/ngx_http_modsecurity_pre_access.c \ + $ngx_addon_dir/src/ngx_http_modsecurity_access.c \ $ngx_addon_dir/src/ngx_http_modsecurity_header_filter.c \ $ngx_addon_dir/src/ngx_http_modsecurity_body_filter.c \ $ngx_addon_dir/src/ngx_http_modsecurity_log.c \ - $ngx_addon_dir/src/ngx_http_modsecurity_rewrite.c \ " ngx_module_deps="$ngx_addon_dir/src/ddebug.h \ $ngx_addon_dir/src/ngx_http_modsecurity_common.h \ @@ -148,11 +147,10 @@ else NGX_ADDON_SRCS="\ $NGX_ADDON_SRCS \ $ngx_addon_dir/src/ngx_http_modsecurity_module.c \ - $ngx_addon_dir/src/ngx_http_modsecurity_pre_access.c \ + $ngx_addon_dir/src/ngx_http_modsecurity_access.c \ $ngx_addon_dir/src/ngx_http_modsecurity_header_filter.c \ $ngx_addon_dir/src/ngx_http_modsecurity_body_filter.c \ $ngx_addon_dir/src/ngx_http_modsecurity_log.c \ - $ngx_addon_dir/src/ngx_http_modsecurity_rewrite.c \ " NGX_ADDON_DEPS="\ diff --git a/src/ngx_http_modsecurity_rewrite.c b/src/ngx_http_modsecurity_access.c similarity index 60% rename from src/ngx_http_modsecurity_rewrite.c rename to src/ngx_http_modsecurity_access.c index ebc4742a..effa8a91 100644 --- a/src/ngx_http_modsecurity_rewrite.c +++ b/src/ngx_http_modsecurity_access.c @@ -22,9 +22,30 @@ #include "ngx_http_modsecurity_common.h" +void +ngx_http_modsecurity_request_read(ngx_http_request_t *r) +{ + ngx_http_modsecurity_ctx_t *ctx; + + ctx = ngx_http_modsecurity_get_module_ctx(r); + +#if defined(nginx_version) && nginx_version >= 8011 + r->main->count--; +#endif + + if (ctx->waiting_more_body) + { + ctx->waiting_more_body = 0; + r->write_event_handler = ngx_http_core_run_phases; + ngx_http_core_run_phases(r); + } +} + + ngx_int_t -ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) +ngx_http_modsecurity_access_handler(ngx_http_request_t *r) { + ngx_pool_t *old_pool; ngx_http_modsecurity_ctx_t *ctx; ngx_http_modsecurity_conf_t *mcf; @@ -44,7 +65,7 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) } */ - dd("catching a new _rewrite_ phase handler"); + dd("catching a new _access_ phase handler"); ctx = ngx_http_modsecurity_get_module_ctx(r); @@ -264,6 +285,181 @@ ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r) } } +#if 1 + + /* + * FIXME: + * In order to perform some tests, let's accept everything. + * + if (r->method != NGX_HTTP_GET && + r->method != NGX_HTTP_POST && r->method != NGX_HTTP_HEAD) { + dd("ModSecurity is not ready to deal with anything different from " \ + "POST, GET or HEAD"); + return NGX_DECLINED; + } + */ + + ctx = ngx_http_modsecurity_get_module_ctx(r); + + dd("recovering ctx: %p", ctx); + if (ctx == NULL) + { + dd("ctx is null; Nothing we can do, returning an error."); + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + + if (ctx->request_body_processed) { + // should we use r->internal or r->filter_finalize? + return NGX_DECLINED; + } + + if (ctx->intervention_triggered) { + return NGX_DECLINED; + } + + if (ctx->waiting_more_body == 1) + { + dd("waiting for more data before proceed. / count: %d", + r->main->count); + + return NGX_DONE; + } + + if (ctx->body_requested == 0) + { + ngx_int_t rc = NGX_OK; + + ctx->body_requested = 1; + + dd("asking for the request body, if any. Count: %d", + r->main->count); + /** + * TODO: Check if there is any benefit to use request_body_in_single_buf set to 1. + * + * saw some module using this request_body_in_single_buf + * but not sure what exactly it does, same for the others options below. + * + * r->request_body_in_single_buf = 1; + */ + r->request_body_in_single_buf = 1; + r->request_body_in_persistent_file = 1; + if (!r->request_body_in_file_only) { + // If the above condition fails, then the flag below will have been + // set correctly elsewhere. We need to set the flag here for other + // conditions (client_body_in_file_only not used but + // client_body_buffer_size is) + r->request_body_in_clean_file = 1; + } + + rc = ngx_http_read_client_request_body(r, + ngx_http_modsecurity_request_read); + if (rc == NGX_ERROR || rc >= NGX_HTTP_SPECIAL_RESPONSE) { +#if (nginx_version < 1002006) || \ + (nginx_version >= 1003000 && nginx_version < 1003009) + r->main->count--; +#endif + + return rc; + } + if (rc == NGX_AGAIN) + { + dd("nginx is asking us to wait for more data."); + + ctx->waiting_more_body = 1; + return NGX_DONE; + } + } + + if (ctx->waiting_more_body == 0) + { + int ret = 0; + int already_inspected = 0; + + dd("request body is ready to be processed"); + + r->write_event_handler = ngx_http_core_run_phases; + + ngx_chain_t *chain = r->request_body->bufs; + + /** + * TODO: Speed up the analysis by sending chunk while they arrive. + * + * Notice that we are waiting for the full request body to + * start to process it, it may not be necessary. We may send + * the chunks to ModSecurity while nginx keep calling this + * function. + */ + + if (r->request_body->temp_file != NULL) { + ngx_str_t file_path = r->request_body->temp_file->file.name; + const char *file_name = ngx_str_to_char(file_path, r->pool); + if (file_name == (char*)-1) { + return NGX_HTTP_INTERNAL_SERVER_ERROR; + } + /* + * Request body was saved to a file, probably we don't have a + * copy of it in memory. + */ + dd("request body inspection: file -- %s", file_name); + + msc_request_body_from_file(ctx->modsec_transaction, file_name); + + already_inspected = 1; + } else { + dd("inspection request body in memory."); + } + + while (chain && !already_inspected) + { + u_char *data = chain->buf->pos; + + msc_append_request_body(ctx->modsec_transaction, data, + chain->buf->last - data); + + if (chain->buf->last_buf) { + break; + } + chain = chain->next; + +/* XXX: chains are processed one-by-one, maybe worth to pass all chains and then call intervention() ? */ + + /** + * ModSecurity may perform stream inspection on this buffer, + * it may ask for a intervention in consequence of that. + * + */ + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); + if (ret > 0) { + return ret; + } + } + + /** + * At this point, all the request body was sent to ModSecurity + * and we want to make sure that all the request body inspection + * happened; consequently we have to check if ModSecurity have + * returned any kind of intervention. + */ + +/* XXX: once more -- is body can be modified ? content-length need to be adjusted ? */ + + old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); + msc_process_request_body(ctx->modsec_transaction); + ctx->request_body_processed = 1; + ngx_http_modsecurity_pcre_malloc_done(old_pool); + + ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); + if (r->error_page) { + return NGX_DECLINED; + } + if (ret > 0) { + return ret; + } + } + + dd("Nothing to add on the body inspection, reclaiming a NGX_DECLINED"); +#endif return NGX_DECLINED; } + diff --git a/src/ngx_http_modsecurity_common.h b/src/ngx_http_modsecurity_common.h index a4687e81..bacd5484 100644 --- a/src/ngx_http_modsecurity_common.h +++ b/src/ngx_http_modsecurity_common.h @@ -165,11 +165,7 @@ int ngx_http_modsecurity_store_ctx_header(ngx_http_request_t *r, ngx_str_t *name void ngx_http_modsecurity_log(void *log, const void* data); ngx_int_t ngx_http_modsecurity_log_handler(ngx_http_request_t *r); -/* ngx_http_modsecurity_pre_access.c */ -ngx_int_t ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r); - -/* ngx_http_modsecurity_rewrite.c */ -ngx_int_t ngx_http_modsecurity_rewrite_handler(ngx_http_request_t *r); - +/* ngx_http_modsecurity_access.c */ +ngx_int_t ngx_http_modsecurity_access_handler(ngx_http_request_t *r); #endif /* _NGX_HTTP_MODSECURITY_COMMON_H_INCLUDED_ */ diff --git a/src/ngx_http_modsecurity_module.c b/src/ngx_http_modsecurity_module.c index e8a5f4b7..a32ed686 100644 --- a/src/ngx_http_modsecurity_module.c +++ b/src/ngx_http_modsecurity_module.c @@ -551,8 +551,7 @@ ngx_module_t ngx_http_modsecurity_module = { static ngx_int_t ngx_http_modsecurity_init(ngx_conf_t *cf) { - ngx_http_handler_pt *h_rewrite; - ngx_http_handler_pt *h_preaccess; + ngx_http_handler_pt *h_access; ngx_http_handler_pt *h_log; ngx_http_core_main_conf_t *cmcf; int rc = 0; @@ -565,35 +564,19 @@ ngx_http_modsecurity_init(ngx_conf_t *cf) } /** * - * Seems like we cannot do this very same thing with - * NGX_HTTP_FIND_CONFIG_PHASE. it does not seems to - * be an array. Our next option is the REWRITE. - * - * TODO: check if we can hook prior to NGX_HTTP_REWRITE_PHASE phase. + * We want to process everything in the NGX_HTTP_ACCESS_PHASE because we need to allow + * ngx_http_limit_*_module to run * */ - h_rewrite = ngx_array_push(&cmcf->phases[NGX_HTTP_REWRITE_PHASE].handlers); - if (h_rewrite == NULL) - { - dd("Not able to create a new NGX_HTTP_REWRITE_PHASE handle"); - return NGX_ERROR; - } - *h_rewrite = ngx_http_modsecurity_rewrite_handler; - /** - * - * Processing the request body on the preaccess phase. - * - * TODO: check if hook into separated phases is the best thing to do. - * - */ - h_preaccess = ngx_array_push(&cmcf->phases[NGX_HTTP_PREACCESS_PHASE].handlers); - if (h_preaccess == NULL) + h_access = ngx_array_push(&cmcf->phases[NGX_HTTP_ACCESS_PHASE].handlers); + if (h_access == NULL) { - dd("Not able to create a new NGX_HTTP_PREACCESS_PHASE handle"); + dd("Not able to create a new NGX_HTTP_ACCESS_PHASE handle"); return NGX_ERROR; } - *h_preaccess = ngx_http_modsecurity_pre_access_handler; + *h_access = ngx_http_modsecurity_access_handler; + /** * Process the log phase. diff --git a/src/ngx_http_modsecurity_pre_access.c b/src/ngx_http_modsecurity_pre_access.c deleted file mode 100644 index 75ac45d4..00000000 --- a/src/ngx_http_modsecurity_pre_access.c +++ /dev/null @@ -1,236 +0,0 @@ -/* - * ModSecurity connector for nginx, 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 security@modsecurity.org. - * - */ - -#include - -#ifndef MODSECURITY_DDEBUG -#define MODSECURITY_DDEBUG 0 -#endif -#include "ddebug.h" - -#include "ngx_http_modsecurity_common.h" - -void -ngx_http_modsecurity_request_read(ngx_http_request_t *r) -{ - ngx_http_modsecurity_ctx_t *ctx; - - ctx = ngx_http_modsecurity_get_module_ctx(r); - -#if defined(nginx_version) && nginx_version >= 8011 - r->main->count--; -#endif - - if (ctx->waiting_more_body) - { - ctx->waiting_more_body = 0; - r->write_event_handler = ngx_http_core_run_phases; - ngx_http_core_run_phases(r); - } -} - - -ngx_int_t -ngx_http_modsecurity_pre_access_handler(ngx_http_request_t *r) -{ -#if 1 - ngx_pool_t *old_pool; - ngx_http_modsecurity_ctx_t *ctx; - ngx_http_modsecurity_conf_t *mcf; - - dd("catching a new _preaccess_ phase handler"); - - mcf = ngx_http_get_module_loc_conf(r, ngx_http_modsecurity_module); - if (mcf == NULL || mcf->enable != 1) - { - dd("ModSecurity not enabled... returning"); - return NGX_DECLINED; - } - /* - * FIXME: - * In order to perform some tests, let's accept everything. - * - if (r->method != NGX_HTTP_GET && - r->method != NGX_HTTP_POST && r->method != NGX_HTTP_HEAD) { - dd("ModSecurity is not ready to deal with anything different from " \ - "POST, GET or HEAD"); - return NGX_DECLINED; - } - */ - - ctx = ngx_http_modsecurity_get_module_ctx(r); - - dd("recovering ctx: %p", ctx); - - if (ctx == NULL) - { - dd("ctx is null; Nothing we can do, returning an error."); - return NGX_HTTP_INTERNAL_SERVER_ERROR; - } - - if (ctx->request_body_processed) { - // should we use r->internal or r->filter_finalize? - return NGX_DECLINED; - } - - if (ctx->intervention_triggered) { - return NGX_DECLINED; - } - - if (ctx->waiting_more_body == 1) - { - dd("waiting for more data before proceed. / count: %d", - r->main->count); - - return NGX_DONE; - } - - if (ctx->body_requested == 0) - { - ngx_int_t rc = NGX_OK; - - ctx->body_requested = 1; - - dd("asking for the request body, if any. Count: %d", - r->main->count); - /** - * TODO: Check if there is any benefit to use request_body_in_single_buf set to 1. - * - * saw some module using this request_body_in_single_buf - * but not sure what exactly it does, same for the others options below. - * - * r->request_body_in_single_buf = 1; - */ - r->request_body_in_single_buf = 1; - r->request_body_in_persistent_file = 1; - if (!r->request_body_in_file_only) { - // If the above condition fails, then the flag below will have been - // set correctly elsewhere. We need to set the flag here for other - // conditions (client_body_in_file_only not used but - // client_body_buffer_size is) - r->request_body_in_clean_file = 1; - } - - rc = ngx_http_read_client_request_body(r, - ngx_http_modsecurity_request_read); - if (rc == NGX_ERROR || rc >= NGX_HTTP_SPECIAL_RESPONSE) { -#if (nginx_version < 1002006) || \ - (nginx_version >= 1003000 && nginx_version < 1003009) - r->main->count--; -#endif - - return rc; - } - if (rc == NGX_AGAIN) - { - dd("nginx is asking us to wait for more data."); - - ctx->waiting_more_body = 1; - return NGX_DONE; - } - } - - if (ctx->waiting_more_body == 0) - { - int ret = 0; - int already_inspected = 0; - - dd("request body is ready to be processed"); - - r->write_event_handler = ngx_http_core_run_phases; - - ngx_chain_t *chain = r->request_body->bufs; - - /** - * TODO: Speed up the analysis by sending chunk while they arrive. - * - * Notice that we are waiting for the full request body to - * start to process it, it may not be necessary. We may send - * the chunks to ModSecurity while nginx keep calling this - * function. - */ - - if (r->request_body->temp_file != NULL) { - ngx_str_t file_path = r->request_body->temp_file->file.name; - const char *file_name = ngx_str_to_char(file_path, r->pool); - if (file_name == (char*)-1) { - return NGX_HTTP_INTERNAL_SERVER_ERROR; - } - /* - * Request body was saved to a file, probably we don't have a - * copy of it in memory. - */ - dd("request body inspection: file -- %s", file_name); - - msc_request_body_from_file(ctx->modsec_transaction, file_name); - - already_inspected = 1; - } else { - dd("inspection request body in memory."); - } - - while (chain && !already_inspected) - { - u_char *data = chain->buf->pos; - - msc_append_request_body(ctx->modsec_transaction, data, - chain->buf->last - data); - - if (chain->buf->last_buf) { - break; - } - chain = chain->next; - -/* XXX: chains are processed one-by-one, maybe worth to pass all chains and then call intervention() ? */ - - /** - * ModSecurity may perform stream inspection on this buffer, - * it may ask for a intervention in consequence of that. - * - */ - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); - if (ret > 0) { - return ret; - } - } - - /** - * At this point, all the request body was sent to ModSecurity - * and we want to make sure that all the request body inspection - * happened; consequently we have to check if ModSecurity have - * returned any kind of intervention. - */ - -/* XXX: once more -- is body can be modified ? content-length need to be adjusted ? */ - - old_pool = ngx_http_modsecurity_pcre_malloc_init(r->pool); - msc_process_request_body(ctx->modsec_transaction); - ctx->request_body_processed = 1; - ngx_http_modsecurity_pcre_malloc_done(old_pool); - - ret = ngx_http_modsecurity_process_intervention(ctx->modsec_transaction, r, 0); - if (r->error_page) { - return NGX_DECLINED; - } - if (ret > 0) { - return ret; - } - } - - dd("Nothing to add on the body inspection, reclaiming a NGX_DECLINED"); -#endif - return NGX_DECLINED; -} - diff --git a/tests/modsecurity-limits.t b/tests/modsecurity-limits.t new file mode 100644 index 00000000..14d4821a --- /dev/null +++ b/tests/modsecurity-limits.t @@ -0,0 +1,72 @@ +#!/usr/bin/perl + +# (C) Test for ModSecurity-nginx connector (limit_req ordering). + +############################################################################### + +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%% + + limit_req_zone $binary_remote_addr zone=limitzone:10m rate=2r/s; + limit_req_status 429; + + server { + listen 127.0.0.1:8080; + server_name localhost; + + modsecurity on; + limit_req zone=limitzone burst=1 nodelay; + + location /limit { + modsecurity_rules ' + SecRuleEngine On + SecRule REQUEST_URI "@rx .*" "id:1001,phase:1,log,deny,status:555,msg:\'Request reached ModSecurity\'" + '; + } + } +} +EOF + +$t->write_file("/limit", "limit test endpoint"); +$t->run(); +$t->plan(4); + +############################################################################### + +my $uri = '/limit'; + +my $res1 = http_get($uri); +my $res2 = http_get($uri); +my $res3 = http_get($uri); +my $res4 = http_get($uri); + +like($res1, qr/^HTTP.*555/, 'limitreq scoring 1 (Blocked by ModSecurity)'); +like($res2, qr/^HTTP.*555/, 'limitreq scoring 2 (Blocked by ModSecurity)'); +like($res3, qr/^HTTP.*429/, 'limitreq scoring 3 (limited by nginx)'); +like($res4, qr/^HTTP.*429/, 'limitreq scoring 4 (limited by nginx)');