Add proper error handling to @rx operator

Author: WGH-

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(&regex_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(&regex_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;
     }

src/operators/verify_cc.cc

@@ -69,8 +69,9 @@ int VerifyCC::luhnVerify(const char *ccnumber, int len) {
 bool VerifyCC::init(const std::string &param2, 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(&regex_error)) {
+        *error = "Failed to compile regular expression " + m_re->getPattern() + ": " + regex_error;
         return false;
     }
     return true;

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<RegexMatch> 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;

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;
+    }
 }
 
 

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<RegexMatch> 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

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<RegexMatch> searchAll(const std::string& s, bool overlapping = false) const override;

src/regex/backend_fallback.h

@@ -45,8 +45,8 @@ class BackendFallback : public backend::Backend {
         : backend(compile_regex_fallback<Args...>(pattern))
     {}
 
-    virtual bool ok() const override {
-        return backend->ok();
+    virtual bool ok(std::string *error = nullptr) const override {
+        return backend->ok(error);
     }
 
     std::vector<RegexMatch> searchAll(const std::string& s, bool overlapping = false) const override {

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&param2=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\""
+    ]
   }
 ]