From 768a76a61ee39df59dca13395829e2d411e91ff3 Mon Sep 17 00:00:00 2001 From: Felipe Zimmerle Date: Tue, 2 Jan 2018 20:54:44 -0300 Subject: [PATCH] perf. improvement/rx: Only compute dynamic regex in case of macro On #1528 was added the support for macro expansion on @rx operator. The performance improvement suggested on the pull request was not thread safe, therefore removed. This patch adds a performance improvement on top of #1528. The benchmarks points to 10x faster results on OWASP CRS. --- src/macro_expansion.cc | 14 ++++++++++++- src/macro_expansion.h | 2 ++ src/operators/rx.cc | 21 ++++++++++++++++--- src/operators/rx.h | 19 ++++++++++++++--- src/parser/seclang-parser.yy | 5 +++++ test/benchmark/basic_rules.conf | 2 -- test/benchmark/benchmark.cc | 25 +---------------------- test/benchmark/download-owasp-v3-rules.sh | 2 +- 8 files changed, 56 insertions(+), 34 deletions(-) diff --git a/src/macro_expansion.cc b/src/macro_expansion.cc index c0fd6880..538abef8 100644 --- a/src/macro_expansion.cc +++ b/src/macro_expansion.cc @@ -48,8 +48,20 @@ std::string MacroExpansion::expand(const std::string& input, } +bool MacroExpansion::containsMacro(std::string& input) { + size_t pos = input.find("%{"); + size_t end = input.find("}"); + + if (pos == std::string::npos || end == std::string::npos) { + return false; + } + + return true; +} + + std::string MacroExpansion::expand(const std::string& input, - modsecurity::Rule *rule, Transaction *transaction) { + modsecurity::Rule *rule, Transaction *transaction) { std::string res; size_t pos = input.find("%{"); diff --git a/src/macro_expansion.h b/src/macro_expansion.h index 3436d56f..30e90251 100644 --- a/src/macro_expansion.h +++ b/src/macro_expansion.h @@ -47,6 +47,8 @@ class MacroExpansion { return toupper(aa) == bb; }); } + + static bool containsMacro(std::string& input); }; diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 8a9efd80..b980e17c 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -28,6 +28,15 @@ namespace modsecurity { namespace operators { +bool Rx::init(const std::string &arg, std::string *error) { + m_containsMacro = MacroExpansion::containsMacro(m_param); + if (m_containsMacro == false) { + m_re = new Regex(m_param); + } + + return true; +} + bool Rx::evaluate(Transaction *transaction, Rule *rule, const std::string& input, std::shared_ptr ruleMessage) { @@ -39,8 +48,12 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, return true; } - std::string eparam = MacroExpansion::expand(m_param, transaction); - re = new Regex(eparam); + if (m_containsMacro) { + std::string eparam = MacroExpansion::expand(m_param, transaction); + re = new Regex(eparam); + } else { + re = m_re; + } matches = re->searchAll(input); if (rule && rule->getActionsByName("capture").size() > 0 && transaction) { @@ -62,7 +75,9 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, logOffset(ruleMessage, i.m_offset, i.m_length); } - delete re; + if (m_containsMacro) { + delete re; + } if (matches.size() > 0) { return true; diff --git a/src/operators/rx.h b/src/operators/rx.h index 3e8862be..d8411907 100644 --- a/src/operators/rx.h +++ b/src/operators/rx.h @@ -36,17 +36,25 @@ class Rx : public Operator { public: /** @ingroup ModSecurity_Operator */ Rx(std::string op, std::string param, bool negation) - : Operator(op, param, negation) { + : Operator(op, param, negation), + m_containsMacro(false) { } Rx(std::string name, std::string param) - : Operator(name, param) { + : Operator(name, param), + m_containsMacro(false) { } explicit Rx(std::string param) - : Operator("Rx", param) { + : Operator("Rx", param), + m_containsMacro(false) { } ~Rx() { + if (m_containsMacro == false && m_re != NULL) { + delete m_re; + m_re = NULL; + } } + bool evaluate(Transaction *transaction, Rule *rule, const std::string &input) override { return evaluate(transaction, NULL, input, NULL); @@ -58,6 +66,11 @@ class Rx : public Operator { bool evaluate(Transaction *transaction, Rule *rule, const std::string& input, std::shared_ptr ruleMessage) override; + + bool init(const std::string &arg, std::string *error); + private: + bool m_containsMacro; + Regex *m_re; }; diff --git a/src/parser/seclang-parser.yy b/src/parser/seclang-parser.yy index 0901c8ef..3b41dd1d 100644 --- a/src/parser/seclang-parser.yy +++ b/src/parser/seclang-parser.yy @@ -1027,6 +1027,11 @@ op_before_init: | OPERATOR_RX FREE_TEXT { OPERATOR_CONTAINER($$, new operators::Rx($2)); + std::string error; + if ($$->init(driver.ref.back(), &error) == false) { + driver.error(@0, error); + YYERROR; + } } | OPERATOR_STR_EQ FREE_TEXT { diff --git a/test/benchmark/basic_rules.conf b/test/benchmark/basic_rules.conf index 14c94185..d6e13db2 100644 --- a/test/benchmark/basic_rules.conf +++ b/test/benchmark/basic_rules.conf @@ -3,5 +3,3 @@ Include "../../modsecurity.conf-recommended" Include "owasp-v3/crs-setup.conf.example" Include "owasp-v3/rules/*.conf" -Include "owasp-v3/crs-setup.conf.example" -Include "owasp-v3/rules/*.conf" diff --git a/test/benchmark/benchmark.cc b/test/benchmark/benchmark.cc index 6c55e8f0..0ec083f1 100644 --- a/test/benchmark/benchmark.cc +++ b/test/benchmark/benchmark.cc @@ -24,31 +24,8 @@ using modsecurity::Transaction; -char request_header[] = "" \ - "GET /tutorials/other/top-20-mysql-best-practices/ HTTP/1.1\n\r" \ - "Host: net.tutsplus.com\n\r" \ - "User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.5)" \ - " Gecko/20091102 Firefox/3.5.5 (.NET CLR 3.5.30729)\n\r" \ - "Accept: text/html,application/xhtml+xml,application/xml; " \ - "q=0.9,*/*;q=0.8\n\r" \ - "Accept-Language: en-us,en;q=0.5\n\r" \ - "Accept-Encoding: gzip,deflate\n\r" \ - "Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7\n\r" \ - "Keep-Alive: 300\n\r" \ - "Connection: keep-alive\n\r" \ - "Cookie: PHPSESSID=r2t5uvjq435r4q7ib3vtdjq120\n\r" \ - "Pragma: no-cache\n\r" \ - "Cache-Control: no-cache\n\r"; - char request_uri[] = "/test.pl?param1=test¶2=test2"; -char request_body[] = ""; - -char response_headers[] = "" \ - "HTTP/1.1 200 OK\n\r" \ - "Content-Type: text/xml; charset=utf-8\n\r" \ - "Content-Length: length\n\r"; - unsigned char response_body[] = "" \ "\n\r" \ "> basic_rules.conf