From ec4ce3ee899913ce4708b994914c953d47fbc9ca Mon Sep 17 00:00:00 2001 From: "Thibault \"bui\" Koechlin" Date: Mon, 6 Nov 2017 11:54:53 +0100 Subject: [PATCH] Collision reduce (#401) * fix collision on vpatch where URL context was sometime ignored * update nginx vers * test case * remove debug --- naxsi_src/Makefile | 2 +- naxsi_src/naxsi_runtime.c | 38 +++++++++++++--------- t/29regression.t | 67 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 91 insertions(+), 16 deletions(-) diff --git a/naxsi_src/Makefile b/naxsi_src/Makefile index 6c7f06e9..964e4862 100644 --- a/naxsi_src/Makefile +++ b/naxsi_src/Makefile @@ -13,7 +13,7 @@ STOCK ?= 1 #Allows to force for specific UT only #TEST := "" -NGINX_VERS := "1.10.0" +NGINX_VERS := "1.12.2" NGINX_OPTIONS="--with-select_module" diff --git a/naxsi_src/naxsi_runtime.c b/naxsi_src/naxsi_runtime.c index ad13d689..24c2bfd5 100644 --- a/naxsi_src/naxsi_runtime.c +++ b/naxsi_src/naxsi_runtime.c @@ -1184,8 +1184,6 @@ ngx_http_spliturl_ruleset(ngx_pool_t *pool, int len, full_len; int nullbytes=0; - NX_DEBUG(_debug_spliturl_ruleset, NGX_LOG_DEBUG_HTTP, req->connection->log, 0, - "XX-check url-like [%s]", str); if (naxsi_escape_nullbytes(nx_str) > 0) { @@ -1196,6 +1194,9 @@ ngx_http_spliturl_ruleset(ngx_pool_t *pool, } str = (char *)nx_str->data; + NX_DEBUG(_debug_spliturl_ruleset, NGX_LOG_DEBUG_HTTP, req->connection->log, 0, + "XX-check url-like [%s]", str); + orig = str; full_len = strlen(orig); while (str < (orig+full_len) && *str) { @@ -1403,7 +1404,7 @@ ngx_http_basestr_ruleset_n(ngx_pool_t *pool, "XX-RULE %d : START", r[i].rule_id); /* does the rule have a custom location ? custom location means checking only on a specific argument */ - if (name && name->len > 0 && r[i].br->custom_location) { + if (name && r[i].br->custom_location) { location = r[i].br->custom_locations->elts; /* @@ -1415,18 +1416,23 @@ ngx_http_basestr_ruleset_n(ngx_pool_t *pool, */ for (z = 0; z < r[i].br->custom_locations->nelts; z++) { - if (location[z].specific_url) { - + if (location[z].specific_url) { /* if matchzone is a regex, ensure it matches (ie. BODY_VAR_X / ARGS_VAR_X / ..) */ - if (r[i].br->rx_mz && ngx_http_dummy_pcre_wrapper(location[z].target_rx, req->uri.data, req->uri.len) == -1) - uri_constraint_ok = 0; + if (r[i].br->rx_mz) { + + if (ngx_http_dummy_pcre_wrapper(location[z].target_rx, req->uri.data, req->uri.len) == -1) { + uri_constraint_ok = 0; + } + } /* if it was a static string, ensure it matches (ie. BODY_VAR / ARGS_VAR / ..) */ - if ( (!r[i].br->rx_mz) && strncasecmp((const char *) req->uri.data, - (const char *) location[z].target.data, - req->uri.len) ) - uri_constraint_ok = 0; - + if (!r[i].br->rx_mz) { + if (req->uri.len != location[z].target.len || strncasecmp((const char *) req->uri.data, + (const char *) location[z].target.data, + req->uri.len) != 0) { + uri_constraint_ok = 0; + } + } break; } } @@ -1435,8 +1441,11 @@ ngx_http_basestr_ruleset_n(ngx_pool_t *pool, ** if one of the custom location rule specifies an $URL/$URL_X ** and it was mismatched, skip the rule. */ - if (uri_constraint_ok == 0) + if (uri_constraint_ok == 0) { + NX_DEBUG(_debug_basestr_ruleset , NGX_LOG_DEBUG_HTTP, req->connection->log, 0, + "XX URI CONSTRAINT MISMATCH, SKIP"); continue; + } /* for each custom location */ for (z = 0; z < r[i].br->custom_locations->nelts; z++) { @@ -1447,7 +1456,7 @@ ngx_http_basestr_ruleset_n(ngx_pool_t *pool, !(zone == HEADERS && location[z].headers_var != 0) && !(zone == ARGS && location[z].args_var != 0)) continue; - + /* if matchzone is a regex, ensure it matches (ie. BODY_VAR_X / ARGS_VAR_X / ..) */ if (r[i].br->rx_mz && ngx_http_dummy_pcre_wrapper(location[z].target_rx, name->data, name->len) == -1) continue; @@ -1459,7 +1468,6 @@ ngx_http_basestr_ruleset_n(ngx_pool_t *pool, location[z].target.len)) ) continue; - NX_DEBUG(_debug_basestr_ruleset, NGX_LOG_DEBUG_HTTP, req->connection->log, 0, "XX-[SPECIFIC] check one rule [%d] iteration %d * %d", r[i].rule_id, i, z); diff --git a/t/29regression.t b/t/29regression.t index b49ddc68..236e03d4 100644 --- a/t/29regression.t +++ b/t/29regression.t @@ -209,3 +209,70 @@ use URI::Escape; "POST /wp-json/wp/v2/posts/111 id=1a&foo2=bar2" --- error_code: 412 +=== WL TEST 3.0: false-positive on virtual-patch with empty var name +--- main_config +load_module /tmp/naxsi_ut/modules/ngx_http_naxsi_module.so; +--- http_config +include /tmp/naxsi_ut/naxsi_core.rules; +MainRule "rx:FOOBAR" "mz:$URL:/wp-includes/js/plupload/plupload.flash.swf|ARGS" "msg:Wordpress PlUpload XSS" "s:$UWA:8,$XSS_UWA:1" id:42000485; +--- config +location / { + SecRulesEnabled; + CheckRule "$LOG_TEST >= 1" LOG; + CheckRule "$UWA >= 8" BLOCK; + DeniedUrl "/RequestDenied"; + CheckRule "$SQL >= 4" BLOCK; + root $TEST_NGINX_SERVROOT/html/; + index index.html index.htm; +} +location /RequestDenied { + return 412; +} +--- request +GET /?a=bui&FOOBAR +--- error_code: 200 +=== WL TEST 3.0: false-positive on virtual-patch with empty var name +--- main_config +load_module /tmp/naxsi_ut/modules/ngx_http_naxsi_module.so; +--- http_config +include /tmp/naxsi_ut/naxsi_core.rules; +MainRule "rx:FOOBAR" "mz:$URL:/wp-includes/js/plupload/plupload.flash.swf|ARGS" "msg:Wordpress PlUpload XSS" "s:$UWA:8,$XSS_UWA:1" id:42000485; +--- config +location / { + SecRulesEnabled; + CheckRule "$LOG_TEST >= 1" LOG; + CheckRule "$UWA >= 8" BLOCK; + DeniedUrl "/RequestDenied"; + CheckRule "$SQL >= 4" BLOCK; + root $TEST_NGINX_SERVROOT/html/; + index index.html index.htm; +} +location /RequestDenied { + return 412; +} +--- request +GET /wp-includes/js/plupload/plupload.flash.swf?a=bui&FOOBAR +--- error_code: 412 +=== WL TEST 3.01: false-positive on virtual-patch with empty var name +--- main_config +load_module /tmp/naxsi_ut/modules/ngx_http_naxsi_module.so; +--- http_config +include /tmp/naxsi_ut/naxsi_core.rules; +MainRule "rx:FOOBAR" "mz:$URL:/wp-includes/js/plupload/plupload.flash.swf|ARGS" "msg:Wordpress PlUpload XSS" "s:$UWA:8,$XSS_UWA:1" id:42000485; +--- config +location / { + SecRulesEnabled; + CheckRule "$LOG_TEST >= 1" LOG; + CheckRule "$UWA >= 8" BLOCK; + DeniedUrl "/RequestDenied"; + CheckRule "$SQL >= 4" BLOCK; + root $TEST_NGINX_SERVROOT/html/; + index index.html index.htm; +} +location /RequestDenied { + return 412; +} +--- request +GET /wp-includes/js/plupload/plupload.flash.swf/xxx/?a=bui&FOOBAR +--- error_code: 404 +