diff --git a/src/operators/rx.cc b/src/operators/rx.cc index 455044a4..7ba446b8 100644 --- a/src/operators/rx.cc +++ b/src/operators/rx.cc @@ -29,7 +29,12 @@ namespace operators { bool Rx::init(const std::string &arg, std::string *error) { if (m_string->m_containsMacro == false) { + std::string regex_error; m_re = new Regex(m_param); + if (!m_re->ok(®ex_error)) { + *error = "Failed to compile regular expression " + m_re->getPattern() + ": " + regex_error; + return false; + } } return true; @@ -47,6 +52,14 @@ bool Rx::evaluate(Transaction *transaction, Rule *rule, if (m_string->m_containsMacro) { std::string eparam(m_string->evaluate(transaction)); re = new Regex(eparam); + std::string regex_error; + if (!re->ok(®ex_error)) { + ms_dbg_a(transaction, 2, + "Failed to compile regular expression with macro " + + re->getPattern() + ": " + regex_error); + delete re; + return false; + } } else { re = m_re; } diff --git a/src/operators/verify_cc.cc b/src/operators/verify_cc.cc index 6605d01e..03603d63 100644 --- a/src/operators/verify_cc.cc +++ b/src/operators/verify_cc.cc @@ -69,8 +69,9 @@ int VerifyCC::luhnVerify(const char *ccnumber, int len) { bool VerifyCC::init(const std::string ¶m2, std::string *error) { m_re.reset(new modsecurity::regex::Regex(m_param)); - if (!m_re->ok()) { - *error = "Failed to compile regular expression " + m_re->getPattern(); + std::string regex_error; + if (!m_re->ok(®ex_error)) { + *error = "Failed to compile regular expression " + m_re->getPattern() + ": " + regex_error; return false; } return true; diff --git a/src/regex/backend/backend.h b/src/regex/backend/backend.h index e3bcd3a6..fb4ada9f 100644 --- a/src/regex/backend/backend.h +++ b/src/regex/backend/backend.h @@ -28,7 +28,7 @@ class Backend { public: virtual ~Backend() {} - virtual bool ok() const = 0; + virtual bool ok(std::string *error = nullptr) const = 0; virtual std::vector searchAll(const std::string& s, bool overlapping = false) const = 0; virtual bool search(const std::string &s, RegexMatch *m = nullptr, ssize_t max_groups = -1) const = 0; diff --git a/src/regex/backend/pcre.cc b/src/regex/backend/pcre.cc index bb13d5fd..b01ee3d7 100644 --- a/src/regex/backend/pcre.cc +++ b/src/regex/backend/pcre.cc @@ -43,10 +43,28 @@ Pcre::Pcre(const std::string& pattern_) m_pc = pcre_compile(pattern.c_str(), PCRE_DOTALL|PCRE_MULTILINE, &errptr, &erroffset, NULL); + if (m_pc == NULL) { + m_error = "pcre_compile error at offset " + std::to_string(erroffset) + ": " + std::string(errptr); + return; + } m_pce = pcre_study(m_pc, pcre_study_opt, &errptr); + if (m_pce == NULL) { + m_error = "pcre_study error: " + std::string(errptr); + pcre_free(m_pc); + m_pc = nullptr; + return; + } - pcre_fullinfo(m_pc, m_pce, PCRE_INFO_CAPTURECOUNT, &m_capture_count); + int res = pcre_fullinfo(m_pc, m_pce, PCRE_INFO_CAPTURECOUNT, &m_capture_count); + if (res != 0) { + // N.B. There's no need to free m_pce here, because it's freed in + // destructor anyway. However, m_pc must be freed and set to NULL + // here so error condition is properly detected by ok() method. + m_error = "pcre_fullinfo error: code " + std::to_string(res); + pcre_free(m_pc); + m_pc = nullptr; + } } diff --git a/src/regex/backend/pcre.h b/src/regex/backend/pcre.h index a1b99d49..d4249233 100644 --- a/src/regex/backend/pcre.h +++ b/src/regex/backend/pcre.h @@ -43,8 +43,15 @@ class Pcre : public Backend { Pcre(const Pcre&) = delete; Pcre& operator=(const Pcre&) = delete; - virtual bool ok() const override { - return m_pc != NULL; + virtual bool ok(std::string *error = nullptr) const override { + if (m_pc != NULL) { + return true; + } + if (error != nullptr) { + *error= m_error; + } + + return false; } std::vector searchAll(const std::string& s, bool overlapping = false) const override; @@ -60,6 +67,8 @@ class Pcre : public Backend { pcre *m_pc = NULL; pcre_extra *m_pce = NULL; + + std::string m_error; }; #endif diff --git a/src/regex/backend/re2.h b/src/regex/backend/re2.h index aeb09ba1..25f2dfaf 100644 --- a/src/regex/backend/re2.h +++ b/src/regex/backend/re2.h @@ -40,8 +40,14 @@ class Re2 : public Backend { Re2(const Re2&) = delete; Re2& operator=(const Re2&) = delete; - virtual bool ok() const override { - return re.ok(); + virtual bool ok(std::string *error = nullptr) const override { + if (re.ok()) { + return true; + } + if (error != nullptr) { + *error = re.error(); + } + return false; } std::vector searchAll(const std::string& s, bool overlapping = false) const override; diff --git a/src/regex/backend_fallback.h b/src/regex/backend_fallback.h index 185460d2..fecd1b35 100644 --- a/src/regex/backend_fallback.h +++ b/src/regex/backend_fallback.h @@ -45,8 +45,8 @@ public: : backend(compile_regex_fallback(pattern)) {} - virtual bool ok() const override { - return backend->ok(); + virtual bool ok(std::string *error = nullptr) const override { + return backend->ok(error); } std::vector searchAll(const std::string& s, bool overlapping = false) const override { diff --git a/test/test-cases/regression/operator-rx.json b/test/test-cases/regression/operator-rx.json index d6b9839f..cebba8e6 100644 --- a/test/test-cases/regression/operator-rx.json +++ b/test/test-cases/regression/operator-rx.json @@ -85,5 +85,62 @@ "SecRuleEngine On", "SecRule REQUEST_HEADERS:Content-Length \"!^0$\" \"id:1,phase:2,pass,t:trim,block\"" ] + }, + { + "enabled":1, + "version_min":300000, + "version_max":0, + "title":"Testing Operator :: @rx with invalid regular expression", + "expected":{ + "parser_error":"Rules error.*Failed to compile regular expression \\(\\(value1\\):" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@rx ((value1)\" \"id:1,phase:2,pass,t:trim\"" + ] + }, + { + "enabled":1, + "version_min":300000, + "title":"Testing Operator :: @rx with invalid regular expression after macro expansion", + "client":{ + "ip":"200.249.12.31", + "port":123 + }, + "server":{ + "ip":"200.249.12.31", + "port":80 + }, + "request":{ + "headers":{ + "Host":"localhost", + "User-Agent":"curl/7.38.0", + "Accept":"*/*", + "Content-Length": "27", + "Content-Type": "application/x-www-form-urlencoded" + }, + "uri":"/", + "method":"POST", + "body": [ + "param1=value1¶m2=value2" + ] + }, + "response":{ + "headers":{ + "Date":"Mon, 13 Jul 2015 20:02:41 GMT", + "Last-Modified":"Sun, 26 Oct 2014 22:33:37 GMT", + "Content-Type":"text/html" + }, + "body":[ + "no need." + ] + }, + "expected":{ + "debug_log":"Failed to compile regular expression with macro \\(\\(\\):" + }, + "rules":[ + "SecRuleEngine On", + "SecRule ARGS \"@rx ((%{TX.DOESNT_NEED_TO_EXIST_IT_WILL_BE_AN_EMPTY_STRING})\" \"id:1,phase:2,pass,t:trim\"" + ] } ]